Skip to content
This repository was archived by the owner on Dec 13, 2024. It is now read-only.

Include kernel patch directly instead of by github patch url #64

Merged
merged 1 commit into from
Dec 7, 2024

Conversation

jsternberg
Copy link
Contributor

Includes the kernel patch as a file directly instead of relying on fetchurl. The patch returned by fetchurl changed in some manner that invalidated the hash. This method prevents a change to github's download from affecting the build.

@gnull
Copy link
Contributor

gnull commented Dec 2, 2024

Fixes #63

May be a good idea to just change the hash to new value (if we're sure Github is not doing a MitM attack on us).

I like the practice of referencing the patch url instead of copy-pasting patch contents (when we're using the patch unmodified) because this way the source of the patch is better documented. A new dev who's looking at this repo for the first time won't have to wonder where this patch came from.

@jsternberg
Copy link
Contributor Author

Unfortunately, the reason I ran into this issue is because the previous store path had been gc'd away so I don't have the old patch. My conclusion that github must have changed something in their patch is mostly because it worked at some point so the old hash had to work and the hash now doesn't work and the only thing that could possibly change is the downloaded patch.

While I do like the idea of just changing the hash, I don't think it's necessarily reliable to assume that the patch Github sends is stable. If this happened once, it's likely to happen again. There's a certain reliability to including the kernel patch directly that's hard to beat. I've included the original hashes as variables and added a comment for how to generate the hash so it's easy for a new dev to find it.

@ryan4yin
Copy link
Owner

ryan4yin commented Dec 4, 2024

To prevent the hash from changing due to certain modifications on GitHub’s end, I prefer this pull request over PR #65.

@ryan4yin
Copy link
Owner

ryan4yin commented Dec 4, 2024

I’ve double-checked and the patch file in this PR is identical to the one from GitHub’s link. I’ll go ahead and merge it as soon as the little thing I mentioned is sorted out. Thanks!

@jsternberg
Copy link
Contributor Author

I'll update the PR when I'm able to get to my personal computer a little later.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants