[CMake] All post-commit buildbots should build with --no-fatal-warnings #5710

mcatanzaro · 2022-10-24T16:28:03Z

`e868bfe`

[CMake] All post-commit buildbots should build with --no-fatal-warnings
https://bugs.webkit.org/show_bug.cgi?id=246760

Reviewed by Jonathan Bedard.

We need to enable fatal warnings on EWS to prevent developers from
introducing new warnings. But we do not want fatal warnings on
post-commit bots, since that may result in lost test results, which
would be a very harsh price to pay for an unused variable or whatnot.

Note this is not needed for the JSC bots since those do not use
ENABLE_DEVELOPER_MODE.

* Tools/CISupport/build-webkit-org/steps.py:
(CompileWebKit):
* Tools/CISupport/build-webkit-org/steps_unittest.py:
(TestCompileWebKit.test_success):
(TestCompileWebKit.test_success_gtk):
(TestCompileWebKit.test_success_wpe):
(TestCompileWebKit.test_failure):

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

f6c7213

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
✅ 🛠 🧪 unsafe-merge	✅ 🛠 watch-sim	✅ 🧪 mac-wk2-stress

webkit-early-warning-system · 2022-10-24T16:30:19Z

EWS run on previous version of this PR (hash 0d64f55)

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	✅ 🛠 jsc-armv7
❌ 🧪 services	✅ 🛠 tv-sim	⏳ 🧪 mac-wk2	✅ 🧪 jsc-armv7-tests
	✅ 🛠 watch	⏳ 🧪 mac-AS-debug-wk2	✅ 🛠 jsc-mips
	✅ 🛠 watch-sim	⏳ 🧪 mac-wk2-stress	~~🧪 jsc-mips-tests~~

mcatanzaro · 2022-10-24T16:31:14Z

I'm sure there's a better way to do this, but this should allow me to land #5708 without triggering an apocalypse on any post-release bots.

webkit-early-warning-system · 2022-10-24T16:33:22Z

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

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

JonWBedard

These changes will need to be landed either squashed or in two separate PRs, we'll also need to coordinate a build.webkit.org restart.

mcatanzaro · 2022-10-24T16:35:45Z

These changes will need to be landed either squashed or in two separate PRs, we'll also need to coordinate a build.webkit.org restart.

Sorry, that was just an error. It was supposed to be only one commit. Fixed.

mcatanzaro · 2022-10-24T16:39:14Z

we'll also need to coordinate a build.webkit.org restart.

How much coordination is required here? Any chance it's just a big blue button that you press and then everything restarts smoothly?

BTW it looks like the build.webkit.org unit tests are failing due to what appears to be a preexisting issue:

 ['configure-build',
   'configuration',
   'clean-and-update-working-directory',
   'checkout-specific-revision',
   'show-identifier',
   'kill-old-processes',
   'delete-WebKitBuild-directory',
   'delete-stale-build-files',
   'jhbuild',
   'compile-webkit',
   'layout-test',
   'dashboard-tests',
   'archive-test-results',
   'upload',
   'extract-test-results',
   'set-permissions',
   'webkitpy-test',
   'webkitperl-test',
   'bindings-generation-tests',
   'builtins-generator-tests',
   'API-tests',
+  'archive-built-product',
+  'upload-built-product',
+  'trigger',
   'webdriver-test'] : Expected steps don't match for builder GTK-Linux-64-bit-Release-GTK4-Tests

Tools/CISupport/build-webkit-org/config.json

lauromoura · 2022-10-24T18:28:02Z

BTW it looks like the build.webkit.org unit tests are failing due to what appears to be a preexisting issue:

Tested locally and this failure doesn't appear in the current main, but appears in the pull request.

The cause is in https://github.com/WebKit/WebKit/blob/main/Tools/CISupport/build-webkit-org/factories.py#L180

BuildAndTestAllButJSCFactory subclasses BuildAndTestFactory but doesn't forwards the triggers parameter, which should go between architecture and additionalArguments in the parent constructor invocation.

