Skip to content
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 for reflink - build reflink for macos11 #877

Merged
merged 6 commits into from
Jun 22, 2023
Merged

Conversation

olsen232
Copy link
Collaborator

Description

Building for macos10.9 (as we were doing previously) or equivalently pip installing it with the macos python version we have, ends up with a stub version of reflink that can't actually reflink anything.

Fixes a test that was failing due to reflink being less powerful than shutil.copy (it can't overwrite existing files, so we have to make sure the old files are out of the way).

Adds tests that actually check if things are reflinked.

#876

  • Have you reviewed your own change?
  • Have you included test(s)?
  • Have you updated the changelog?

olsen232 and others added 5 commits June 22, 2023 09:18
On macOS, typically we use a Python.org distribution which has been
built with compatibility back to OSX10.9 (via MACOSX_DEPLOYMENT_TARGET).
reflink doesn't build on releases older than 10.12.

Build reflink in vcpkg-vendor like the other python wheels, which
forces/overrides the deployment target version.
@olsen232 olsen232 requested review from craigds and rcoup June 21, 2023 22:30
Copy link
Member

@rcoup rcoup left a comment

Choose a reason for hiding this comment

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

LGTM 👍

fienode = shutil.which("fienode")
has_checker = bool(clone_checker or fienode)

def _check_tile_is_reflinked(tile_path, repo, do_raise_skip=False):
Copy link
Member

Choose a reason for hiding this comment

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

wrt do_raise_skip, why wouldn't we always skip if the OS/infra doesn't support the test/feature? That's the pattern we've used elsewhere IIUC

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added a couple new tests, both called test_working_copy_reflink.
If reflink is missing, we skip them.
However, I also used this same fixture to add some more reflink asserts to some existing import tests. Those tests should now make extra asserts - if they are able to do so - but shouldn't skip the test if they can't.


def test_working_copy_reflink(cli_runner, data_archive, check_tile_is_reflinked):
if is_windows:
# Nothing to test, so let's mark test will show as "passed".
Copy link
Member

Choose a reason for hiding this comment

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

same here...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah I was unsure what to do here: wasn't sure if I should try to use pytest.skip to mean "functionality is untested" and just a pass to mean "nothing to test". But I think you're right, pytest.skip makes more sense here.

@olsen232 olsen232 merged commit 454e56f into master Jun 22, 2023
@olsen232 olsen232 deleted the macosx-version-11 branch June 22, 2023 00:07
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.

2 participants