-
Notifications
You must be signed in to change notification settings - Fork 6k
Copy gen_snapshots using python's shutil.copy, avoid links #55830
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. Alternatively, you could tweak the macOS copy rule in the buildroot to drop the ln and just always rsync.
|
May I land this? Seems good to go. |
I wanted to get another failure before landing this to confirm that this will fix it. We haven't had a failure in a few days - we landed flutter/buildroot#910 since then as well(which should not fix anything, just will provide another data point regarding whether 2nd run is successful or not). |
| "$dart_src/runtime/bin:$gen_snapshot_target_name($build_toolchain)" | ||
|
|
||
| copy(target_name) { | ||
| action(target_name) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the rationale for only using it here and not updating the toolchain definition so that all copy targets use it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was to limit impact to only gen_snapshots. Only them had an issue with Gatekeeper.
Generally speaking ln has an advantage of reduced disk space usage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Presumably, Gatekeeper would also impact other binaries as well. If this is the fix for gen_snapshot, then we wouldn't want to go through all this investigation again a second time for some other binary. I think it's fine to do it just for gen_snpashot for now as an experiment, but if it works, there has to be follow-up work to deploy the solution more generally.
|
Triage: The investigation of this issue might have gone in a different direction. Do we still need this PR? |
Have been waiting for a recent failure before submitting this fix. |
…157323) flutter/engine@8828844...30bd6c9 2024-10-22 [email protected] Roll Fuchsia Linux SDK from avSAUwQTf5xVGuZQU... to xPbin_33tT-_omeQT... (flutter/engine#56011) 2024-10-22 [email protected] Copy gen_snapshots using python's shutil.copy, avoid links (flutter/engine#55830) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from avSAUwQTf5xV to xPbin_33tT-_ If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
|
I ran into the following error after pulling past this commit and building today. flutter/flutter#157367 |
|
A reason for requesting a revert of flutter/engine/55830 could |
|
Reason for revert: Incremental builds are broken. flutter/flutter#157367 |
…55830)" (#56034) Reverts: #55830 Initiated by: chinmaygarde Reason for reverting: Incremental builds are broken. flutter/flutter#157367 Original PR Author: aam Reviewed By: {mraleph, cbracken} This change reverts the following previous change: Default implementation of copy does it via hardlink, which seems to be causing issues with Gatekeeper on mac. BUG=flutter/flutter#154437
…lutter#55830)" This reverts commit 78f4ffa as it addresses the issues with copying over existing link.
…ngine#55830) Default implementation of copy does it via hardlink, which seems to be causing issues with Gatekeeper on mac. BUG=flutter#154437
…lutter#55830)" (flutter/engine#56034) Reverts: flutter/engine#55830 Initiated by: chinmaygarde Reason for reverting: Incremental builds are broken. flutter#157367 Original PR Author: aam Reviewed By: {mraleph, cbracken} This change reverts the following previous change: Default implementation of copy does it via hardlink, which seems to be causing issues with Gatekeeper on mac. BUG=flutter#154437
Default implementation of copy does it via hardlink, which seems to be causing issues with Gatekeeper on mac.
BUG=flutter/flutter#154437