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

Jump to bottom

buildRustPackages: Fix postPatch phase. #89531

Closed
wants to merge 1 commit into from
Closed

buildRustPackages: Fix postPatch phase. #89531

wants to merge 1 commit into from

Conversation

rbartlensky
Copy link

Since the user's postPatch is simply concatenated to
buildRustPackage's postPatch, it can lead to an invalid command
being run unless the user's postPatch contains a ;. Hopefully this
patch fixes the issue.

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@rbartlensky rbartlensky requested a review from andir as a code owner June 5, 2020 10:00
@SuperSandro2000
Copy link
Member

Hopefully this patch fixes the issue.

Please test this with a patch that contains a ;.

@SuperSandro2000 SuperSandro2000 marked this pull request as draft November 28, 2020 04:52
@rbartlensky
Copy link
Author

Hopefully this patch fixes the issue.

Please test this with a patch that contains a ;.

I am not sure I understand what you are asking for. The workaround to this issue is to use ; (example:
postPatch = "patchShebangs .; ";) , notice the ; in the postPatch. Are you asking whether this particular workaround works?

@SuperSandro2000
Copy link
Member

Are you asking whether this particular workaround works?

yes because

Hopefully this patch fixes the issue.

Is not enough.

@rbartlensky
Copy link
Author

@SuperSandro2000 Sorry for the confusion. I tested this and these are the results:

  • before, if you specified a postPatch = "patchShebangs .", you would get:
patchShebangs . cargoDepsLockfile=$NIX_BUILD_TOP/$cargoDepsCopy/Cargo.lock
srcLockfile=$NIX_BUILD_TOP/$sourceRoot/Cargo.lock
# <....>

Notice that the user's postPatch is on the same line.

  • With this PR you get:
patchShebangs . 
cargoDepsLockfile=$NIX_BUILD_TOP/$cargoDepsCopy/Cargo.lock
srcLockfile=$NIX_BUILD_TOP/$sourceRoot/Cargo.lock
# <...>

which is now correct.

I can also confirm that this PR fixes the issue correctly. Sorry for the confusion and late reply!

SuperSandro2000
SuperSandro2000 reviewed Jan 7, 2021
View reviewed changes
pkgs/build-support/rust/default.nix Outdated
@@ -72,6 +72,7 @@ let
# inputs do not cause us to find the wrong `diff`.
diff = "${diffutils}/bin/diff";

userPostPatch = args.postPatch or "";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
userPostPatch = args.postPatch or "";

pkgs/build-support/rust/default.nix Outdated
@@ -119,7 +120,8 @@ stdenv.mkDerivation (args // {
# After unpacking and applying patches, check that the Cargo.lock matches our
# src package. Note that we do this after the patchPhase, because the
# patchPhase may create the Cargo.lock if upstream has not shipped one.
postPatch = (args.postPatch or "") + stdenv.lib.optionalString validateCargoDeps ''
postPatch = stdenv.lib.optionalString validateCargoDeps ''
${userPostPatch}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
${userPostPatch}
${args.postPatch or ""}

I think that should work.

buildRustPackages: Fix postPatch phase.
3175169 
Since the user's postPatch is simply concatenated to
`buildRustPackage`'s postPatch, it can lead to an invalid command
being run unless the user's postPatch contains a `;`.
@rbartlensky rbartlensky marked this pull request as ready for review January 11, 2021 14:27
@rbartlensky
Copy link
Author

@SuperSandro2000 thanks for the suggestions, I squashed those changes into my original commit

@SuperSandro2000
Copy link
Member

/rebase-staging

@github-actions
Copy link
Contributor

Failed to rebase on staging

@stale
Copy link

stale bot commented Jul 11, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 11, 2021
@Twey
Copy link
Contributor

Twey commented Dec 13, 2021

Not stale AFAIK.

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Dec 13, 2021
@SuperSandro2000
Copy link
Member

This PR needs to be rebased against staging and since that didn't happen for 11 months now it is stale.

@Artturin
Copy link
Member

Artturin commented Apr 7, 2022

fixed

@Artturin Artturin closed this Apr 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SuperSandro2000 SuperSandro2000 SuperSandro2000 left review comments

@andir andir Awaiting requested review from andir

@zowoq zowoq Awaiting requested review from zowoq zowoq is a code owner

Assignees
No one assigned
Labels
2.status: merge conflict 6.topic: rust 10.rebuild-darwin: 101-500 10.rebuild-linux: 501+ 10.rebuild-linux: 2501-5000 12. first-time contribution
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@rbartlensky @SuperSandro2000 @Twey @Artturin @Janik-Haag