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

Add CIBW_ENVIRONMENT option #16

Closed
YannickJadoul opened this issue Jul 5, 2017 · 27 comments
Closed

Add CIBW_ENVIRONMENT option #16

YannickJadoul opened this issue Jul 5, 2017 · 27 comments

Comments

@YannickJadoul
Copy link
Member

YannickJadoul commented Jul 5, 2017

Sorry to open yet another issue, but in my efforts to try getting my wheels built on the manylinux docker image, I'm running into the problem that there seems no way in cibuildwheel to prepare the container (i.e. install stuff and set environment variables).

In my particular case, for example, I want to install a newer GCC toolchain (https://github.com/squeaky-pl/centos-devtools/releases), set the PATH to use it, and set the CXXFLAGS to be picked up by my build.

Am I overlooking a possibilty, or is this currently not possible?

(Probably also related to #13)

@joerick joerick changed the title Setting environment variables in Linux docker container Add CIBW_ENVIRONMENT option Jul 5, 2017
@joerick
Copy link
Contributor

joerick commented Jul 5, 2017

We discussed in #6 the possibility of a CIBW_ENVIRONMENT option. I think that's definitely needed in your case. I'm really busy a the moment, but contributions welcome!

@joerick
Copy link
Contributor

joerick commented Jul 5, 2017

#7 would also be useful for this too

@YannickJadoul
Copy link
Member Author

Thanks! Hadn't found that issue. I'll have a look at that, and see what I can do to implement something and get a pull request!

@YannickJadoul
Copy link
Member Author

Did you have anything in mind on what that CIBW_ENVIRONMENT would do?
Would it be bash code that could install stuff, etc? Or would it be purely a specification of environment variables to be set?

@joerick
Copy link
Contributor

joerick commented Jul 5, 2017

CIBW_ENVIRONMENT would purely set environment variables. We'll need format that'll fit into an environment variable itself - I'm thinking space-separated variables, as parsed by shlex.split() an example value of it would be PATH="/usr/local/bin/:$PATH" CXXFLAGS=-Wall.

@YannickJadoul
Copy link
Member Author

YannickJadoul commented Jul 5, 2017

Sounds reasonable, and fairly easily implementable. I'll start working on that in a bit.

Three more questions/thougts:

  • Should this happen before the main loop over Python version or within that loop?
    I mean, there's up- and downsides to both: the former allows a single execution of things and doesn't get into problems with applying e.g. PATH="/usr/local/bin/:$PATH" multiple times, but the latter allows for customization per build (in case environment variables depend on the Python version).
  • What about installations common for all Python executables (e.g. the GCC suite in my case)? Feels silly to repeat the download and extraction all the time, doesn't it? (You can download before calling cibuildwheel and pass it on, ofc, or you could somehow check, of course, but that gets messy rather quickly.)
  • This is actually only necessary in Linux. Since OS X and Windows don't have docker images, you can just do all these things before calling cibuildwheel. So ... should we make this something specific for Linux, or something extra for all platforms for the sake of consistency?

@YannickJadoul
Copy link
Member Author

YannickJadoul commented Jul 5, 2017

(@tgarc: since you opened #6, any thoughts on this/use cases that would benefit from a certain solution?)

@tgarc
Copy link
Contributor

tgarc commented Jul 5, 2017

@YannickJadoul

[...] should we make this something specific for Windows or [...]

You meant to say Linux here correct? Otherwise I'm confused.

I'll give some input when I get some time but just wanted to ask: What issues/limitations are you having using CIBW_BEFORE_BUILD? It's not ideal I know, but I just want to know what specific issues you were having with it. Are you able to get your use case working using BEFORE_BUILD or is it currently not even possible?

@YannickJadoul
Copy link
Member Author

Thanks for the answer!

You meant to say Linux here correct? Otherwise I'm confused.

Yeah, sorry, I'll edit that.

What issues/limitations are you having using CIBW_BEFORE_BUILD?

Since that's being run in sh -c, you cannot export any environment variables there, because they will not propagate to the parent scope. In particular, in my case, I'd like to export CXXFLAGS="-static-libstdc++", but that's never going to work since that variable will only be set in that subshell.

Are you able to get your use case working using BEFORE_BUILD or is it currently not even possible?

The best I can come up with right now is to move the executable g++ to e.g. g++-old and then write a script to the original g++ in the existing PATH (so ... /opt/rh/devtoolset-2/root/usr/bin) which would set CXXFLAGS before calling g++-old (or just call g++-old with the appropriate extra flags).

So yeah, I could do it (and I will if this issue would be deemed to be 'out of scope' of cibuildwheel), but I'd like to use this project to simplify my building process life, not to make it harder :-D

@tgarc
Copy link
Contributor

tgarc commented Jul 6, 2017

This is actually only necessary in Linux. Since OS X and Windows don't have docker images, you can just do all these things before calling cibuildwheel. So ... should we make this something specific for Linux, or something extra for all platforms for the sake of consistency?

I think what has been discussed so far has been the lack of ability to customize the linux docker environment so I would tend to focus on that. But even if focusing on just linux there is the case of 32-bit and 64-bit architectures. Do we provide a separate ENVIRONMENT variable for each of these? Maybe. But if we start going the way of adding specific options for every possible platform things get kinda ugly quick. (And somebody has to maintain that mess..)

Since that's being run in sh -c, you cannot export any environment variables there, because they will not propagate to the parent scope.

And if the sh -c was removed, would that allow you to do what you wanted?

@YannickJadoul
Copy link
Member Author

YannickJadoul commented Jul 6, 2017

@tgarc Agreed! We should think this through and not just start adding bits and pieces everywhere. (That's why I started discussing instead of implementing straight away and why I tried to involve you.)

And if the sh -c was removed, would that allow you to do what you wanted?

Probably, yeah. I would not be pretty (e.g. writing code to not extend export PATH="/dir:$PATH" each time, but only the first time), but it would work, yes.
But I'm guessing that there's a reason for that sh -c? (We could pushd and popd, but there's maybe more reasons?)

@tgarc
Copy link
Contributor

tgarc commented Jul 9, 2017

But I'm guessing that there's a reason for that sh -c? (We could pushd and popd, but there's maybe more reasons?)

I don't know. @joerick ?

It seems like if we did remove the 'sh -c' prefix, we wouldn't want to add any pushd/popd or anything that would attempt to 'restore' the build environment because those environment changes may be exactly what the user wanted as exemplified by #13. The user should be responsible for restoring the working directory and things like that if they want to. In fact, unless there's a good reason sh -c was used in the first place it seems like removing it would make CIBW_BEFORE_BUILD more flexible.

@joerick
Copy link
Contributor

joerick commented Jul 10, 2017

I think we need to keep the sh -c for compatibility with the other OSs. There Linux build is the only one that uses bash for the build script, and this is an implementation detail that might change in the future. Sorry guys!

@YannickJadoul
Copy link
Member Author

@joerick I've tried, and kept that sh -c in #17 , but just 'extended' it such that the CIBW_BEFORE_BUILD and pip wheel happen in the same environment. That seems like the best of both worlds?

@tgarc
Copy link
Contributor

tgarc commented Jul 12, 2017

@YannickJadoul
I had a thought...first, let me see if I understand your use case. Am I right in saying that you just want to be able to edit the environment where the pip wheel command is run? If that's the case, then probably the right thing to do is to add some logic into your setup.py that checks the current platform and sets the appropriate environment variables before issuing a build. I'll see if I can dig up an example...

OTOH, if you only need to modify the docker environment for some pre-build commands you should be able to do that by using BEFORE_BUILD.

@YannickJadoul
Copy link
Member Author

@tgarc Yeps, that's correct. I would want/need to:

  • either download an archive with a new compiler or transfer it from the docker host (already possible if I'd download it in /project)
  • extract that archive (preferably once, shared for all different builds on that docker image)
  • add this compiler to the PATH
  • something similar for a new enough CMake version
  • set the CXXFLAGS
  • then have pip wheel called

I could add all of this to setup.py of course, but I'm not so happy to change my build files just for a continuous integration service. I mean, just with standard Unix practices, you áre already able to change the behaviour of setup.py (since CMake picks up these environment variables).
So because it's more a local configuration of the system, I'm not convinced I want to change my setup.py/CMakeLists.txt for this.. (But maybe I'm being overly difficult/idealistic; feel free to tell me that 😄 )

@tgarc
Copy link
Contributor

tgarc commented Jul 13, 2017

Yeah I wasn't suggesting you do all that in the setup.py. But the two steps of adding the compiler to the PATH and setting CXXFLAGS would seem appropriate to add into your setup.py. Would that make sense in your case?

@YannickJadoul
Copy link
Member Author

Hmm, but I don't feel like tweaking my setup.py for platform specific things?
It feels as is PATH and CXXFLAGS are some kind of 'input parameters' to the build process, that are out of the abstraction/scope for setup.py. To me, it makes sense that you need to update .travis.yml if Travis' build environment changes, but not that you need to change setup.py, since that one's meant to be generic.

I mean, of course it's possible, and if needed, I can do that. But it feels the wrong way around, design-wise. (Though, of course, it would fix things.)

@joerick
Copy link
Contributor

joerick commented Jul 13, 2017

@YannickJadoul I think the planned CIBW_ENVIRONMENT option would fix things for you though?

@YannickJadoul
Copy link
Member Author

YannickJadoul commented Jul 13, 2017

Probably, yes. Can't think of a reason yet why it would not.
It would still be kind of weird to first export the PATH as /not/yet/existent:... before actually creating it in the CIW_BEFORE_BUILD, but that's still reasonable, I guess.
The only difficulty I see is that you need to be able to extend the PATH (analogous to export PATH="/some/dir:$PATH"), instead of just setting it (because you don't want to hardcode the rest of the PATH probably).

@joerick
Copy link
Contributor

joerick commented Jul 13, 2017

😎 great! That's the direction I'd like to go. I won't have time in the next two weeks to implement but PRs are welcome if someone wants to have a go at implementing :)

@joerick
Copy link
Contributor

joerick commented Jul 13, 2017

Ps @YannickJadoul in the meantime, perhaps something like this in your setup.py would work

if os.path.exists('buildenv.json'):
  with open('buildenv.json') as f:
    os.environ.update(json.load(f))

(on phone so haven't tested this ⬆️)

You could create that file in your Travis yml to keep the settings isolated 🙂

@YannickJadoul
Copy link
Member Author

YannickJadoul commented Jul 13, 2017

@joerick Hmmm, yeah, that would be a reasonably clean solution, actually.
Right now, I had actually realized it's not that much code (since I don't want tests, or so, those are done immediately by CMake, and not via wheels), and started making an ad-hoc implementation of that in my .travis.yml. (Since ... I want to program again; I'm getting rather fed up with that whole build environment tweaking :-D )

I'm not sure how much time I'll have over the next weeks either, but I'll see. If I do, I can see how easy CIBW_ENVIRONMENT would be.

@tgarc
Copy link
Contributor

tgarc commented Jul 13, 2017 via email

@YannickJadoul
Copy link
Member Author

@tgarc Yeah, that's true, actually. I was planning that at some point, but then I'd forgotten about the possibility.
Turns out nót using cibuildwheel quickly turns rather messy :-P

@tgarc
Copy link
Contributor

tgarc commented Jul 21, 2017

I still don't know what an implementation of CIBW_ENVIRONMENT would look like. Would it set the environment for all build steps? Only the pip wheel step?

@joerick
Copy link
Contributor

joerick commented Jul 21, 2017

All the build steps, I'd imagine. I think that covers the simplest use cases and follows the principle of least surprise.

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

No branches or pull requests

3 participants