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

[macOS] Fix COPY step on macOS 15 Sequoia #884

Merged
merged 1 commit into from
Aug 7, 2024

Conversation

cbracken
Copy link
Member

@cbracken cbracken commented Aug 7, 2024

macOS build fails on Sequoia beta 2 and later (verified in public beta 3 as well) hosts during ninja COPY steps when copying files with the restricted extended attribute set. As part of our build, we copy the /System/Library/Fonts/Apple Color Emoji.ttc which is one such file.

The problem appears to be due to the -a (archive) option, which is an alias for -RpP. Of these, the -p option appears to be the source of the problem. This flag attempts to preserve file metadata including Access Control Lists and Extended Attributes. However, the restricted extended attribute is not settable when System Integrity Protection (SIP) is enabled, which it is by default.

This appears to be a change in the implementation of cp in macOS 15 Sequoia beta 2 and later. Previous versions would fail to copy the attribute, but continue without failing. The current version appears to fail if it cannot set a given extended attribute.

Two alternatives to the previous cp -af step:

  • Use cp -Rf. This succeeds but file metadata such as creation time, modification time, etc. are not preserved.
  • Use rsync -arl. rsync's -a flag also copies metadata but on a best-efforts basis and doesn't error out when it fails to set the restricted attribute.

Of the two, the latter more closely approximates the previous behaviour.

Fixes: flutter/flutter#152978

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read the Flutter Style Guide recently, and have followed its advice.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@@ -222,7 +222,7 @@ template("mac_toolchain") {
}

tool("copy") {
command = "ln -f {{source}} {{output}} 2>/dev/null || (rm -rf {{output}} && cp -af {{source}} {{output}})"
command = "ln -f {{source}} {{output}} 2>/dev/null || (rm -rf {{output}} && rsync -arl {{source}} {{output}})"
Copy link
Member

@jmagman jmagman Aug 7, 2024

Choose a reason for hiding this comment

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

  1. man rsync says -a is the same as -rlptgoD so do we need the rl?
  2. Instead of the rm -rf we can use rsync --delete so it will only copy what's needed, but delete anything extraneous (this is what we do in the tool)
    https://github.com/flutter/flutter/blob/0a7f8af6d14696e9086cdd390792f7eb8a9cefa5/packages/flutter_tools/lib/src/base/os.dart#L458-L461
Suggested change
command = "ln -f {{source}} {{output}} 2>/dev/null || (rm -rf {{output}} && rsync -arl {{source}} {{output}})"
command = "ln -f {{source}} {{output}} 2>/dev/null || (rsync -a --delete {{source}} {{output}})"

Copy link
Member

Choose a reason for hiding this comment

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

Also, do you know what's up with the symlink part?

Copy link
Member Author

@cbracken cbracken Aug 7, 2024

Choose a reason for hiding this comment

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

so do we need the rl?

Good catch; nope, safe to remove.

do you know what's up with the symlink part?

The code is trying to be clever. The logic is basically:

  1. If possible, create a hard link (notice it's not -s), deleting any existing file (because of the -f).
  2. If that's not possible (e.g. the source file is on a different volume), then rm -rf the previous file and copy the file.

This logic definitely predates APFS. With APFS, copies are free (well you still pay the cost of the inodes) and the copy is materialised only if you edit the file. i.e. APFS is effectively copy-on-write. I think we can probably safely replace the whole thing unless we have bots with non-APFS volumes, but I think we should probably do that in a follow-up patch after we check our bots.

Instead of the rm -rf we can use rsync --delete

Looks like the default behaviour is --delete-before when --delete is specified, so I think this would preserve the existing behaviour exactly. Will do a quick test locally just out of paranoia.

Copy link
Member Author

@cbracken cbracken Aug 7, 2024

Choose a reason for hiding this comment

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

Re: APFS, looking closer at the cp code, cp only takes advantage of cloned inodes if the -c option is passed, but falls back to copyfile() if clonefile() fails, which presumably is under the same conditions as hardlinking would -- i.e. source and destination on a different volume.

I'd have to look to see if rsync takes advantage of this or not; there's no mention in the manpage.

Regardless, we should keep the ln until we can convince ourselves that just using plain rsync won't result in worse performance and more disk usage.

Copy link
Member Author

Choose a reason for hiding this comment

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

Based on a really quick grep of rsync it doesn't appear Apple is using clonefile or passing COPYFILE_CLONE etc. to copyfile flags so tempted to keep the ln.

macOS build fails on Sequoia beta 2 and later (verified in public beta 3
as well) hosts during COPY step when copying files with the `restricted`
extended attribute set.

The problem appears to be due to the `-a` (archive) option, which is an
alias for `-RpP`. Of these, the `-p` option appears to be the source of
the problem. This flag attempts to preserve file metadata including
Access Control Lists and Extended Attributes. However, the `restricted`
extended attribute is not settable when System Integrity Protection
(SIP) is enabled, which it is by default.

This appears to be a change in the implementation of `cp` in macOS 15
Sequoia beta 2 and later. Previous versions would fail to copy the
attribute, but continue without failing. The current version appears to
fail if it cannot set any extended attribute.

Two alternatives to the previous `cp -af` step:
* Use `cp -Rf`. This succeeds but file metadata such as
  creation time, modification time, etc. are not preserved.
* Use `rsync -a`. rsync's -a flag also copies metadata but on a
  best-efforts basis and doesn't error out when it fails to set the
  restricted attribute.

Of the two, the latter more closely approximates the previous behaviour.

The `rm -rf` step can be replaced with `rsync`'s `--delete` option; with
the default `--delete-before` behaviour, this replicates the function of
the `rm -rf`.

Fixes: flutter/flutter#152978
@cbracken cbracken merged commit 6ef931b into flutter:master Aug 7, 2024
1 check passed
@cbracken cbracken deleted the rsync-not-cp branch August 7, 2024 05:15
cbracken added a commit to cbracken/flutter_engine that referenced this pull request Aug 7, 2024
Contains:

* [macOS] Fix COPY step on macOS 15 Sequoia (flutter/buildroot#884)
* Bump actions/upload-artifact from 4.3.4 to 4.3.5 (flutter/buildroot#882)
* Bump github/codeql-action from 3.25.14 to 3.25.15 (flutter/buildroot#880)
* Bump ossf/scorecard-action from 2.3.3 to 2.4.0 (flutter/buildroot#881)

Issue: flutter/flutter#152978
auto-submit bot pushed a commit to flutter/engine that referenced this pull request Aug 7, 2024
Contains:

* [macOS] Fix COPY step on macOS 15 Sequoia (flutter/buildroot#884)
* Bump actions/upload-artifact from 4.3.4 to 4.3.5 (flutter/buildroot#882)
* Bump github/codeql-action from 3.25.14 to 3.25.15 (flutter/buildroot#880)
* Bump ossf/scorecard-action from 2.3.3 to 2.4.0 (flutter/buildroot#881)

Issue: flutter/flutter#152978

[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
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.

Engine host build fails on macOS 15 (Sequoia) beta 2 and later
3 participants