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

Updated macos copy_dir_contents_to_dir to be like other platforms #687

Merged
merged 2 commits into from
Jun 22, 2021

Conversation

UebelAndre
Copy link
Collaborator

@UebelAndre UebelAndre commented Jun 18, 2021

Unfortunately, the implementation of cp for macos does not have --no-target-directory

-T, --no-target-directory
              treat DEST as a normal file

This is likely why the macos implementation of copy_dir_contents_to_dir always differered (looking back to #196 and #377).

This change aims to bring the macos implementation more into alignment and avoid the recursive function that was there before.

https://unix.stackexchange.com/a/109704 seems to be a good description of the behavior of --no-target-directory and I believe I've encompassed that in this change.

@UebelAndre UebelAndre marked this pull request as ready for review June 18, 2021 02:35
@UebelAndre UebelAndre requested a review from oquenchil as a code owner June 18, 2021 02:35
@UebelAndre UebelAndre requested a review from jsharpe June 18, 2021 02:35
@UebelAndre
Copy link
Collaborator Author

Sounds like this resolved issues for #622 (comment)

@UebelAndre UebelAndre mentioned this pull request Jun 21, 2021
Copy link
Member

@jsharpe jsharpe left a comment

Choose a reason for hiding this comment

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

I wonder if we can achieve the same with ditto (https://ss64.com/osx/ditto.html) but lets go with this as its been tested and works

@jsharpe jsharpe merged commit 96dc580 into bazel-contrib:main Jun 22, 2021
@UebelAndre
Copy link
Collaborator Author

I wonder if we can achieve the same with ditto (https://ss64.com/osx/ditto.html) but lets go with this as its been tested and works

OOH! I didn't know that existed, very interesting! If there are any issues from this, that'll definitely be the first thing to try.

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