New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix overwriting wheel on macos #1129
Conversation
When I run on macOS, I get the following error: ``` Traceback (most recent call last): File "/Users/david/.local/pipx/.cache/fd54e06d50697e7/bin/cibuildwheel", line 8, in <module> sys.exit(main()) File "/Users/david/.local/pipx/.cache/fd54e06d50697e7/lib/python3.10/site-packages/cibuildwheel/__main__.py", line 128, in main build_in_directory(args) File "/Users/david/.local/pipx/.cache/fd54e06d50697e7/lib/python3.10/site-packages/cibuildwheel/__main__.py", line 252, in build_in_directory cibuildwheel.macos.build(options, tmp_path) File "/Users/david/.local/pipx/.cache/fd54e06d50697e7/lib/python3.10/site-packages/cibuildwheel/macos.py", line 538, in build shutil.move(str(repaired_wheel), build_options.output_dir) File "/opt/homebrew/Cellar/python@3.10/3.10.4/Frameworks/Python.framework/Versions/3.10/lib/python3.10/shutil.py", line 811, in move raise Error("Destination path '%s' already exists" % real_dst) shutil.Error: Destination path '/Users/david/Documents/GitHub/python-mpy-cross/wheelhouse/mpy_cross_v5-1.0.0-py3-none-macosx_11_0_arm64.whl' already exists ``` The comment in the code says that it should overwrite the file, so this removes the file if it exists first before trying to move the file.
I think the correct fix would be to treat a (ninja and cmake are other examples of platform but not Python specific wheels). Currently we just limit to one Python version, but I guess this could be used to check multiple Python versions (I'd probably still limit this to just the oldest and newest Python version, or maybe just one - the built part doesn't touch Python so I'm not that worried about it breaking). |
The problem is not on CI but locally when I am running Based on the existing code comment, I think it was assumed that |
|
Yes, that is the workaround I am using. |
Ahh, in shutils.move docs: If the destination already exists but is not a directory, it may be overwritten depending on os.rename() semantics. As long as this still errors on making a pure wheel, I think it’s fine and more consistent. |
Exactly. This change takes away the OS semantics and makes it work the same on all OSes. |
I fully agree with this assessment:
Yeah, I've been seeing this sometimes as well when I forget to remove files but given I really want this to fail in CI, I considered it a minor annoyance. That being said, I think we can implement a win/win given we know if we're on CI or not (well almost, some users might be forcing platform on unsupported CIs). I think it shall be implemented, probably in a helper to be reused on all platforms, somewhere along:
Any thoughts on that proposal ? |
I think it should fail early (before the build) rather than after the build. There is no sense in wasting time doing the build only for it to fail on the very last step. |
We can't know the name of the wheel until the repair step is done. So failing early would mean after the repair step and before the tests. |
This change looks good to me.
This is a manual check currently, so this PR wouldn't affect that. cibuildwheel/cibuildwheel/linux.py Lines 244 to 245 in 46702f5
cibuildwheel/cibuildwheel/macos.py Lines 386 to 387 in 46702f5
cibuildwheel/cibuildwheel/windows.py Lines 354 to 356 in 46702f5
Hmm. "destination already exists" hasn't been an error up to now that we deliberately emit. It's only raised on macos. Some users do appear to be running locally more these days, where overwrite is certainly the expected behaviour. e.g. #1089 where the second run was simply overwriting the previously built wheel I'm slightly -1 on changing existing behaviour to make something an error that previously wasn't one. Your proposal is interesting @mayeut, but I think that switching on the CI environment is gonna cause confusion/frustration - e.g. a user gets the build working locally, but then behaviour changes in CI and it fails. So not 100% convinced on that either. But multiple identical wheels in the same run could be considered an error, if that's useful @mayeut ? e.g. rather than looking at |
@joerick, that seems like a good plan. It would error out on building a
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to merge this as-is. I've created #1142 to cover the conversation above.
cibuildwheel/macos.py
Outdated
@@ -535,6 +535,11 @@ def build(options: Options, tmp_path: Path) -> None: | |||
|
|||
# we're all done here; move it to output (overwrite existing) | |||
if abi3_wheel is None: | |||
try: | |||
os.remove(build_options.output_dir / repaired_wheel.name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason this isn't using Path.unlink
(perhaps with missing_ok=True
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This missing_ok
parameter was added in Python 3.8. According to the setup.cfg
in this repo python_requires = >=3.6
so it still needs to run on Python 3.6.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about just unlink and keeping the try, then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both functions do the same, so I don't really see any difference in one over the other.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are already using pathlib
, so why mix that with os
? Also this would transition more elegantly to missing_ok
once 3.6 & 3.7 are dropped. If actions/setup-python#411 is merged in some form, we might be able to drop 3.6 for running.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. I also changed OSError
to FileNotFoundError
to be more specific.
Thank you @dlech for the report and fix! |
This is the same fix as pypa#1129 but for Windows this time.
This is the same fix as pypa#1129 but for Windows this time.
fix overwriting wheel on windows This is the same fix as #1129 but for Windows this time.
When I run on macOS, I get the following error:
The comment in the code says that it should overwrite the file, so this removes the file if it exists first before trying to move the file.