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

suitesparse: rename fixDarwinDylibNames override #96982

Merged
merged 1 commit into from Sep 7, 2020

Conversation

@graham33
Copy link
Contributor

@graham33 graham33 commented Sep 2, 2020

Motivation for this change

This seems to prevent shadowing the regular fixDarwinDylibNames function, which is a nativeBuildInput. Without this change some dylibs still had relative paths embedded. It's possible something deeper is happening, but this change fixed the issue for me. See #96981 for details.

Fixes #96981

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.
@jtojnar
Copy link
Contributor

@jtojnar jtojnar commented Sep 3, 2020

According the comment, we might be able to remove the whole postInstall and fixDarwinDylibNames.

@ttuegel
ttuegel approved these changes Sep 4, 2020
@ttuegel
Copy link
Member

@ttuegel ttuegel commented Sep 4, 2020

According the comment, we might be able to remove the whole postInstall and fixDarwinDylibNames.

That would be better. I don't have a Darwin machine to check, maybe @graham33 could? Still, these changes look reasonable to me, and I'm fine with merging them if they fix the problem.

@graham33
Copy link
Contributor Author

@graham33 graham33 commented Sep 4, 2020

Yes I think this works, just rebuilding to test. Thanks.

This shadows the regular fixDarwinDylibNames function, which is a
nativeBuildInput, and prevents it from converting some relative paths to
absolute.  According to the comment this can be removed as of 5.7.2.
@graham33 graham33 force-pushed the graham33:fix/96981_suitesparse_dylib_names branch from 3b12b5d to e10228d Sep 6, 2020
@graham33
Copy link
Contributor Author

@graham33 graham33 commented Sep 6, 2020

I successfully built and ran octave against the updated commit.

@jtojnar
Copy link
Contributor

@jtojnar jtojnar commented Sep 6, 2020

Thanks for testing. Is it also okay to remove fixDarwinDylibNames from dependencies?

@graham33
Copy link
Contributor Author

@graham33 graham33 commented Sep 6, 2020

I think we still need fixDarwinDylibNames in general, if I remove it entirely we get libraries with relative references, e.g.:

otool -L /nix/store/sx5p4mwhkxxhd2zwm8y7yz1di1abfl1a-suitesparse-5.7.2/lib/libcholmod.dylib                                                 
/nix/store/sx5p4mwhkxxhd2zwm8y7yz1di1abfl1a-suitesparse-5.7.2/lib/libcholmod.dylib:
...
	libamd.2.dylib (compatibility version 2.0.0, current version 2.4.6)
...

vs this with fixDarwinDylibNames still in nativeBuildInputs:

otool -L /nix/store/amyna7fbzk3ag3yp7ghj441fbaq36kpi-suitesparse-5.7.2/lib/libcholmod.dylib 
/nix/store/amyna7fbzk3ag3yp7ghj441fbaq36kpi-suitesparse-5.7.2/lib/libcholmod.dylib:
...
	/nix/store/amyna7fbzk3ag3yp7ghj441fbaq36kpi-suitesparse-5.7.2/lib/libamd.2.dylib (compatibility version 2.0.0, current version 2.4.6)
...

I don't know much about Darwin dylibs, but I think absolute store paths are what we want (and these were the source of the problems I had with Octave).

@ofborg ofborg bot requested a review from ttuegel Sep 7, 2020
@jtojnar jtojnar merged commit 87ec0fb into NixOS:master Sep 7, 2020
19 checks passed
19 checks passed
tests tests
Details
action
Details
Evaluation Performance Report Evaluator Performance Report
Details
Wait for ofborg
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="e10228d"; rev="e10228defa08877593ca31d50e38a430e3cb3c64"; } ./pkgs/t
Details
grahamcofborg-eval-lib-tests nix-build --arg pkgs import ./. {} ./lib/tests/release.nix
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="e10228d"; rev="e10228defa08877593ca31d50e38a430e3cb3c64"; } ./nixos/
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="e10228d"; rev="e10228defa08877593ca31d50e38a430e3cb3c64"; } ./nixos/
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="e10228d"; rev="e10228defa08877593ca31d50e38a430e3cb3c64"; } ./nixos/
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="e10228d"; rev="e10228defa08877593ca31d50e38a430e3cb3c64"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="e10228d"; rev="e10228defa08877593ca31d50e38a430e3cb3c64"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="e10228d"; rev="e10228defa08877593ca31d50e38a430e3cb3c64"; } ./pkgs/t
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
suitesparse, suitesparse.passthru.tests on aarch64-linux Success
Details
suitesparse, suitesparse.passthru.tests on x86_64-linux Success
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.