So in practice it was incidentally passing before because additionalArguments was empty, which is not the case in the current patch.

JonWBedard · 2022-10-24T19:12:31Z

we'll also need to coordinate a build.webkit.org restart.

How much coordination is required here? Any chance it's just a big blue button that you press and then everything restarts smoothly?

It's not quite that easy, usually it's best to have the restarted and the lander be the same person. If this is ready to go tomorrow morning and @mcatanzaro you let me know you're ready to land it, I can land and do the restart first thing tomorrow morning when I get on.

BTW it looks like the build.webkit.org unit tests are failing due to what appears to be a preexisting issue:

 ['configure-build',
   'configuration',
   'clean-and-update-working-directory',
   'checkout-specific-revision',
   'show-identifier',
   'kill-old-processes',
   'delete-WebKitBuild-directory',
   'delete-stale-build-files',
   'jhbuild',
   'compile-webkit',
   'layout-test',
   'dashboard-tests',
   'archive-test-results',
   'upload',
   'extract-test-results',
   'set-permissions',
   'webkitpy-test',
   'webkitperl-test',
   'bindings-generation-tests',
   'builtins-generator-tests',
   'API-tests',
+  'archive-built-product',
+  'upload-built-product',
+  'trigger',
   'webdriver-test'] : Expected steps don't match for builder GTK-Linux-64-bit-Release-GTK4-Tests

Looks like @lauromoura broke down what happened here.

mcatanzaro · 2022-10-24T19:14:13Z

It's not quite that easy, usually it's best to have the restarted and the lander be the same person. If this is ready to go tomorrow morning and @mcatanzaro you let me know you're ready to land it, I can land and do the restart first thing tomorrow morning when I get on.

It's not ready currently. I will try to implement Lauro's suggestion and then we'll see if that works. Will let you know.

BuildAndTestAllButJSCFactory subclasses BuildAndTestFactory but doesn't forwards the triggers parameter, which should go between architecture and additionalArguments in the parent constructor invocation.

So in practice it was incidentally passing before because additionalArguments was empty, which is not the case in the current patch.

After updating this pull request, I'll no longer be touching additionalArguments, so want to report a bug for this?

lauromoura · 2022-10-24T19:33:04Z

After updating this pull request, I'll no longer be touching additionalArguments, so want to report a bug for this?

I can submit it, alongside the fix.

webkit-early-warning-system · 2022-10-24T20:58:25Z

EWS run on previous version of this PR (hash 4abb3ad)

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

mcatanzaro · 2022-10-24T21:19:35Z

@lauromoura does this look better?

mcatanzaro · 2022-10-24T21:19:51Z

(I need to update the test expectations... incoming.)

webkit-early-warning-system · 2022-10-24T21:26:40Z

EWS run on current version of this PR (hash f6c7213)

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
✅ 🛠 🧪 unsafe-merge	✅ 🛠 watch-sim	✅ 🧪 mac-wk2-stress

mcatanzaro · 2022-10-24T21:38:10Z

If this is ready to go tomorrow morning and @mcatanzaro you let me know you're ready to land it, I can land and do the restart first thing tomorrow morning when I get on.

I think this is ready from my side. Let's just wait and see if Lauro is happy. Thanks @JonWBedard !

lauromoura · 2022-10-25T02:41:31Z

Looks better now. Thanks for the update.

mcatanzaro · 2022-10-25T12:24:58Z

OK, this is ready to land.

https://bugs.webkit.org/show_bug.cgi?id=246760 Reviewed by Jonathan Bedard. We need to enable fatal warnings on EWS to prevent developers from introducing new warnings. But we do not want fatal warnings on post-commit bots, since that may result in lost test results, which would be a very harsh price to pay for an unused variable or whatnot. Note this is not needed for the JSC bots since those do not use ENABLE_DEVELOPER_MODE. * Tools/CISupport/build-webkit-org/steps.py: (CompileWebKit): * Tools/CISupport/build-webkit-org/steps_unittest.py: (TestCompileWebKit.test_success): (TestCompileWebKit.test_success_gtk): (TestCompileWebKit.test_success_wpe): (TestCompileWebKit.test_failure): Canonical link: https://commits.webkit.org/255954@main

webkit-commit-queue · 2022-10-25T13:38:02Z

Committed 255954@main (e868bfe): https://commits.webkit.org/255954@main

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

mcatanzaro requested review from aj062, JonWBedard and ryanhaddad as code owners October 24, 2022 16:28

mcatanzaro self-assigned this Oct 24, 2022

mcatanzaro added CMake Bugzilla component for CMake build system changes WebKit Nightly Build labels Oct 24, 2022

mcatanzaro force-pushed the eng/no-fatal-warnings branch from 0d64f55 to 2065253 Compare October 24, 2022 16:32

JonWBedard approved these changes Oct 24, 2022

View reviewed changes

mcatanzaro requested review from dpino and lauromoura October 24, 2022 16:35

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

lauromoura reviewed Oct 24, 2022

View reviewed changes

Tools/CISupport/build-webkit-org/config.json Outdated Show resolved Hide resolved

mcatanzaro removed the merging-blocked Applied to prevent a change from being merged label Oct 24, 2022

mcatanzaro force-pushed the eng/no-fatal-warnings branch from 2065253 to 4abb3ad Compare October 24, 2022 20:56

mcatanzaro force-pushed the eng/no-fatal-warnings branch from 4abb3ad to f6c7213 Compare October 24, 2022 21:26

lauromoura mentioned this pull request Oct 25, 2022

[build.webkit.org] BuildAndTestAllButJSCFactory must forward parameters correctly to BuildAndTestFactory #5729

Merged

JonWBedard added the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Oct 25, 2022

webkit-early-warning-system force-pushed the eng/no-fatal-warnings branch from f6c7213 to e868bfe Compare October 25, 2022 13:37

webkit-early-warning-system merged commit e868bfe into WebKit:main Oct 25, 2022

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

mcatanzaro deleted the eng/no-fatal-warnings branch December 22, 2022 23:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CMake] All post-commit buildbots should build with --no-fatal-warnings #5710

[CMake] All post-commit buildbots should build with --no-fatal-warnings #5710

mcatanzaro commented Oct 24, 2022 •

edited by webkit-early-warning-system

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

edited

mcatanzaro commented Oct 24, 2022

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

edited

JonWBedard left a comment

mcatanzaro commented Oct 24, 2022

mcatanzaro commented Oct 24, 2022

lauromoura commented Oct 24, 2022

JonWBedard commented Oct 24, 2022

mcatanzaro commented Oct 24, 2022

lauromoura commented Oct 24, 2022

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

edited

mcatanzaro commented Oct 24, 2022

mcatanzaro commented Oct 24, 2022

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

edited

mcatanzaro commented Oct 24, 2022

lauromoura commented Oct 25, 2022

mcatanzaro commented Oct 25, 2022

webkit-commit-queue commented Oct 25, 2022

[CMake] All post-commit buildbots should build with --no-fatal-warnings #5710

[CMake] All post-commit buildbots should build with --no-fatal-warnings #5710

Conversation

mcatanzaro commented Oct 24, 2022 • edited by webkit-early-warning-system

e868bfe

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

mcatanzaro commented Oct 24, 2022

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

JonWBedard left a comment

Choose a reason for hiding this comment

mcatanzaro commented Oct 24, 2022

mcatanzaro commented Oct 24, 2022

lauromoura commented Oct 24, 2022

JonWBedard commented Oct 24, 2022

mcatanzaro commented Oct 24, 2022

lauromoura commented Oct 24, 2022

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

mcatanzaro commented Oct 24, 2022

mcatanzaro commented Oct 24, 2022

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

mcatanzaro commented Oct 24, 2022

lauromoura commented Oct 25, 2022

mcatanzaro commented Oct 25, 2022

webkit-commit-queue commented Oct 25, 2022

mcatanzaro commented Oct 24, 2022 •

edited by webkit-early-warning-system

`e868bfe`

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

edited

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

edited

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

edited

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

edited