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

pagmo2, pythonPackages.pygmo refactor fix broken packages #51399

Closed
wants to merge 3 commits into from

Conversation

costrouc
Copy link
Member

@costrouc costrouc commented Dec 2, 2018

Motivation for this change

Fixed issues with #50645 for merging.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nox --run "nox-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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@Mic92
Copy link
Member

Mic92 commented Dec 2, 2018

@GrahamcOfBorg build pythonPackages.pygmo

@worldofpeace worldofpeace self-assigned this Dec 6, 2018

buildInputs = [ cmake eigen nlopt ipopt boost pagmo2 ];
propagatedBuildInputs = [ numpy cloudpickle ipyparallel numba ];

preBuild = ''
cp -v -r $src/* .
cmake -DCMAKE_INSTALL_PREFIX=$out -DPAGMO_BUILD_TESTS=no -DCMAKE_SYSTEM_NAME=Linux -DPagmo_DIR=${pagmo2} -DPAGMO_BUILD_PYGMO=yes -DPAGMO_BUILD_PAGMO=no -DPAGMO_WITH_EIGEN3=yes -DPAGMO_WITH_NLOPT=yes -DNLOPT_LIBRARY=${nlopt}/lib/libnlopt_cxx.so -DPAGMO_WITH_IPOPT=yes -DIPOPT=${ipopt}
cmake -DCMAKE_INSTALL_PREFIX=$out -DPAGMO_BUILD_TESTS=no -DCMAKE_SYSTEM_NAME=Linux -DPagmo_DIR=${pagmo2} -DPAGMO_BUILD_PYGMO=yes -DPAGMO_BUILD_PAGMO=no -DPAGMO_WITH_EIGEN3=yes -DPAGMO_WITH_NLOPT=yes -DNLOPT_LIBRARY=${nlopt}/lib/libnlopt.so -DPAGMO_WITH_IPOPT=yes
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm seeing

CMake Warning:
  Manually-specified variables were not used by the project:

    PAGMO_BUILD_TESTS
    PAGMO_WITH_EIGEN3
    PAGMO_WITH_IPOPT
    PAGMO_WITH_NLOPT

So I think we can drop those for the pygmo build.

Copy link
Member

Choose a reason for hiding this comment

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

Actually python could become an build output of pagmo2.
There is a function called toPythonModule.
Do you know if that works? @dotlambda for outputs of derivation?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Do you know if that works? @dotlambda for outputs of derivation?

It doesn't because it uses overrideAttrs.

Copy link
Member

@dotlambda dotlambda Dec 11, 2018

Choose a reason for hiding this comment

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

And I think doing anything to use the compilation output of pagmo will fail because we have to compile with different versions of python.

Copy link
Member

Choose a reason for hiding this comment

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

Please also split the lines like this:

cmake -DCMAKE_INSTALL_PREFIX=$out \
  -DPAGMO_BUILD_TESTS=no \
  -DCMAKE_SYSTEM_NAME=Linux
  ...

Do you really have to pass CMAKE_SYSTEM_NAME?

@worldofpeace
Copy link
Contributor

cc @dotlambda

@worldofpeace
Copy link
Contributor

I've addressed our feedback and resolved the conflict.

@GrahamcOfBorg build pagmo2 pythonPackages.pygmo python3Packages.pygmo python3Packages.dftfit

@dotlambda
Copy link
Member

This is what I was able to come up with: dotlambda@4d0a561
I don't know if it's any better or if it's even working correctly.

@Mic92
Copy link
Member

Mic92 commented Dec 12, 2018

@dotlambda I prefer your solution because it does not use bare cmake commands and instead rely on our cmake support hooks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants