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.

Sign up for GitHub

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

autoPatchelfHook: add autoPatchelfIgnoreMissing #89718

Merged
merged 1 commit into from Jun 22, 2020
Merged

autoPatchelfHook: add autoPatchelfIgnoreMissing #89718

merged 1 commit into from Jun 22, 2020

Conversation

DavHau
Copy link
Member

@DavHau DavHau commented Jun 7, 2020

Motivation for this change

Currently autoPatchelfHook fails as soon as any dependency cannot be satisfied. But in some cases it is favorable to continue patching and ignore the missing dependencies. Some binary distributions contain optional features with their own dependencies. The current autoPatchelfHook forces you to patch all dependencies even if you know you will never use those features.
A specific example would be the python wheel distribution of Pytorch which contains files which depend on cuda, but in practice this dependency doesn't need to be satisfied if you don't plan to use cuda. (pytorch/pytorch#38624)

To solve this problem, I added the possibility to ignore missing dependencies by setting the environment variable autoPatchelfIgnoreMissing.

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.

@DavHau DavHau requested a review from Ericson2314 as a code owner June 7, 2020 07:24
@Ma27 Ma27 requested a review from aszlig June 17, 2020 22:44
@FRidh
Copy link
Member

FRidh commented Jun 19, 2020

Ignore missing what? I suggest appending something to the attribute that indicates what is missing.

aszlig
aszlig approved these changes Jun 20, 2020
View reviewed changes
pkgs/build-support/setup-hooks/auto-patchelf.sh Outdated Show resolved Hide resolved
@aszlig
Copy link
Member

aszlig commented Jun 20, 2020

Ignore missing what? I suggest appending something to the attribute that indicates what is missing.

@FRidh: I also found the naming a bit odd, but haven't found a better name that is to the point and also short. Something like autoPatchelfIgnoreMissingSharedLibraryDependencies clearly is a bit long and apIgnoreMissingLibDeps also sounds weird.

I think just "ignore missing" is okay in the context of autoPatchelf, since the only thing that could be missing are shared library dependencies. Maybe I'm missing something here, but what's the first thing that comes to your mind in this context and "something missing"?

@FRidh
Copy link
Member

FRidh commented Jun 20, 2020

Like you wrote, it seems odd. I was thinking autoPatchelfIgnoreMissingDeps or autoPatchelfIgnoreMissingLibs.

@DavHau DavHau force-pushed the autopatchelf-ignore-missing branch from f8e4940 to 9be312b Compare June 22, 2020 08:28
@DavHau
Copy link
Member Author

DavHau commented Jun 22, 2020

Added the proposed syntax change from @aszlig and renamed the variable to autoPatchelfIgnoreMissingDeps. I tested it again and it works like expected.

@FRidh FRidh changed the base branch from master to staging June 22, 2020 08:41
@FRidh FRidh merged commit c8c09b7 into NixOS:staging Jun 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aszlig aszlig aszlig approved these changes

@Ericson2314 Ericson2314 Awaiting requested review from Ericson2314

Assignees
No one assigned
Labels
8.has: documentation 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@DavHau @FRidh @aszlig