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

python.buildEnv: install out and lib outputs #29079

Closed
wants to merge 1 commit into from

Conversation

abbradar
Copy link
Member

@abbradar abbradar commented Sep 7, 2017

Motivation for this change

Needed for example for Caffe which places its binaries into a separate bin
output.

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
    • Linux
  • 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.

Needed for example for Caffe which places its binaries into a separate `bin`
output.
@FRidh
Copy link
Member

FRidh commented Sep 7, 2017

With Python libraries we do not use multiple outputs.

Caffe is not a Python library. How about we stick to the buildEnv behaviour, that is,

extraOutputsToInstall ? []

What is needed long-term is a "higher-level" buildEnv that is aware of how to building Python envs, and Haskell envs, and Perls envs, and ...., all together in one env.

@abbradar
Copy link
Member Author

abbradar commented Sep 7, 2017

@FRidh Caffe is actually a Python library (it has Python bindings). The problem is that because they are installed in out and the default output is bin buildEnv gets confused now.

@FRidh
Copy link
Member

FRidh commented Sep 8, 2017

I just saw you added the bindings to python-packages.nix.

In any case, I still think we should not deviate from buildEnv by installing more outputs.

@abbradar
Copy link
Member Author

abbradar commented Sep 8, 2017

Yeah, I see why this is a good approach. Should we then add extraOutputsToInstall?

I still don't like that people will ask questions like "I made a Python environment with Theano and it doesn't work, what's the matter?" but I don't see any better way to do it (apart from making Theano not use multiple outputs but I don't like it...).

@FRidh
Copy link
Member

FRidh commented Sep 8, 2017

Yeah, I see why this is a good approach. Should we then add extraOutputsToInstall?

Yes!

I still don't like that people will ask questions like "I made a Python environment with Theano and it doesn't work, what's the matter?" but I don't see any better way to do it (apart from making Theano not use multiple outputs but I don't like it...).

Don't you mean another package? Theano does not have multiple outputs.

@abbradar
Copy link
Member Author

abbradar commented Sep 8, 2017

@FRidh Argh, sorry, I'm a little tired with messing with all ML libraries at once lately ^_^" I meant caffe.

@FRidh
Copy link
Member

FRidh commented Sep 10, 2017

Closing in favor of a commit that adds extraOutputsToInstall ? []

@FRidh FRidh closed this Sep 10, 2017
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

2 participants