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.

[henryiii]

Edited above; I'd like to see if iOS can mimic the current behavior. Would the placeholders still refer to the original paths? I like the idea that it simply changes what is added to the new cwd, and doesn't touch the placeholders at all. (Which is what you described but I was thinking of a different model when I read it, so missed that).

[joerick]

Yeah, I think we'd just leave {project} and {package} as they are on most platforms - they still refer to the source-tree. But on iOS we can't supply those paths because they don't exist in the simulator. So you'd have to use the cwd in iOS.

That might actually be an opportunity for a nice error message on iOS - if a test-command contains {project}, raise an error like "On iOS, the project source files are not accessible from the simulator. Instead you must use test-sources, which will copy your test files into the working directory"

[joerick]

Yeah, I think we'd just leave {project} and {package} as they are on most platforms - they still refer to the source-tree. But on iOS we can't supply those paths because they don't exist in the simulator. So you'd have to use the cwd in iOS.

That might actually be an opportunity for a nice error message on iOS - if a test-command contains {project}, raise an error like "On iOS, the project source files are not accessible from the simulator. Instead you must use test-sources, which will copy your test files into the working directory"

Ya know, this is pretty nice actually. We currently have an error on iOS when you don't define test-sources, to tell people that it's required to run tests. But it isn't really required. For example, numpy, astropy, bitarray don't want to copy anything, they just wanna call something inside the wheel.

I think we can remove the error on unset test-sources, and just use the test-fail.py pattern, just with a slightly different message. Because actually the use of {project} implies a misunderstanding regarding iOS more strongly than a blank test-sources.

[freakboy3742]

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.

As noted by @joerick, the complication on iOS will be that the build machine (i.e., the machine starting the test run) won't have any visibility of what the {project} expansion would be on the target (i.e., the simulator). That would mean that the on-simulator test runner would need to understand how to interpret {project}, which means leaking cibuildwheel implementation details into the CPython testbed runner.

Having {project} expand to the build machine location won't work either, because the simulator won't have visibility on that location.

The best case I can think of would be replacing {project} with ""... which might work in some circumstances? (maybe?) - but you'd need to have test-sources defined anyway.

I think we can remove the error on unset test-sources, and just use the test-fail.py pattern, just with a slightly different message. Because actually the use of {project} implies a misunderstanding regarding iOS more strongly than a blank test-sources.

To make sure I'm following correctly - you're suggesting:

• Dropping the error on an empty test-sources for iOS
• If test-sources is undefined on iOS, we copy in a test_fail.py analog as the "test sources"
• Raise an error on iOS if test-command contains {project} or {package} (are there any others we need to catch?)

That way, on an iOS project with no test-sources definition:

1. test-command = "python -m numpy.conftest" works because numpy will be on the PYTHONPATH
2. test-command= "pytest" will fail, hitting the iOS test_fail.py that provides iOS-specific advice about using test-sources etc
3. test-command= "python {project}/test_script.py" will fail with an iOS-specific error that {project} isn't allowed.
4. test-command= "pytest dirname" will fail with a pytest error about dirname not existing (because it hasn't been copied to the test environment)
5. test-command = "python dirname/test_script.py" or test-command = "python test_script.py" will fail with an error about being unable to find the named script.

1-3 all look like good outcomes; 4 and 5 could be potentially confusing - but I'm not sure there's any good way around those other than documentation, and you'd get those anyway if you had an incorrect test-sources definition.

Have I understood correctly?

[joerick]

Yeah, you're right generally speaking, that's what I'm thinking. With the slight correction that (5) would have to be python -m ... otherwise the user would get the error about module-level stuff.

Additionally, (4) and (5) are already invalid configurations on other platforms too - that would be no different on iOS.

[joerick]

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.

Any feeling on this @henryiii ? I'm tempted to remove it from a YAGNI perspective.

[henryiii]

Which one? PYTHONSAFEPATH, yes, can be removed now. I'm +0 on test-environment too.