fix overwriting wheel on macos

[dlech]

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.

[henryiii]

I think the correct fix would be to treat a py3-none-macosx_11_0_arm64 wheel just like an ABI3 wheel, and test but not build / overwrite on newer versions. Building then overwriting wastes CI time, and doesn't really gain you much; testing the original wheel on newer Python versions gains more with less CI time. For a py3-none-any, it should continue to be an error (you don't need cibuildwheel for those). It should not overwrite in any case.

(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).

[dlech]

The problem is not on CI but locally when I am running cibuildwheel multiple times. For example, during development, I was building for multiple arches on macOS and the first one succeeded but the second one failed. So I tried to fix the problem and run again, but cibuildwheel crashes with the error above since there is already one wheel in the wheelhouse directory`.

Based on the existing code comment, I think it was assumed that shutil.move() would overwrite an existing file without error, but I think in reality, the behavior of error or no error for existing files is platform dependent. So this change is just making it work as intended on all platforms.

[Czaki]

So I tried to fix the problem and run again, but cibuildwheel crashes with the error above since there is already one wheel in the wheelhouse directory

rm -rf wheelhouse && cibuildwheel --platform=macos?

[dlech]

Yes, that is the workaround I am using.

[henryiii]

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.

[dlech]

If the destination already exists but is not a directory, it may be overwritten depending on os.rename() semantics.

Exactly. This change takes away the OS semantics and makes it work the same on all OSes.

[mayeut]

I think the correct fix would be to treat a py3-none-macosx_11_0_arm64 wheel just like an ABI3 wheel, and test but not build / overwrite on newer versions. Building then overwriting wastes CI time, and doesn't really gain you much; testing the original wheel on newer Python versions gains more with less CI time. For a py3-none-any, it should continue to be an error (you don't need cibuildwheel for those). It should not overwrite in any case.

I fully agree with this assessment:

• py3-none-macosx_11_0_arm64 should be treated like an ABI3 wheel
• would like to see "destination already exists" continue to fail in CI.

The problem is not on CI but locally

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:

if exists(build_options.output_dir / repaired_wheel.name):
    if CI:
        fail with clear message
    warn a file already exists & is being removed
    delete file
shutil.move(str(repaired_wheel), build_options.output_dir)

Any thoughts on that proposal ?

[dlech]

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.

[mayeut]

I think it should fail early (before the build) rather than after the build.

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.

[joerick]

This change looks good to me.

As long as this still errors on making a pure wheel, I think it’s fine and more consistent.

This is a manual check currently, so this PR wouldn't affect that.

cibuildwheel/cibuildwheel/linux.py

Lines 244 to 245 in 46702f5

	if built_wheel.name.endswith("none-any.whl"):
	raise NonPlatformWheelError()

cibuildwheel/cibuildwheel/macos.py

Lines 386 to 387 in 46702f5

	if built_wheel.name.endswith("none-any.whl"):
	raise NonPlatformWheelError()

cibuildwheel/cibuildwheel/windows.py

Lines 354 to 356 in 46702f5

	if built_wheel.name.endswith("none-any.whl"):
	raise NonPlatformWheelError()

I fully agree with this assessment:

• py3-none-macosx_11_0_arm64 should be treated like an ABI3 wheel
• would like to see "destination already exists" continue to fail in CI.

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 build_options.output_dir for duplicates, we build build an array of produced wheels as we go, and if two build configurations produce the same wheel filename, we could error on that. Would that cover the py3-none-macosx_11_0_arm64 case you mention?

[mayeut]

we build build an array of produced wheels as we go, and if two build configurations produce the same wheel filename, we could error on that. Would that cover the py3-none-macosx_11_0_arm64 case you mention?

@joerick, that seems like a good plan. It would error out on building a py3-none-macosx_11_0_arm64 wheel with different python versions which is, I think, for now(1), the expected behavior.

1. as mentioned by @henryiii, the appropriate behavior would be to get something like was done for ABI3 wheel.

[joerick]

I'm happy to merge this as-is. I've created #1142 to cover the conversation above.

[henryiii]

Is there a reason this isn't using Path.unlink (perhaps with missing_ok=True)?

[dlech]

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.

[henryiii]

What about just unlink and keeping the try, then?

[dlech]

Both functions do the same, so I don't really see any difference in one over the other.

[henryiii]

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.

[dlech]

Done. I also changed OSError to FileNotFoundError to be more specific.

[joerick]

Thank you @dlech for the report and fix!