Skip to content

Conversation

henryiii
Copy link
Contributor

@henryiii henryiii commented May 30, 2025

Critical fix before 3.0: test-sources should be relative to the project root, not the package directory. The package directory is always a subdirectory of the project directory, so this is more flexible.

By the way, on iOS, it might make sense to expand {project} to the relative path? While you normally do know it as well, I'm not sure what all the details are if you run directly on an SDist. (So test-sources work, {project}/cwd() should probably be the root of the SDist, would be good to verify)

No documentation change since this is how it's described in the docs already. :)

Fix #2436.

@joerick
Copy link
Contributor

joerick commented May 30, 2025

Totally agreed on the design front. Good catch.

@joerick
Copy link
Contributor

joerick commented May 30, 2025

By the way, on iOS, it might make sense to expand {project} to the relative path? While you normally do know it as well, I'm not sure what all the details are if you run directly on an SDist. (So test-sources work, {project}/cwd() should probably be the root of the SDist, would be good to verify)

Outside of test-command, I think we should can support {project} and {package} on iOS.

But I'm okay with leaving placeholder support out of test-sources because the current implementation relies on it being a relative path. (Technically we could support relative paths for test-sources as you say, but I think it does confuse things as the same paths don't work in test-command, so I think it would be a bad idea to encourage this, and I'm not sure why we'd bother implementing it. )

In the sdist case, all of package, project and cwd are the same path, which is the root of the sdist.

@henryiii
Copy link
Contributor Author

henryiii commented May 30, 2025

Looks like there is an issue copying files on iOS in pybind/pybind11#5705. That or "*" doesn't work on iOS, which is possible.

@henryiii
Copy link
Contributor Author

henryiii commented May 30, 2025

Nevermind, iOS doesn't support * expansion.

@henryiii
Copy link
Contributor Author

relative paths for test-sources as you say, but I think it does confuse things as the same paths don't work in test-command

It would be relative sources for test-command, just like everywhere else, it just would require test-sources be set, as otherwise the files you are referring to wouldn't be there.

I agree, though, I think it's better of users adopting test-sources to use manual relative paths and not the placeholders. (It would be handy if they wanted to set ios.test-sources only and use one test-command for both, but don't see much reason for that. You can duplicate test-command in that case to be explicit.)

@henryiii
Copy link
Contributor Author

henryiii commented May 31, 2025

pybind/pybind11#5705 is passing (using this PR)!

@henryiii henryiii requested a review from Copilot May 31, 2025 04:24
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes the handling of test-sources by having them be relative to the project root instead of the package directory. Key changes include renaming the parameter in copy_test_sources from package_dir to project_dir and updating all platform-specific calls to use Path.cwd().

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file
File Description
cibuildwheel/util/file.py Renamed parameter in copy_test_sources and updated its docstring.
cibuildwheel/platforms/windows.py Replaced build_options.package_dir with Path.cwd() for resolution.
cibuildwheel/platforms/pyodide.py Replaced build_options.package_dir with Path.cwd() for resolution.
cibuildwheel/platforms/macos.py Replaced build_options.package_dir with Path.cwd() for resolution.
cibuildwheel/platforms/linux.py Replaced build_options.package_dir with Path.cwd() for resolution.
cibuildwheel/platforms/ios.py Replaced build_options.package_dir with Path.cwd() for resolution.
Comments suppressed due to low confidence (2)

cibuildwheel/util/file.py:113

  • The function parameter has been renamed to 'project_dir', but the docstring still refers to 'package_dir'. Please update the docstring to use 'project_dir' for consistency.
def copy_test_sources(test_sources: list[str], project_dir: Path, test_dir: PurePath, copy_into: Callable[[Path, PurePath], None] = copy_into_local) -> None:

cibuildwheel/util/file.py:123

  • The parameter for the copy function is named 'copy_into' in the function signature but is described as 'copy_info' in the docstring. Please update the docstring to match the parameter name 'copy_into'.
    :param copy_info: The copy function to use. By default, does a local filesystem copy; but an OCIContainer.copy_info method (or equivalent) can be provided.

Copy link
Contributor

@joerick joerick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot's low-confidence comments are worth looking at, but otherwise this looks good to me.

@henryiii
Copy link
Contributor Author

OK, I'll fix those soon. Generally have found the low confidence comments to be as good or better than the high confidence ones. Both of them seem to find something useful about a third of the time.

@henryiii
Copy link
Contributor Author

(Though I did forget to look at this one, they don't show up in the email.)

Signed-off-by: Henry Schreiner <[email protected]>
@henryiii henryiii merged commit 2125d71 into pypa:main Jun 1, 2025
27 checks passed
@henryiii henryiii deleted the henryiii/fix/test_sources branch June 1, 2025 03:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test-sources issue

2 participants