Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

review https://github.com/NixOS/nixpkgs/pull/239450 #1

Merged
merged 2 commits into from
Aug 14, 2023

Conversation

TheBrainScrambler
Copy link

Things changed:

  • split name into pname/version. This is the recommended way to do it, don't know why you use name
  • left the unwrapped binary too in case someone is unhappy with the wrapper or wants to make his own wrapper
  • https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md: "meta.description must: [...] Not end with a period (or any punctuation for that matter)."
    And some minor stuff

@hughobrien
Copy link
Owner

split name into pname/version. This is the recommended way to do it, don't know why you use name

Version wasn't used in the build.nix stage despite being passed in so I rejiggered it. It's obviously harmless though so easier to stick to the recommendations I guess.

Copy link
Owner

@hughobrien hughobrien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the interest of expediency I'm going to commit the suggestions but please yell if you disagree. Thanks again for helping with this.

@hughobrien hughobrien merged commit 85a7b65 into hughobrien:fallout-ce Aug 14, 2023
3 checks passed
@TheBrainScrambler
Copy link
Author

Ah sorry looks like I didn't try to build my review, thought I did it :/ but yes it's good that you fixed those mistakes.

split name into pname/version. This is the recommended way to do it, don't know why you use name

Version wasn't used in the build.nix stage despite being passed in so I rejiggered it. It's obviously harmless though so easier to stick to the recommendations I guess.

Uuh, but it is used ? Line 16: inherit pname version src; Since you kept it there's nothing to change, but just to tell you that this change wasn't just harmless but actually needed. If you specify only name instead of pname/version it's going to think that name is pname-version basically. And since you don't give any version but just give the pname as name it would cause problems.

@TheBrainScrambler TheBrainScrambler deleted the fallout-ce branch August 14, 2023 09:46
hughobrien pushed a commit that referenced this pull request Feb 22, 2024
Since ba83271 the build fails with

    applying patch /nix/store/46rxbbvl2l3mrxb50y9rzy7ahgx0lraj-d741901dddd731895346636c0d3556c6fa51fbe6.patch
    patching file tests/hazmat/primitives/test_aead.py
    Hunk #1 FAILED at 56.
    Hunk #2 FAILED at 197.
    Hunk NixOS#3 FAILED at 378.
    Hunk NixOS#4 FAILED at 525.
    Hunk NixOS#5 FAILED at 700.
    Hunk NixOS#6 FAILED at 844.
    6 out of 6 hunks FAILED -- saving rejects to file tests/hazmat/primitives/test_aead.py.rej
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants