Test CWD issues discovered in beta

[joerick]

It seems that in the beta we're seeing a couple instances of the test cwd change causing significant backwards incompatibility-

• CI: update cibuildwheel to 3.0.0b1 and enable cp314 and cp314t nightlies numpy/numpy#29007
• EXP/WHL: test cibuildwheel 3.0.0b5 astropy/astropy#18159

Even with PYTHONSAFEPATH applied, it seems that there's some behaviour (could be pytest's prepend mode?) that is causing issues with import paths in test commands. I haven't looked into it in any depth, but I wanted to make an issue as somewhere to discuss.

For what it's worth, I suggested making the change to the test CWD to clean up options specification, as I thought the temp dir wasn't providing us much benefit anyway. It looks like that might not have been 100% right!

I think we still have the option of keeping the temp CWD behaviour when test-sources is unset. One thing to consider is if it's worth just returning to that behaviour. Of course the best thing would be for projects to switch to /src for their code, but we know that a cibuildwheel upgrade is unlikely to be enough of a reason to make that (huge) change.

[henryiii]

My thought was to check for usage of {project}. The object we pass in can detect if it's used. But it might be simpler to just base it on test-sources. You can't tell empty from non-defined (at least if you want to be nice to users), but test-sources=["*"] is pretty close to the current default. It would be nice to offer "normal" builds without copies for src structured projects though, so I think {project} detection might still be a good idea instead.

There are a couple of things still to understand or fix: #2405 and I want to understand what numpy is doing to try to import numpy/conftest.py. It's not in the installed wheel, so this is a really weird mishmash of local and installed files, I think. I'll try to look at it tonight or tomorrow.

[henryiii]

Here's what I was thinking:

            if build_options.test_sources:
                test_cwd = testing_temp_dir / "test_cwd"
                container.call(["mkdir", "-p", test_cwd])
                copy_test_sources(
                    build_options.test_sources,
                    build_options.package_dir,
                    test_cwd,
                    copy_into=container.copy_into,
                )
            elif test_command_prepared != prepare_command(
                build_options.test_command,
                wheel=wheel_to_test,
            ):
                # Back-compat mode for cibuildwheel < 3
                # Run the tests from a different directory
                log.warning(
                    "Using {project} and {package} in test_command is deprecated; triggering back-compat mode for cibuildwheel < 3."
                )
                test_cwd = testing_temp_dir / "test_cwd"
                container.call(["mkdir", "-p", test_cwd])
            else:
                # There are no test sources. Run the tests in the project directory.
                test_cwd = PurePosixPath(container_project_path)

            container.call(["sh", "-c", test_command_prepared], cwd=test_cwd, env=virtualenv_env)

That would cover:

• New projects get the current directory, providing a nice modern default
• Previous projects keep the old structure, since they always had to use the substitution
• Projects ideally move to setting test-sources

I'm also fine if we just want to not allow the new project cwd case, and instead have:

• Empty test-sources: old mechanism
• Set test-sources: copy mechanism

One possible option would be to just fix the test-sources problem, and then do another beta release. I'm not sure how common the problem is, as the issue here stems from numpy.conftest, which is in the local directory, but not the wheel. It might be that only packages that are copying numpy have a problem. (which is probably a non-trivial number)

[joerick]

Thanks for this.

I want to understand the various failure modes a little better e.g. is the reason that PYTHONSAFEPATH isn't helping just that it only has an effect on 3.11+?

My feeling is that the old behaviour doesn't hurt anyone, and was actually helping more than we suspected. I think the backward-compatibility method might work, but-

• we'd be introducing a footgun for new/upgrading users without /src - their tests might be importing from the wrong place
• is the benefit of running tests from the project-dir worth the increase in complexity? for example, documenting how the CWD is set for test-command becomes quite complicated. It's actually not too bad if we stick with the temp cwd, because then it's always a temp cwd, regardless of the setting of test-sources.

It also sorta depends on how strongly we want to push test-sources as the 'proper' way to do things... I should get some time tomorrow to do some experiments and have a proper look.

[neutrinoceros]

is the reason that PYTHONSAFEPATH isn't helping just that it only has an effect on 3.11+?

NumPy and Astropy both already required Python >= 3.11 at the time of testing, so I don't think that explains it.

[henryiii]

