[macOS] Fix COPY step on macOS 15 Sequoia #884
Conversation
build/toolchain/mac/BUILD.gn
Outdated
|
|
||
| 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}})" |
There was a problem hiding this comment.
man rsyncsays-ais the same as-rlptgoDso do we need therl?- Instead of the
rm -rfwe can usersync --deleteso 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
| 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}})" |
There was a problem hiding this comment.
Also, do you know what's up with the symlink part?
There was a problem hiding this comment.
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:
- If possible, create a hard link (notice it's not
-s), deleting any existing file (because of the-f). - If that's not possible (e.g. the source file is on a different volume), then
rm -rfthe 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 -rfwe can usersync --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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
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
macOS build fails on Sequoia beta 2 and later (verified in public beta 3 as well) hosts during ninja
COPYsteps when copying files with therestrictedextended attribute set. As part of our build, we copy the/System/Library/Fonts/Apple Color Emoji.ttcwhich is one such file.The problem appears to be due to the
-a(archive) option, which is an alias for-RpP. Of these, the-poption appears to be the source of the problem. This flag attempts to preserve file metadata including Access Control Lists and Extended Attributes. However, therestrictedextended 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
cpin 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 -afstep:cp -Rf. This succeeds but file metadata such as creation time, modification time, etc. are not preserved.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
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.