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

diffoscope: wrap executable and include tools in PATH #27534

Merged
merged 1 commit into from
Jul 21, 2017
Merged

Conversation

KaiHa
Copy link
Contributor

@KaiHa KaiHa commented Jul 21, 2017

Motivation for this change

diffoscope was looking for the tools it uses during runtime, but the
tools there neither part of the closure nor were they in the
PATH. This commit fixes this.

Things done

Wrapped the diffoscope executable.

  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • 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 nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

diffoscope was looking for the tools it uses during runtime, but the
tools there neither part of the closure nor were they in the
PATH. This commit fixes this.
@mention-bot
Copy link

@KaiHa, thanks for your PR! By analyzing the history of the files in this pull request, we identified @FRidh, @dezgeg and @edolstra to be potential reviewers.

@Mic92 Mic92 merged commit e66195d into NixOS:master Jul 21, 2017
@Mic92
Copy link
Member

Mic92 commented Jul 21, 2017

Thanks!

@Mic92
Copy link
Member

Mic92 commented Jul 21, 2017

Also fixed in 17.03.

@edolstra
Copy link
Member

Doesn't this create a double wrapper script?

edolstra added a commit that referenced this pull request Jul 21, 2017
Alternative fix for #27534 that prevents the use of a double wrapper
(creating even uglier script names than usual, like
..diffoscope-wrapped-wrapped).

This was my bad in 96d7f35.
@Mic92
Copy link
Member

Mic92 commented Jul 21, 2017

ah, right. An alternative would been to use makeWrapperArgs

@KaiHa
Copy link
Contributor Author

KaiHa commented Jul 21, 2017 via email

@KaiHa
Copy link
Contributor Author

KaiHa commented Jul 21, 2017 via email

@Mic92
Copy link
Member

Mic92 commented Jul 21, 2017

See also 91dc811#commitcomment-23226040

bugworm pushed a commit to bugworm/nixpkgs that referenced this pull request Jul 23, 2017
Alternative fix for NixOS#27534 that prevents the use of a double wrapper
(creating even uglier script names than usual, like
..diffoscope-wrapped-wrapped).

This was my bad in 96d7f35.
edolstra added a commit that referenced this pull request Jul 28, 2017
Alternative fix for #27534 that prevents the use of a double wrapper
(creating even uglier script names than usual, like
..diffoscope-wrapped-wrapped).

This was my bad in 96d7f35.

(cherry picked from commit 91dc811)
adrianpk added a commit to adrianpk/nixpkgs that referenced this pull request May 31, 2024
Alternative fix for NixOS#27534 that prevents the use of a double wrapper
(creating even uglier script names than usual, like
..diffoscope-wrapped-wrapped).

This was my bad in 96d7f35.

(cherry picked from commit 91dc811)
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.

None yet

4 participants