The problem is that numpy has numpy/conftest.py, and something weird happens with that - it's not in the installed wheel, but it's somehow loaded as numpy.conftest, which is what breaks. I'm not sure how this was supposed to work, but that's why this is breaking even with PYTHONSAFEPATH. Running pytest directly should also work as well (only python -m pytest is broken by not having PYTHONSAFEPATH in <3.11, entry points do not have this injection), so it's this specific setup that's breaking (I'm guessing astropy copies this). Using the importlib mode might fixes the issue, but runs into the fact numpy's tests suite doesn't support this mode.

Given that you can run the test suite from anywhere, maybe conftest.py should be in the wheel?

It would be nice to know what the fix is, and how common this setup is.

[henryiii]

we'd be introducing a footgun

New users aren't that big of a problem - users always have to work around this to run their tests locally. Maybe back when "inplace" building was a thing, but now everyone uses editable or wheel builds, and they have to work around it. That's why it was so easy to fix for numpy; the CI tests already used the chdir trick. Users have to do this locally to test numpy.

The current method is more of a footgun, IMO. It's unexpected to have to use {project}, and the setup still confuses people sometimes. Less difference from local running makes it easier to setup and understand, with special setup being opt-in instead of... Well, you can't even opt out today.

But I do also want to minimize upgrading issues, so unless we can tell users exactly what can go wrong and how to fix it, we probably should have something for backcompat. We do want people using test-sources so they can more easily add mobile wheels, I think, so maybe forfeiting in-place builds when it's not set is the way to go.

[neutrinoceros]

(I'm guessing astropy copies this)

correct, we also have an astropy.conftest module, though ours is also distributed into wheels, if it makes any difference.
I also want to note that astropy's layout is pretty much used as an example in its own ecosystem of coordinated and affiliated packages, so we likely have multiple packages that would break under the current behavior (thankfully, only a fraction of our collection of packages have any extension code, so most of them don't need cibuildwheel).
For those who do need it, the upgrade might be especially confusing because we recommend using https://github.com/OpenAstronomy/github-actions-workflows, which wraps cibuildwheel, so it's possible that some affected maintainers have a hard time figuring it all out. Of course we can stress this wrapper workflow's next release as breaking too, but I feel that if backward compat could be preserved by default in that area, it might save time for more people than we expect.

[joerick]

I've done some research-

A project without src/, checking which module is imported - the wheel version, or the project-dir version.

test setup	v2.x	v3.0.0b2
1. pytest, invoked with pytest {project}/test	✅	✅
2. pytest, invoked with python -m pytest {project}/test	✅	❌ imports from project-dir on Python earlier than 3.11
3. a test script e.g. python {project}/script.py	✅	❌ imports from project-dir on Python earlier than 3.11
4. an embedded test module e.g. python -m mymodule.tests	✅	❌ imports from project-dir on Python earlier than 3.11
5. pytest, invoked with pytest --pyargs mymodule	✅	❌ pytest imports from project-dir rather than the wheel

[joerick]

It feels to me that it's just not worth the change to shift the CWD to the project-dir. The impacts will likely be more than just the --pyargs style of setups.

This also rules out a backward-compat behaviour keyed on {project}, because (4) and (5) above don't include them.

My proposal, then, is to revert back to the v2.0 behaviour when test-sources is unset - cibuildwheel will create a temp dir containing the test_fail.py guard file, and run from there, and require users to specify {project} to refer to paths in their project directly.

[joerick]

I'm not sure how common the problem is, as the issue here stems from numpy.conftest, which is in the local directory, but not the wheel.

Minor correction - it's in both places, pytest seems to get upset with importing conftest file because it finds it twice. But that error is incidental (fortunate, even!) to the issue that the wrong package was found to when --pyargs was used.

[henryiii]

BTW, if 1 works everywhere, and 2-4 work on Python 3.11+, it was nearly working as expected.

Okay, let's go with 2.x style out of source runs.

So then the question becomes, do we want to mimic this in test-sources too? We could copy to a neighboring folder instead and still keep this setup. We could even add a flag to enable/disable this setup for both. test-in-source could default to False (2.x behavior), but setting it to True would get the current behavior. And you could apply it to test-sources too. That would allow simpler test runs for packages that have good local testing, and no structure change from just adding test-sources.

Also, if we revert this, what about the PYTHONSAFEPATH and test-environment? Do we keep those? If we had the above option, I think yes, but without it, not so sure.

[joerick]

I don't think we need a test-in-source option. If a user really wants to run from the project dir, they can cd {project}. But it doesn't seem to be worth an extra option to me.

So then the question becomes, do we want to mimic this in test-sources too? We could copy to a neighboring folder instead and still keep this setup. We could even add a flag to enable/disable this setup for both. test-in-source could default to False (2.x behavior), but setting it to True would get the current behavior. And you could apply it to test-sources too.

Hmm. I don't follow. I think we can be a little simpler than what you describe! We don't need 3 different dirs, only two. We have the test_cwd, that's either-

a) if test-sources is set, it contains the files specified by test-sources. In this mode, you can do relative path test-command = "pytest ./tests"
b) if it's unset, it contains test_fail.py, (if that platform allows it). In this mode, you do test-command = "pytest {project}/tests"

I'll put together a PR to illustrate what I'm thinking.

Also, if we revert this, what about the PYTHONSAFEPATH and test-environment? Do we keep those? If we had the above option, I think yes, but without it, not so sure.

I'd say we don't need to set PYTHONSAFEPATH - no need to set a global option without a strong reason. Especially since it will propagate to subprocesses of tests.

I'm +0 on keeping test-environment. We have had use-cases for it before e.g. SYSTEM_VERSION_COMPAT. Or some users might prefer to set PYTHONSAFEPATH using it, I suppose.

[henryiii]

The problem is iOS. There, this will run in the test source and using placeholders is not allowed. The old mechanism requires a placeholder in the test command! test-in-source would allow a well packaged library to run with the same test-command everywhere. It’s also nice to not require users learn about placeholders in test commands if they’ve already properly designed the package to run locally (NumPy has to do the cd trick to run locally, too!).

We can tie this behavior change to test-sources instead. I’d like to know if iOS can support placeholders as long as the path is inside the right directory. If so, having both seems clearer for the user. If not, then just combining them seems fine.

[joerick]

It doesn't require a placeholder in the test command if test-sources is set, because test-sources brings the test files into the cwd.