Skip to content

boost: Do not force numpy support from 1.65#33359

Merged
Ericson2314 merged 1 commit intoNixOS:masterfrom
bnikolic:boostnumpy
Jan 3, 2018
Merged

boost: Do not force numpy support from 1.65#33359
Ericson2314 merged 1 commit intoNixOS:masterfrom
bnikolic:boostnumpy

Conversation

@bnikolic
Copy link
Contributor

@bnikolic bnikolic commented Jan 2, 2018

Motivation for this change

The reason is that if cross compiling (or for other reasons) python bindings as a whole are turned off. Those two lines then trigger assertion errors unless manually overridden for cross compilation.

This way:

  1. The enableNumpy default respects the `enablePython deafult.
  2. Cross works by default
  3. Absurd manual overrides still break as they should
  4. The >= 1.65 logic is direct and not a maintaince gotcha.
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-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/)
  • Fits CONTRIBUTING.md.

@GrahamcOfBorg GrahamcOfBorg added 10.rebuild-darwin: 101-500 This PR causes between 101 and 500 packages to rebuild on Darwin. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. labels Jan 2, 2018
Copy link
Member

@Ericson2314 Ericson2314 left a comment

Choose a reason for hiding this comment

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

Instead of doing a mass rebuild, we can just change the fallback value like so. I think that would be better?

Copy link
Member

Choose a reason for hiding this comment

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

enableNumpy = args.enableNumpy or stdenv.hostPlatform == stdenv.buildPlatform;`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes could do this but it is not ideal: the same logic is repeated in the generic.nix and for each version. And the proper logic would also take into account if enablePython is passed into args: if that is false, enableNumpy should be false too.

Ideally it could all be handled in generic.nix?

Copy link
Member

Choose a reason for hiding this comment

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

diito

(enableNumpy = args.enableNumpy or stdenv.hostPlatform == stdenv.buildPlatform;`)

@Ericson2314 Ericson2314 added the 6.topic: cross-compilation Building packages on a different platform than they will be used on label Jan 2, 2018
@Ericson2314 Ericson2314 changed the base branch from staging to master January 3, 2018 00:20
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

@Ericson2314 Ericson2314 Jan 3, 2018

Choose a reason for hiding this comment

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

This more directly implements 19dbfb6's description "In order to manipulate Python arrays numpy is needed from boost 1.65 on".

@Ericson2314 Ericson2314 requested review from Ericson2314 and globin and removed request for FRidh, edolstra, peti and zimbatm January 3, 2018 00:23
@GrahamcOfBorg GrahamcOfBorg added 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. and removed 10.rebuild-darwin: 101-500 This PR causes between 101 and 500 packages to rebuild on Darwin. labels Jan 3, 2018
The reason is that if cross compiling (or for other reasons) python
bindings as a whole are turned off. Those two lines then trigger
assertion errors unless manually overridden for cross compilation.

This way:

 1. The `enableNumpy` default respects the `enablePython deafult.
 2. Cross works by default
 3. Absurd manual overrides still break as they should
 4. The `>= 1.65` logic is direct and not a maintaince gotcha.
@GrahamcOfBorg GrahamcOfBorg added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. and removed 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. labels Jan 3, 2018
@Ericson2314 Ericson2314 requested review from globin and removed request for Ericson2314 and globin January 3, 2018 00:40
@Ericson2314 Ericson2314 changed the title boost: do not enable numpy unless explicit argument is set boost: Do not force numpy support from 1.65 Jan 3, 2018
@Ericson2314
Copy link
Member

Waited for @GrahamcOfBorg to confirm no rebuilds.

@Ericson2314 Ericson2314 merged commit f3bb3de into NixOS:master Jan 3, 2018
@Ericson2314
Copy link
Member

Btw @bnikolic I feel kind obnoxious pushing to your PRs again and again, sorry about that. I'm just sort feel in a rush after #26805 finally landed, and indeed you are finding good stuff. Hope this is the last time I'll succumb to that temptation.

Copy link
Member

@FRidh FRidh left a comment

Choose a reason for hiding this comment

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

Thanks!

@bnikolic
Copy link
Contributor Author

bnikolic commented Jan 3, 2018 via email

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

Labels

6.topic: cross-compilation Building packages on a different platform than they will be used on 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants