Add more logging to Tools/Scripts/delete-if-too-large #4574

chgibb-apple · 2022-09-21T17:44:45Z

`7380280`

Add more logging to Tools/Scripts/delete-if-too-large
https://bugs.webkit.org/show_bug.cgi?id=245489
rdar://99293146

Reviewed by Aakash Jain.

* Tools/Scripts/delete-if-too-large:

Canonical link: https://commits.webkit.org/255737@main

1fa7e4c

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🛠 mac-AS-debug	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 api-mac	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-wk1
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2
	✅ 🛠 watch	~~🧪 mac-AS-debug-wk2~~
✅ 🛠 🧪 unsafe-merge	✅ 🛠 watch-sim	✅ 🧪 mac-wk2-stress

webkit-early-warning-system · 2022-09-21T17:45:04Z

EWS run on previous version of this PR (hash a3500d4)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
❌ 🧪 style	✅ 🛠 ios	⏳ 🛠 mac	⏳ 🛠 wpe	⏳ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	⏳ 🛠 mac-debug	⏳ 🛠 gtk	⏳ 🛠 wincairo
✅ 🧪 webkitperl	⏳ 🧪 ios-wk2	⏳ 🛠 mac-AS-debug	⏳ 🧪 gtk-wk2
	⏳ 🧪 api-ios	⏳ 🧪 api-mac	⏳ 🧪 api-gtk
⏳ 🛠 🧪 jsc	⏳ 🛠 tv	⏳ 🧪 mac-wk1
✅ 🧪 services	⏳ 🛠 tv-sim	⏳ 🧪 mac-wk2
	⏳ 🛠 watch	⏳ 🧪 mac-AS-debug-wk2
	⏳ 🛠 watch-sim	⏳ 🧪 mac-wk2-stress

webkit-early-warning-system · 2022-09-21T17:50:21Z

EWS run on previous version of this PR (hash e14e113)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	⏳ 🛠 mac	⏳ 🛠 wpe	⏳ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	⏳ 🛠 mac-debug	⏳ 🛠 gtk	⏳ 🛠 wincairo
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	⏳ 🛠 mac-AS-debug	⏳ 🧪 gtk-wk2
	⏳ 🧪 api-ios	⏳ 🧪 api-mac	⏳ 🧪 api-gtk
	⏳ 🛠 tv	⏳ 🧪 mac-wk1
✅ 🧪 services	⏳ 🛠 tv-sim	⏳ 🧪 mac-wk2
	⏳ 🛠 watch	⏳ 🧪 mac-AS-debug-wk2
	⏳ 🛠 watch-sim	⏳ 🧪 mac-wk2-stress

aj062 · 2022-09-21T17:58:44Z

Tools/CISupport/build-webkit-org/steps.py

@@ -216,6 +216,15 @@ class PruneCoreSymbolicationdCacheIfTooLarge(shell.ShellCommand):
               "/System/Library/Caches/com.apple.coresymbolicationd"]


+class PruneCoreSymbolicationdDataIfTooLarge(shell.ShellCommand):


This PR is just adding the step. Need to update factory as well to invoke this step.

Indeed. I opened the PR early to get feedback from the Python linter. Related, I implemented the most naive solution to this problem. I'm not sure how to best go about parameterising PruneCoreSymbolicationdCacheIfTooLarge within our use of buildbot.

webkit-early-warning-system · 2022-09-21T18:10:02Z

EWS run on previous version of this PR (hash bb6c917)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	⏳ 🛠 mac	✅ 🛠 wpe	⏳ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	⏳ 🛠 mac-debug	✅ 🛠 gtk	🛠 wincairo
✅ 🧪 webkitperl	⏳ 🧪 ios-wk2	✅ 🛠 mac-AS-debug	✅ 🧪 gtk-wk2
	⏳ 🧪 api-ios	⏳ 🧪 api-mac	⏳ 🧪 api-gtk
	⏳ 🛠 tv	⏳ 🧪 mac-wk1
❌ 🧪 services	⏳ 🛠 tv-sim	⏳ 🧪 mac-wk2
	⏳ 🛠 watch	⏳ 🧪 mac-AS-debug-wk2
	⏳ 🛠 watch-sim	⏳ 🧪 mac-wk2-stress

webkit-early-warning-system · 2022-09-21T23:22:47Z

EWS run on previous version of this PR (hash 150a3c9)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🛠 mac-AS-debug	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 api-mac	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-wk1
✅ 🧪 services	✅ 🛠 tv-sim	✅ 🧪 mac-wk2
	✅ 🛠 watch	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch-sim	✅ 🧪 mac-wk2-stress

aj062 · 2022-09-21T23:31:52Z

Tools/CISupport/build-webkit-org/steps.py

+    flunkOnFailure = False
+    haltOnFailure = False
+    command = ["python3", "Tools/Scripts/delete-if-too-large",
+               "/System/Volumes/Data/System/Library/Caches/com.apple.coresymbolicationd"]


Running the same step/script twice just to pass two different arguments doesn't seems right. Either the script should accept multiple parameters and handle them appropriately, or the paths can be hard-coded inside the script itself.

I agree.

Related, I implemented the most naive solution to this problem. I'm not sure how to best go about parameterising PruneCoreSymbolicationdCacheIfTooLarge within our use of buildbot.

How can I go about parameterising PruneCoreSymbolicationdCacheIfTooLarge? It's not clear to me if we can actually (or should be )pass arguments or if the way we compose out list of steps is meant to be purely declarative.

How can I go about parameterising PruneCoreSymbolicationdCacheIfTooLarge?

In steps.py for the command for PruneCoreSymbolicationdCacheIfTooLarge You can pass more arguments, just like you are passing one argument currently.

You can modify delete-if-too-large script so that users can pass multiple directories. You can either use argument parser (like https://docs.python.org/dev/library/argparse.html) or for this simple user-case, you can simply loop over sys.argv and store the arguments in a list of directories. Then just loop over that list and execute your logic to delete.

If the list of directories is going to be fixed, other option is to hard-code these directories names inside the script.

aproskuryakov · 2022-09-22T02:05:18Z

Tools/CISupport/build-webkit-org/steps.py

+    flunkOnFailure = False
+    haltOnFailure = False
+    command = ["python3", "Tools/Scripts/delete-if-too-large",
+               "/System/Volumes/Data/System/Library/Caches/com.apple.coresymbolicationd"]


Isn't /System/Volumes/Data/System/Library/Caches just a worse way to write /System/Library/Caches?

I believe it is. Though, for some reason we are seeing both of these paths needed.

I think that's the same directory, just linked to two places in the hierarchy in some way.

There's some machines in the fleet that seem to need this path for some reason.

webkit-early-warning-system · 2022-09-28T23:52:15Z

EWS run on previous version of this PR (hash fb41a23)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	⏳ 🛠 mac-debug	✅ 🛠 gtk	⏳ 🛠 wincairo
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	✅ 🛠 mac-AS-debug	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	~~🧪 api-mac~~	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-wk1
✅ 🧪 services	✅ 🛠 tv-sim	✅ 🧪 mac-wk2
	✅ 🛠 watch	~~🧪 mac-AS-debug-wk2~~
	✅ 🛠 watch-sim	✅ 🧪 mac-wk2-stress

webkit-early-warning-system · 2022-09-29T16:17:33Z

EWS run on previous version of this PR (hash bdc1047)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	❌ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-debug	✅ 🛠 gtk	💥 🛠 wincairo
✅ 🧪 webkitperl	⏳ 🧪 ios-wk2	✅ 🛠 mac-AS-debug	❌ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 api-mac	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-wk1
✅ 🧪 services	✅ 🛠 tv-sim	✅ 🧪 mac-wk2
	✅ 🛠 watch	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch-sim	✅ 🧪 mac-wk2-stress

aj062 · 2022-09-29T16:34:34Z

Tools/Scripts/delete-if-too-large

+            os.remove(directory)
+            print("Deleted " + directory)
+        else:
+            print(f'{directory} is {directorySize}. Doing nothing') 


Nit: please add unit (kb/mb/gb) in this message.

this comment about adding unit is still applicable.

aj062 · 2022-09-29T16:36:52Z

Looks good to me assuming Alexey's concern about /System/Volumes/Data/System/Library/Caches and /System/Library/Caches being same is addressed.

chgibb-apple · 2022-10-01T00:08:12Z

Thanks @aj062 . With respect to @aproskuryakov 's concern, I believe they are indeed the same folder. However, we are seeing a need for both across some machines. I can close this PR and open a new one that adds additional logging.

webkit-early-warning-system · 2022-10-04T18:39:44Z

EWS run on current version of this PR (hash 1fa7e4c)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🛠 mac-AS-debug	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 api-mac	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-wk1
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2
	✅ 🛠 watch	~~🧪 mac-AS-debug-wk2~~
✅ 🛠 🧪 unsafe-merge	✅ 🛠 watch-sim	✅ 🧪 mac-wk2-stress

https://bugs.webkit.org/show_bug.cgi?id=245489 rdar://99293146 Reviewed by Aakash Jain. * Tools/Scripts/delete-if-too-large: Canonical link: https://commits.webkit.org/255737@main

webkit-commit-queue · 2022-10-19T16:21:00Z

Committed 255737@main (7380280): https://commits.webkit.org/255737@main

Reviewed commits have been landed. Closing PR #4574 and removing active labels.

chgibb-apple requested review from aj062, JonWBedard and ryanhaddad as code owners September 21, 2022 17:44

chgibb-apple force-pushed the eng/99293146 branch from a3500d4 to e14e113 Compare September 21, 2022 17:50

aj062 reviewed Sep 21, 2022

View reviewed changes

chgibb-apple force-pushed the eng/99293146 branch from e14e113 to bb6c917 Compare September 21, 2022 18:09

chgibb-apple force-pushed the eng/99293146 branch from bb6c917 to 150a3c9 Compare September 21, 2022 20:29

aj062 reviewed Sep 21, 2022

View reviewed changes

aproskuryakov reviewed Sep 22, 2022

View reviewed changes

chgibb-apple changed the title ~~Consider More coresymbolicationd Locations~~ Consider More coresymbolicationd locations. Refactor delete-if-too-large Sep 29, 2022

chgibb-apple force-pushed the eng/99293146 branch from fb41a23 to bdc1047 Compare September 29, 2022 16:14

aj062 reviewed Sep 29, 2022

View reviewed changes

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Sep 29, 2022

chgibb-apple changed the title ~~Consider More coresymbolicationd locations. Refactor delete-if-too-large~~ Add more logging to Tools/Scripts/delete-if-too-large Oct 4, 2022

chgibb-apple force-pushed the eng/99293146 branch from bdc1047 to 1fa7e4c Compare October 4, 2022 18:39

aj062 approved these changes Oct 4, 2022

View reviewed changes

JonWBedard added unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing and removed merging-blocked Applied to prevent a change from being merged labels Oct 19, 2022

Add more logging to Tools/Scripts/delete-if-too-large

7380280

https://bugs.webkit.org/show_bug.cgi?id=245489 rdar://99293146 Reviewed by Aakash Jain. * Tools/Scripts/delete-if-too-large: Canonical link: https://commits.webkit.org/255737@main

webkit-early-warning-system force-pushed the eng/99293146 branch from 1fa7e4c to 7380280 Compare October 19, 2022 16:20

webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Oct 19, 2022

webkit-early-warning-system merged commit 7380280 into WebKit:main Oct 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add more logging to Tools/Scripts/delete-if-too-large #4574

Add more logging to Tools/Scripts/delete-if-too-large #4574

chgibb-apple commented Sep 21, 2022 •

edited by webkit-early-warning-system

webkit-early-warning-system commented Sep 21, 2022 •

edited

webkit-early-warning-system commented Sep 21, 2022 •

edited

aj062 Sep 21, 2022

chgibb-apple Sep 21, 2022

webkit-early-warning-system commented Sep 21, 2022 •

edited

webkit-early-warning-system commented Sep 21, 2022 •

edited

aj062 Sep 21, 2022

chgibb-apple Sep 23, 2022

aj062 Sep 26, 2022 •

edited

aproskuryakov Sep 22, 2022

chgibb-apple Sep 23, 2022

aproskuryakov Sep 23, 2022

chgibb-apple Sep 26, 2022

webkit-early-warning-system commented Sep 28, 2022 •

edited

webkit-early-warning-system commented Sep 29, 2022 •

edited

aj062 Sep 29, 2022

aj062 Oct 4, 2022

aj062 commented Sep 29, 2022

chgibb-apple commented Oct 1, 2022

webkit-early-warning-system commented Oct 4, 2022 •

edited

webkit-commit-queue commented Oct 19, 2022

		@@ -216,6 +216,15 @@ class PruneCoreSymbolicationdCacheIfTooLarge(shell.ShellCommand):
		"/System/Library/Caches/com.apple.coresymbolicationd"]


		class PruneCoreSymbolicationdDataIfTooLarge(shell.ShellCommand):

Add more logging to Tools/Scripts/delete-if-too-large #4574

Add more logging to Tools/Scripts/delete-if-too-large #4574

Conversation

chgibb-apple commented Sep 21, 2022 • edited by webkit-early-warning-system

webkit-early-warning-system commented Sep 21, 2022 • edited

webkit-early-warning-system commented Sep 21, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webkit-early-warning-system commented Sep 21, 2022 • edited

webkit-early-warning-system commented Sep 21, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aj062 Sep 26, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webkit-early-warning-system commented Sep 28, 2022 • edited

webkit-early-warning-system commented Sep 29, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aj062 commented Sep 29, 2022

chgibb-apple commented Oct 1, 2022

webkit-early-warning-system commented Oct 4, 2022 • edited

webkit-commit-queue commented Oct 19, 2022

chgibb-apple commented Sep 21, 2022 •

edited by webkit-early-warning-system

webkit-early-warning-system commented Sep 21, 2022 •

edited

webkit-early-warning-system commented Sep 21, 2022 •

edited

webkit-early-warning-system commented Sep 21, 2022 •

edited

webkit-early-warning-system commented Sep 21, 2022 •

edited

aj062 Sep 26, 2022 •

edited

webkit-early-warning-system commented Sep 28, 2022 •

edited

webkit-early-warning-system commented Sep 29, 2022 •

edited

webkit-early-warning-system commented Oct 4, 2022 •

edited