Run each test using a temporary directory

[mayeut]

Simpler way of getting each test to run in a unique temporary directory than #213 and #214 which also attempts to make all tests being run with a single pytest invocation.

[YannickJadoul]

Hmmm, interesting. Are we sure about this?

[mayeut]

I was hesitant about this and thought I'd get feedback.
While all tests are passing as of now, this check is not applicable in the general case.
Wheels are put in the output directory config per config on Windows/macOS or all at once on Linux.
The behavior is thus different depending on the platform. On Linux, you either get all or none of the wheels depending of a single error. On macOS/Windows, you get all wheels with tests successful (if applicable) up until the error.
Either we consider the linux behavior to be the right one and this check can be left as-is (requiring to add further tests later to ensure this behavior) or I can make this check optional for the one test that does use it (and we keep a different behavior depending on the platform).

[YannickJadoul]

The behavior is thus different depending on the platform.

Good point. I hadn't even considered that. But that's a good point.
I was more thinking in terms that it should be the test asserting (because asserting no wheels are built is part of the test-), rather than here in utils.

In the end, the difference between OSs doesn't matter that much, I'd say? The error is thrown anyway?

[YannickJadoul]

There are two more solutions, though:

1. Catch all errors, and just return the list? Normally, the test should catch that the wrong wheels are built? If necessary, we can return a few things, like error status, output, and built wheels; or return built wheels and None/caught exception?

2. Throw a custom exception on catching this one, that "includes the list of built wheels", as a way of having an exception ánd return value?

Neither of these two solutions give me a very warm feeling, though.

[mayeut]

There's yet another solution which provides flexibility (but consistency-wise would be a bit lower).
Add an output_dir=None default arg to cibuildwheel_run, much like @Czaki proposed at one time in order to use tmp_path only on selected project before saying all tests shall use a temp folder.

My feeling is that all tests needs to run in a temp folder.
90% of them won't need to bother with an output_dir and just need the resulting wheel names and what's here just works. By adding output_dir=None, we allow selected tests that require access to output_dir to provide it.

Any thoughts ?

[YannickJadoul]

No. If you build 64 an 32 bits, then it copy wheels in two packs.

Huh? We don't mount a directory into the docker image anymore, do we? Because CircleCI doesn't allow that?

EDIT: See https://github.com/joerick/cibuildwheel/blob/34515b78c6a097df9c709d484cb7cecefda33d28/cibuildwheel/linux.py#L174; I think @mayeut was correct?

[YannickJadoul]

@mayeut

There's yet another solution which provides flexibility (but consistency-wise would be a bit lower).
[...]
Any thoughts ?

Hmmm, myes, interesting; I hadn't thought of that, indeed. I guess that's as good of a solution as the other two things I suggested (or even better), since it's less of a hack, and doesn't try to "return a value from a function that's throwing an exception". (Or maybe this is the sign that the abstraction of having run_cibuildwheel return a list of wheels is not completely right?)

[Czaki]

But you run separated docker containers for x86_64 and i686 wheels.
and when container finish work then result is copied. So you will have two transactions.

[YannickJadoul]

@Czaki Ooooh, yes, right. I thought you meant you get pairs of 32- and 64-bit wheels. But you get two separate batches of all 32- and all 64-bit wheels.

[mayeut]

No. If you build 64 an 32 bits, then it copy wheels in two packs.

You're right. Doesn't change the fact that behavior is different though. Anyway, this is not the matter here.

Or maybe this is the sign that the abstraction of having run_cibuildwheel return a list of wheels is not completely right?

I think the option doesn't hurt too much the abstraction.

[YannickJadoul]

Other annoying detail: in #179 (about which @joerick is not fully conviced yet, either, as far as I understood), @Czaki needs subprocess.run rather than subprocess.check_call. Then again, he already wasn't using utils.cibuildwheel_run, so maybe that's not relevant.

[Czaki]

But now this test (from #179) has symilar structure to other. After this changes, even after refactor to new style it will have "different structure"

[Czaki]

@YannickJadoul @mayeut
I would like to argue why I prefer #213 or #218 rather the #214 or this.
My points of view is point of view of person who short time ago join to developing package.

In my opinion test should be self explaining. So person who do not have perfect knowledge about project should understand what scenario is executed. For me this PR and #214 do not satisfy this.

Reading test under this PR I rather expect that utils.cibuildwheel_run clean wheelhouse rather than use TemporaryDirectory. And I do not expect that it will return list of wheelhouse.

I think that all my remarks can be removed by rename functions. Ex. cibuildwheel_run -> cibuildwheel_run_and_list_wheels, but this can produce very long names. Maybe this approach needs rethink structure of tests.

[mayeut]

My points of view is point of view of person who short time ago join to developing package.
In my opinion test should be self explaining. So person who do not have perfect knowledge about project should understand what scenario is executed. For me this PR and #214 do not satisfy this.

Thanks for that feedback, that's valuable. I do understand this point of view and I guess the current size of this part of the project does allow for what you're asking. @joerick ?

From a generic maintenance point of view, duplicate code is most often a nightmare. Things can grow out of proportions quite fast and, I really dislike having to change something at many places when it could have been factored. Yes, it does requires a better understanding of the code base but it's a trade-off between maintainability and ease of discovery. It does require to spend time thinking this refactor in order not to create something too convoluted either. (and if you think this PR is, maybe I'm not leaning towards a solution that's good enough)

May I ask if the current state of tests in master could have benefited from a bit more docs (in the specific README probably) ?
Explaining the structure of a test perhaps ? or linking to a reference test that's more commented than other (maybe test 02 which has 3 tests)

[YannickJadoul]

Reading test under this PR I rather expect that utils.cibuildwheel_run clean wheelhouse rather than use TemporaryDirectory. And I do not expect that it will return list of wheelhouse.

Hmmm, I don't fully understand why, because I feel the opposite is more intuitive: If you are not very familiar with the code or cibuildwheel in general, this hides away the details. actual_wheels = utils.cibuildwheel_run(...) and then assert set(actual_wheels) == set(expected_wheels) feels very clear to me, hiding away all the details you at first don't care about? (This kind of code that's almost readable as text, is why we love Python, no? :) )

My resistance against making tmp_dir explicit is 1) it increases maintenance and the possibility for inconsistencies (Don't repeat yourself!). But 2) it also adds complexity by specifying a directory we don't care about. By that I mean: we care about it to give correct results, but the actual test does not depend on the value of the directory and does not e.g. test that if you pass -w wheelhouse cibuildwheel will not place them in a different folder wheelhouse_dir or so. So, if you want to look at the implementation details, you'll have to look at utils.py anyway, but if you take a quick look at what a certain test is testing, why bother the reader with tmp_dir?

In my opinion test should be self explaining. So person who do not have perfect knowledge about project should understand what scenario is executed.

So I completely agree on this; I just seem to have a different solution for it :-)