{ecwolf,lzwolf}: fix build with gcc-14, cleanup #368178
Conversation
There was a problem hiding this comment.
This PR mostly looks good to me. I tried building ecwolf without the changes in this PR, and it failed. The changes in this commit fix that failure which is good. (I didn’t test lzwolf, though).
I only really have two critiques:
-
“ecwolf: use finalAttrs” does more than just switch to using
finalAttrs. It also switches from usingrev = <tag-name>;torev = "refs/tags/<tag-name>;. -
In addition to writing properly formatted commit messages, it's important to include relevant information so other developers can later understand why a change was made. While this information usually can be found by digging code, mailing list/Discourse archives, pull request discussions or upstream changes, it may require a lot of work.
I think that the “fix build with gcc-14” commits are self-explanitory, and I think that the commit message for “ecwolf: don't use pname in fetchFromBitbucket” does a good job at explaining why the change was made. I think that the other commits should have longer commit messages that explain why the changes were made.
I created a branch named dev/fix-ecwolf-suggestions. In that branch, I split “ecwolf: use finalAttrs” into two separate commits, and expanded the commit messages that I felt were lacking. If you agree with my changes, then you can push the dev/fix-ecwolf-suggestions branch to your PR branch.
This is discouraged. See NixOS#277994.
Before this change, the ecwolf package used a recursive attribute set. In general, it’s recommended that you avoid using recursive attribute sets [1]. This change makes it so that the ecwolf package uses finalAttrs instead of a recursive attribute set. As an added bonus, this also means that ecwolf.src.rev will automatically get updated if someone calls ecwolf.overrideAttrs and overrides ecwolf.version. [1]: <https://nix.dev/guides/best-practices.html#recursive-attribute-set-rec> Co-authored-by: Jason Yundt <[email protected]>
Before this change, ecwolf.src.rev was set to the name of a tag. This is generally not recommended. It’s better to set rev to "refs/tags/<tag-name>" [1]. [1]: <https://nixos.org/manual/nixpkgs/stable/#fetchgit>
It looks like sha256 will eventually be deprecated in favor of hash: <NixOS#325892>.
It looks like sha256 will eventually be deprecated in favor of hash: <NixOS#325892>.
xokdvium
force-pushed
the
dev/fix-ecwolf
branch
from
4aa37cb to
83378e0
Compare
|
Rebased and cherry-picked commits with the improved descriptions. Thanks for taking the time to fix them up! |
|
I've checked that
Though in the long run I think we should consider dropping
Now it's pretty easy to get it working, but further breakages will be harder to fix and continuing to carry downstream patches is always a burden.
|
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.