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

[build.webkit.org] Add WebKitGTK and WPE bots for Ubuntu 22.04 #1323

Closed
wants to merge 3 commits into from

Conversation

dpino
Copy link
Contributor

@dpino dpino commented Jun 6, 2022

c28c6a1

[build.webkit.org] Add WebKitGTK and WPE bots for Ubuntu 22.04
https://bugs.webkit.org/show_bug.cgi?id=241335

Reviewed by Adrian Perez de Castro and Aakash Jain.

* Tools/CISupport/build-webkit-org/config.json: Add two new entries for
  building WebKitGTK and WPE on Ubuntu 20.04. The current Ubuntu LTS
  bots are now building WebKitGTK and WPE on Ubuntu 22.04.
* Tools/CISupport/build-webkit-org/factories_unittest.py: Add two new
  entries for testing WebKitGTK and WPE on Ubuntu 20.04.
* Tools/glib/dependencies/apt: Install package 'python-gi' only if available.
* Tools/gtk/dependencies/apt: Install packages only if available.

Canonical link: https://commits.webkit.org/251526@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@295521 268f45cc-cd09-0410-ab3c-d52691b4dbfc

@dpino dpino force-pushed the feat/switch-ubuntu-lts-to-22.04 branch from 50b8434 to 3bfd690 Compare June 6, 2022 14:48
@dpino dpino requested a review from aj062 June 6, 2022 14:52
@webkit-early-warning-system webkit-early-warning-system added the merging-blocked Applied to prevent a change from being merged label Jun 6, 2022
@dpino dpino self-assigned this Jun 7, 2022
@dpino dpino force-pushed the feat/switch-ubuntu-lts-to-22.04 branch from 3bfd690 to cd0fa0c Compare June 8, 2022 05:02
@dpino dpino requested a review from JonWBedard as a code owner June 8, 2022 05:02
@dpino dpino force-pushed the feat/switch-ubuntu-lts-to-22.04 branch 2 times, most recently from 5de0bf0 to 4efb87f Compare June 8, 2022 05:18
zdobersek and others added 2 commits June 14, 2022 10:01
https://bugs.webkit.org/show_bug.cgi?id=241591

Patch by Žan Doberšek <zdobersek@igalia.com> on 2022-06-14
Unreviewed, adding missing RISCV64 build guards alongside guards for other
64-bit platforms to get the build back up and running.

* Source/JavaScriptCore/llint/WebAssembly.asm:
* Source/JavaScriptCore/wasm/WasmMemoryInformation.cpp:
(JSC::Wasm::PinnedRegisterInfo::get):

Canonical link: https://commits.webkit.org/251524@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@295519 268f45cc-cd09-0410-ab3c-d52691b4dbfc
https://bugs.webkit.org/show_bug.cgi?id=241081

Reviewed by Dewei Zhu.

The lsof command is shipped on Linux typically on /usr/bin meanwhile on
Mac is shipped on /usr/sbin. Check if is on PATH before defaulting to the
Mac path.

* Tools/Scripts/webkitpy/benchmark_runner/http_server_driver/simple_http_server_driver.py:
  (SimpleHTTPServerDriver._find_http_server_port):

Canonical link: https://commits.webkit.org/251525@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@295520 268f45cc-cd09-0410-ab3c-d52691b4dbfc
@dpino dpino requested review from aperezdc and removed request for ryanhaddad June 14, 2022 10:50
@aperezdc
Copy link
Contributor

@dpino Changes look fine to me, you might want to have someone else like @aj062 rubber-stamp it as well 😺

@dpino dpino added merge-queue Applied to send a pull request to merge-queue and removed merging-blocked Applied to prevent a change from being merged labels Jun 14, 2022
https://bugs.webkit.org/show_bug.cgi?id=241335

Reviewed by Adrian Perez de Castro and Aakash Jain.

* Tools/CISupport/build-webkit-org/config.json: Add two new entries for
  building WebKitGTK and WPE on Ubuntu 20.04. The current Ubuntu LTS
  bots are now building WebKitGTK and WPE on Ubuntu 22.04.
* Tools/CISupport/build-webkit-org/factories_unittest.py: Add two new
  entries for testing WebKitGTK and WPE on Ubuntu 20.04.
* Tools/glib/dependencies/apt: Install package 'python-gi' only if available.
* Tools/gtk/dependencies/apt: Install packages only if available.

Canonical link: https://commits.webkit.org/251526@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@295521 268f45cc-cd09-0410-ab3c-d52691b4dbfc
@webkit-early-warning-system
Copy link
Collaborator

Committed r295521 (251526@main): https://commits.webkit.org/251526@main

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

@webkit-early-warning-system webkit-early-warning-system removed the merge-queue Applied to send a pull request to merge-queue label Jun 14, 2022
@aj062
Copy link
Member

aj062 commented Jun 15, 2022

Restarted buildbot to pick up this change

if apt-cache show $1 &>/dev/null; then
echo $1
else
echo $2
fi
}

aptIfExists() {
local ret=$(apt show "$1" 2>/dev/null)

Choose a reason for hiding this comment

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

@dpino I'm not going to pretend to understand quite why this is the case, but per this comment this formulation needs to look instead like local ret; ret=$(apt show "$1" 2>/dev/null) (but maybe with a newline? see below), otherwise $? always ends up 0 and running Tools/gtk/install-dependencies will try to install a bunch of nonexistent packages like "libenchant-dev" and this breaks the install request such that important packages like "ruby" won't be installed.

I ran into this when trying to update the searchfox wubkat tree to use the new explicit support for Ubuntu 22.04 by using the install-dependencies script and the configure step failed because there was no ruby because the call to "install-dependencies" only installed a few python packages. I added set -x to the script so I could see what the commands were and it was clear from the output that the evaluation of $? in the next line was incorrectly 0 so I searched and found the above stackoverflow comment and when I changed the script to instead have:

    local ret;
    ret=$(apt show "$1" 2>/dev/null)

the script then installed everything as expected. I only tried this formulation with the newline, I didn't try on the same line.

Note that I think it's also the case that apt is explicitly not supposed to be used for scripting so it might be better to use apt-cache show here like is used above? Like when used in a sub-shell it complains "WARNING: apt does not have a stable CLI interface. Use with caution in scripts."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the pointer. This was indeed a source of errors that was preventing running install-dependencies in different platforms.

The reason why the code isn't working as it's writen is because in Bash an assigment always returns exit code 0. So, as a rule of thumb, is generally wrong in Bash to initialize a variable and inmediately after check the result of its assigned expression. That's why it's necessary to break down the statement into declaration of var, assigned expression and evaluation of result. I remember @aperezdc actually warned me about this once in another review of this code.

Unrelated to this but perhaps you already noticed you also needed to manually install the package unidef. It's pending to be added in apt dependencies file.

Choose a reason for hiding this comment

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

Shell-wise: Aha, that does make sense, thanks! That will probably make a good addition to https://github.com/mozsearch/mozsearch/blob/master/docs/bash-scripting-cheatsheet.md

Build-wise: Indeed, the build broke because of (I'm assuming there's a missing "f" typo in your current comment) unifdef not being installed and then I (very gratefully!) realized that support for Ubuntu 22.04 had been added so I thought I'd try using it. I've restored the "wubkat" indexer behavior to fail if the build step fails, so once the build starts working again wubkat should go back to always having semantic data available at the trade-off of the webkit tree seeming to have fatal build errors surprisingly frequently so data will be stale when that happens.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, the name of the package is unifdef. I noticed it has been already added in Tools/gtk/dependencies/atk in the patch that introduced this dependency 253528@main. So nothing pending to be done here.

We're currently in the middle of an upgrade of webkit-search.igalia.com to Ubuntu 22.04 (plus upgrade of the mozsearch checkout). I've been reviewing your wubkat scripts. They're very useful, I learned many things. There are other things, on the hand, that could be improved or you'd like to reconsinder. I think I will open a PR to comment on the code.

Choose a reason for hiding this comment

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

A PR would be fantastic, thank you! I very much am not familiar with building webkit and mainly was just trying to get things working at all, and clearly leaned very heavily on your existing (public) config and the config in emilio's branch.

The canonical searchfox discussion channel is https://chat.mozilla.org/#/room/#searchfox:mozilla.org (and maybe it's federated?) if you want to chat more about searchfox outside of specific pull requests, etc. I'll actually be working this week on finishing implementing a TOML configuration file that will allow for custom mappings for what's a test file and allow for more normalized ingestion of JSON files with per-file info, plus support for custom templating to augment the rendered HTML as an evolution of the current test info / bugzilla mapping / coverage data ingestion mechanism we use for mozilla-central.

Copy link
Contributor Author

@dpino dpino Aug 22, 2022

Choose a reason for hiding this comment

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

Created bug and opened PR to fix the aptIfExists error and update system library dependencies #3525

@dpino dpino deleted the feat/switch-ubuntu-lts-to-22.04 branch September 20, 2022 08:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants