Skip to content

Extended Attributes on macOS #973

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 9 commits into from
May 11, 2023
Merged

Extended Attributes on macOS #973

merged 9 commits into from
May 11, 2023

Conversation

srivarra
Copy link
Contributor

@srivarra srivarra commented Apr 7, 2023 •
edited
Loading

If you haven't already, please read through our contributing guidelines before opening your PR

What is the purpose of this PR?

Closes #936. Fixes an issue with cluster_pixels where on macOS, the meta file (prefixed with "._") gets unlinked when the original file gets unlinked.

How did you implement your changes

Added the solution described in the issue post. The test tests that the logic of that function works appropriately, but not testing it's usage in rmtree.

Remaining issues

N/A

@srivarra srivarra added the enhancement New feature or request label Apr 7, 2023
@srivarra srivarra self-assigned this Apr 7, 2023
@srivarra srivarra added bug Something isn't working and removed enhancement New feature or request labels Apr 7, 2023
@srivarra
Copy link
Contributor Author

srivarra commented Apr 7, 2023

@camisowers @alex-l-kong Do you guys have any suggestions on testing this? I'm not too sure how to go about it.

@alex-l-kong
Copy link
Contributor

You could try creating some dummy ._ files within a folder recursively to see if it works.

I've been using subprocess.call(["find", folder_path, "-type", "f", "-name", "._*-delete"]) prior to shutil.rmtree in Rosetta to handle this, seems to work just as well. I think Noah suggested that this approach might be easier, reason being we don't use shutil.rmtree very often for external drives.

@ngreenwald can you confirm which solution you feel is better?

@srivarra srivarra marked this pull request as ready for review April 28, 2023 17:03
camisowers
camisowers approved these changes Apr 28, 2023
View reviewed changes
Copy link
Contributor

@camisowers camisowers left a comment

Choose a reason for hiding this comment

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

Looks good.

alex-l-kong
alex-l-kong approved these changes Apr 28, 2023
View reviewed changes
Copy link
Contributor

@alex-l-kong alex-l-kong left a comment

Choose a reason for hiding this comment

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

👍

@srivarra srivarra added this pull request to the merge queue Apr 28, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 28, 2023
@srivarra srivarra enabled auto-merge May 10, 2023 23:37
@srivarra srivarra disabled auto-merge May 10, 2023 23:37
@srivarra srivarra added this pull request to the merge queue May 11, 2023
Merged via the queue into main with commit abc15e7 May 11, 2023
@srivarra srivarra deleted the extended_attribute_file_support branch May 11, 2023 19:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alex-l-kong alex-l-kong alex-l-kong approved these changes

@camisowers camisowers camisowers approved these changes

Assignees

@srivarra srivarra

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

cluster_pixels function fails to delete pixel_mat_data folder
3 participants
@srivarra @alex-l-kong @camisowers