Option for alternative docker images for manylinux1

[YannickJadoul]

Start of #45 implementation.

Things I'm not yet happy about:

• CIBW_MANYLINUX1_X86_64_IMAGE and CIBW_MANYLINUX1_I686_IMAGE are not added to the build_options dictionary. Reason: passing **build_options to windows and mac functions would then fail (and adding the two extra arguments there does not make sense). Maybe it's time to make these build options a custom class (since the number of parameters of the build function starts getting quite large, too)?
• For the test, I don't have an alternative image (yet). @jbarlow83 might have one? For now, I've just looked up an alternative manylinux1 docker image, and found/used the ones from dockcross.

[YannickJadoul]

So, Linux builds succeed, but Windows and OS X fail because no wheels get build (because I'm skipping win and macos in the added test and because the assert that at least 4 wheels have been built is false).

[jbarlow83]

@YannickJadoul I started work on a PR too, but you actually added a test case, so you win.

I don't have an image yet, although for quick testing you could use docker tag to make an alias for the manylinux1 and confirm it works with the alias.

For the build_options directory, you could change the function signature in each file:

def build(project_dir, package_name, output_dir, test_command, test_requires, before_build, skip, environment, **platform_options):

Then each platform can check platform_options for any special platform specific keywords, and ignore those not relevant to it.

[YannickJadoul]

Oops, sorry :-/ Thought it was a quick thing to implement (and a nice distraction from actual things that needed to get done).

But then I haven't put a lot of thought into different options, so if you have any remarks/different choices, please let me know :-)
Your idea for the platform_options is better than the current thing, so I'll change that in a bit. (If there was a way to collaborate on a PR, that'd be nice, but apparently, I can only have 'collaborators' push changes to this branch?)

[joerick]

Hi all, just checking in, I haven't looked at this in detail but I agree with the discussion so far and I'm happy to add @jbarlow83 as a collaborator for the purposes of collaborating on this if that's helpful?

edit- actually this branch is on your repo fork @YannickJadoul, so you can add @jbarlow83 as a collaborator to that and you can both push to the pr i think.

[YannickJadoul]

Yeah, that's up to @jbarlow83, if he prefers to immediately commit. I'm also happy to have stuff reviewed here in the PR (using GitHub facilities) and make the suggested changes, if you don't mind that I would make all the commits?

[YannickJadoul]

@joerick Any ideas on getting the tests working for cases where no wheels are built (and assert len(wheels) >= 4 fails)? Or should I just not skip the windows/OS X tests?

[joerick]

Any ideas on getting the tests working for cases where no wheels are built

Tricky one. I'm starting to think that the current structure (folders with environment.json for each test) as the mechanism is running a little thin. Probably the longer-term solution is to have a Python script in each test folder run_test.py that contains the environment variables, a subprocess call and some asserts.

That's a bit of a drag, so if you don't fancy tackling that in this PR for now I'd be happy for you to hack around it - if not '06_docker_images' in test_project: assert len(wheels) >= 4

[jbarlow83]

The change I proposed is fairly simple so feel free to add it yourself. At this point it would be more work to involve me.

[YannickJadoul]

Sorry it took so long; it's been ages, but I suddenly remembered I still had these changes to be implemented.

I took another look, and this is what I came up with:

• I collapsed manylinux1_x86_64_image and manylinux1_i686_image into a single dictionary argument (manylinux1_images). Somehow, splitting up the arguments on the platform-specific side into 'shared by all' and 'the rest' seemed quite arbitrary. So instead, I came up with this, hoping it'll do the job until the whole configuration is maybe refactored at some point?
• I removed the CIBW_SKIP in the sixth test's configuration. Running the example on OS X or Windows is maybe not really necessary, but I guess it won't hurt that much to build this simple example project?

CI tests seem to be passing, now. So please let me know if this is good enough, @joerick & @jbarlow83, or whether I missed something. I'll try to get changes implemented more quickly, next time ;-)

[joerick]

Sorry, I missed this a few weeks back! 😳 Taking a look now

[joerick]

This looks great! Is there any way to check in the test you have added that you're indeed in the docker image that was supplied? Perhaps checking for the existence of /dockcross directory.

[YannickJadoul]

Pfff, good question, but I don't really know. Why would a /dockcross directory be made?
The reason I chose this Docker image for the tests was because it seemed to be the same as the original manylinux1 one

[YannickJadoul]

Build seems to be failing because on OS X / Windows, it's obviously not running in the dockercross image. I see two way to fix it:

1. Use CIBW_SKIP to not run OS X / Windows, but then you run into the assert where less than 4 wheels are built on some platforms. (We discussed that before, but I just removed the CIBW_SKIP, since we could also just test if builds on OS X / Windows still run normally and are not affected by the CIBW_MANYLINUX1_..._IMAGE variables.)
2. Run this check @joerick added only on when building linux wheels

[joerick]

Hey Yannick, apologies for the drive-by commit! I'm busy with family stuff over the holidays. I had forgotten about the Mac/Win discussion

since we could also just test if builds on OS X / Windows still run normally and are not affected by the CIBW_MANYLINUX1_..._IMAGE variables

This is a great point, not something I'd considered. So adding a check for linux in setup.py would probably be the best option. I've not time to make the change in the next week, but if you get a moment to add it, that would be great! Otherwise I'll get to it in the new year. Cheers! Joe

[YannickJadoul]

Hey, don't worry! I also just saw this passing by and had some time to figure out/remember what was going wrong. (But then I didn't know if you'd push a commit, so ...)

Let's see if I can get green lights before going into the new year. Always a better start of that year if at least the CI is happy ;-)
Happy last part of the holidays and new year, meanwhile!

[joerick]

Aaaand it's merged. Excellent work @YannickJadoul!