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

igraph: don't enable LTO on darwin #122058

Merged
merged 1 commit into from
May 7, 2021
Merged

igraph: don't enable LTO on darwin #122058

merged 1 commit into from
May 7, 2021

Conversation

mweinelt
Copy link
Member

@mweinelt mweinelt commented May 7, 2021 •
edited
Loading

Motivation for this change

Package was failing on darwin only. Unable to test due to lack of darwin machine.

https://nix-cache.s3.amazonaws.com/log/qn3p5hjrprdc9c7phv0mfl89l2k79z8q-igraph-0.9.3.drv

An alternative would be to rely on autodetection, which exists since 0.9.1. But this change should be the safer bet.

The IGRAPH_ENABLE_LTO build option now supports the AUTO value, which uses LTO only if the compiler supports it. Warning: CMake may not always be able to detect that LTO is not fully supported. Therefore, the default setting is OFF.

ZHF #122042

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.

@ofborg ofborg bot added the 6.topic: darwin Running or building packages on Darwin label May 7, 2021
jonringer
jonringer approved these changes May 7, 2021
View reviewed changes
Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

diff LGTM

We can merge when darwin ofborg build succeeds

@ofborg ofborg bot requested a review from dotlambda May 7, 2021 17:51
@rmcgibbo
Copy link
Contributor

rmcgibbo commented May 7, 2021 •
edited
Loading

Built fine for me on x86_64-darwin, but I did get a few test failures. My mac is old and clunky, so it could be that.

builder for '/nix/store/ia2ns7izrbkdw3s75cxhykgaqk7dgx57-igraph-0.9.3.drv' failed with exit code 2; last 10 log lines:
         369 - test::scg3 (Failed)
      381 - test::igraph_adjacency_spectral_embedding (Failed)
  Errors while running CTest
  make[3]: *** [tests/CMakeFiles/check.dir/build.make:77: tests/CMakeFiles/check] Error 8
  make[3]: Leaving directory '/private/tmp/nix-build-igraph-0.9.3.drv-0/source/build'
  make[2]: *** [CMakeFiles/Makefile2:6072: tests/CMakeFiles/check.dir/all] Error 2
  make[2]: Leaving directory '/private/tmp/nix-build-igraph-0.9.3.drv-0/source/build'
  make[1]: *** [CMakeFiles/Makefile2:6079: tests/CMakeFiles/check.dir/rule] Error 2
  make[1]: Leaving directory '/private/tmp/nix-build-igraph-0.9.3.drv-0/source/build'
  make: *** [Makefile:2751: check] Error 2

A lot of the errors look like this. Probably unrelated to this PR

igraph> 311/393 Test #314: test::igraph_eigen_matrix2.........................***Failed    0.09 sec
igraph> dyld: lazy symbol binding failed: Symbol not found: _dgeev_
igraph> Referenced from: /private/tmp/nix-build-igraph-0.9.3.drv-0/source/build/src/libigraph.0.dylib
igraph> Expected in: /usr/lib/libblas.dylib
igraph> dyld: Symbol not found: _dgeev_
igraph> Referenced from: /private/tmp/nix-build-igraph-0.9.3.drv-0/source/build/src/libigraph.0.dylib
igraph> Expected in: /usr/lib/libblas.dylib

@mweinelt
Copy link
Member Author

mweinelt commented May 7, 2021

Thanks for giving this a shot. If you have an idea on how to fix the tests I'm all ears.

@dotlambda
Copy link
Member

An alternative would be to rely on autodetection, which exists since 0.9.1. But this change should be the safer bet.

The IGRAPH_ENABLE_LTO build option now supports the AUTO value, which uses LTO only if the compiler supports it. Warning: CMake may not always be able to detect that LTO is not fully supported. Therefore, the default setting is OFF.

If @rmcgibbo can test it, we should give AUTO a shot.

@mweinelt
Copy link
Member Author

mweinelt commented May 7, 2021

If @rmcgibbo can test it, we should give AUTO a shot.

Pushed.

@rmcgibbo
Copy link
Contributor

rmcgibbo commented May 7, 2021

Same failures on 48c084a33ee925a3cfb08f51ed6b3bcdb6291c93. I think it might be an issue with my version of osx (which is 10.12.6). it looks like during configure, it's picking up blas from /nix/store/sjbaxyq59vish88xvy3yrki16ga0b33k-blas-3/lib/libblas.dylib, but at runtime it's pickup up blas from /usr/lib/libblas.dylib.

@mweinelt
Copy link
Member Author

mweinelt commented May 7, 2021

No idea how to address that without looking deeper into igraph.

@dotlambda
Copy link
Member

Is setting $LD_LIBRARY_PATH instead of prepending to it problematic?

@mweinelt
Copy link
Member Author

mweinelt commented May 7, 2021

Maybe we need to load blas into LD_LIBRAY_PATH?

dotlambda
dotlambda reviewed May 7, 2021
View reviewed changes
pkgs/development/libraries/igraph/default.nix Outdated
@@ -86,7 +86,7 @@ stdenv.mkDerivation rec {

preCheck = ''
# needed to find libigraph.so
export LD_LIBRARY_PATH="$PWD/src"
export LD_LIBRARY_PATH="${lib.getLib blas}/lib:$PWD/src"
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
export LD_LIBRARY_PATH="${lib.getLib blas}/lib:$PWD/src"
export LD_LIBRARY_PATH="${lib.makeLibraryPath [ blas ]}:$PWD/src"

just in case we add something else in the future?

Copy link
Contributor

Choose a reason for hiding this comment

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

trying this now

Copy link
Member Author

Choose a reason for hiding this comment

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

Applied.

Copy link
Contributor

Choose a reason for hiding this comment

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

This didn't work, but this did:

diff --git a/pkgs/development/libraries/igraph/default.nix b/pkgs/development/libraries/igraph/default.nix
index 911c77109c6..a060440456e 100644
--- a/pkgs/development/libraries/igraph/default.nix
+++ b/pkgs/development/libraries/igraph/default.nix
@@ -86,7 +86,8 @@ stdenv.mkDerivation rec {
 
   preCheck = ''
     # needed to find libigraph.so
-    export LD_LIBRARY_PATH="$PWD/src"
+    # export LD_LIBRARY_PATH="$PWD/src"
+    export DYLD_LIBRARY_PATH="${lib.makeLibraryPath [ blas ]}:$PWD/src"
   '';
 
   postInstall = ''

Copy link
Member Author

Choose a reason for hiding this comment

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

Integrated.

@dotlambda
Copy link
Member

I think building python3.pkgs.igraph is a good smoketest for this change, but its tests are sadly a little flaky.

@dotlambda
Copy link
Member

We can merge when darwin ofborg build succeeds

I think ofborg doesn't build on Darwin anymore

@r-rmcgibbo
Copy link

r-rmcgibbo commented May 7, 2021 •
edited
Loading

Result of nixpkgs-review pr 122058 at 48c084a3 run on aarch64-linux 1

2 packages failed to build:
1 package skipped due to time constraints:
  • python38Packages.scikit-tda
6 packages built successfully:
  • hal-hardware-analyzer
  • igraph
  • python38Packages.cozy
  • python38Packages.python-igraph
  • python39Packages.cozy
  • python39Packages.python-igraph

Note that build failures may predate this PR, and could be nondeterministic or hardware dependent.
Please exercise your independent judgement.

Result of nixpkgs-review pr 122058 at cbe9ec6 run on x86_64-linux 1

2 packages failed to build:
1 package skipped due to time constraints:
  • python38Packages.scikit-tda
6 packages built successfully:
  • hal-hardware-analyzer
  • igraph
  • python38Packages.cozy
  • python38Packages.python-igraph
  • python39Packages.cozy
  • python39Packages.python-igraph

Note that build failures may predate this PR, and could be nondeterministic or hardware dependent.
Please exercise your independent judgement.

@mweinelt mweinelt force-pushed the igraph branch 2 times, most recently from 77e2032 to 2d8d367 Compare May 7, 2021 19:27
@ofborg ofborg bot requested a review from dotlambda May 7, 2021 19:29
dotlambda
dotlambda approved these changes May 7, 2021
View reviewed changes
Copy link
Member

@dotlambda dotlambda left a comment

Choose a reason for hiding this comment

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

Nice!

pkgs/development/libraries/igraph/default.nix Outdated
export LD_LIBRARY_PATH="$PWD/src"

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed!

rmcgibbo
rmcgibbo approved these changes May 7, 2021
View reviewed changes
Copy link
Contributor

@rmcgibbo rmcgibbo left a comment

Choose a reason for hiding this comment

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

Builds on darwin for me.

@mweinelt
igraph: enable autodetection for LTO support
cbe9ec6 
> The IGRAPH_ENABLE_LTO build option now supports the AUTO value, which
> uses LTO only if the compiler supports it. Warning: CMake may not
> always be able to detect that LTO is not fully supported. Therefore,
> the default setting is OFF.

https://igraph.org/2021/03/23/igraph-0.9.1-c.html
@ofborg ofborg bot requested a review from dotlambda May 7, 2021 19:56
@dotlambda dotlambda merged commit 2d64c6d into NixOS:master May 7, 2021
@mweinelt mweinelt deleted the igraph branch May 7, 2021 22:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonringer jonringer jonringer approved these changes

@rmcgibbo rmcgibbo rmcgibbo approved these changes

@dotlambda dotlambda Awaiting requested review from dotlambda

Assignees
No one assigned
Labels
6.topic: darwin Running or building packages on Darwin 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mweinelt @rmcgibbo @dotlambda @r-rmcgibbo @jonringer