Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2645523003:
Build ozone/linux_os with jessie sysroot when use_ozone=true (Closed)
DescriptionBuild ozone/linux_os with jessie sysroot when use_ozone=true
This is needed because Debian/wheezy (currently used) does not support
libxkbcommon0/libxkbcommon-dev packages, needed for keyboard support on
Ozone/Wayland.
BUG=681562
Review-Url: https://codereview.chromium.org/2645523003
Cr-Commit-Position: refs/heads/master@{#446081}
Committed: https://chromium.googlesource.com/chromium/src/+/58f836955cb7ca30cffc60a95bc337a5b6fa092c
Patch Set 1 #
Total comments: 4
Patch Set 2 : addressed tom's feedback #
Total comments: 2
Patch Set 3 : addressed dpranke's feedback #
Total comments: 1
Patch Set 4 : addressed moar dpranke's feedback #
Total comments: 3
Patch Set 5 : excluding chromecast builds #Messages
Total messages: 38 (14 generated)
Description was changed from ========== [NOT FOR REVIEW] Build ozone/chromeos with jessie sysroot Build Ozone/ChromeOS builds with Debian/jessie sysroot. This is needed because Ubuntu/precise (currently used), does not support libxkbcommon0/libxkbcommon-dev packages, needed for input support on Ozone/Wayland. BUG=681562 ========== to ========== [NOT FOR REVIEW] Build ozone/chromeos with jessie sysroot Build Ozone/ChromeOS builds with Debian/jessie sysroot. This is needed because Ubuntu/precise (currently used), does not support libxkbcommon0/libxkbcommon-dev packages, needed for input support on Ozone/Wayland. BUG=681562 ==========
tonikitoo@igalia.com changed reviewers: + thomasanderson@chromium.org
So Tom. ChromeOS/desktop builds still use Ubuntu/precise (12.04?!) as seeing in build/config/sysroot.gni. Also, in build/linux/sysroot_scripts/install-sysroot.py, precise sysroot is downloaded without any checks for this purpose. This CL is a request for feedback, since it actually changes build/linux/sysroot_scripts/install-sysroot.py to download debian/jessie-amd64 sysroot for all users, apart from ubuntu/precise. Per si, this imposes +- 145Mb of download on "gclient sync" for instance, which is not something to be ignored. Possible solutions: - go with it, and assume that ubuntu/precise requiment for ChromeOS/Desktop builds is going to be dropped, so we can only need debian/jessie - debian/wheezy is still the de-facto sysroot for building on amd64. There is a CL to move it on to debian/jessie (https://codereview.chromium.org/2361223002) , but it seems staled. - add libxkbcommon packages to either ubuntu/precise or debian/wheezy sysroots, and use it for building chromeos/ozone/desktop. wdyt?
On 2017/01/18 13:03:09, tonikitoo wrote: > So Tom. ChromeOS/desktop builds still use Ubuntu/precise (12.04?!) as seeing in > build/config/sysroot.gni. Also, in > build/linux/sysroot_scripts/install-sysroot.py, precise sysroot is downloaded > without any checks for this purpose. > > This CL is a request for feedback, since it actually changes > build/linux/sysroot_scripts/install-sysroot.py to download debian/jessie-amd64 > sysroot for all users, apart from ubuntu/precise. > > Per si, this imposes +- 145Mb of download on "gclient sync" for instance, which > is not something to be ignored. > > Possible solutions: > > - go with it, and assume that ubuntu/precise requiment for ChromeOS/Desktop > builds is going to be dropped, so we can only need debian/jessie > > - debian/wheezy is still the de-facto sysroot for building on amd64. There is a > CL to move it on to debian/jessie (https://codereview.chromium.org/2361223002) , > but it seems staled. > > - add libxkbcommon packages to either ubuntu/precise or debian/wheezy sysroots, > and use it for building chromeos/ozone/desktop. > > wdyt? another option, which actually goes together with the first option above, is to replace ubuntu/precise by debian/jessie for chromeos/desktop builds (being it ozone or not).
thomasanderson@google.com changed reviewers: + thomasanderson@google.com
I should give you some background about what we have planned with the sysroots. We're currently upgrading our infrastructure so all of the trybots are on Trusty instead of Precise (I know, it's long overdue!). Once this finally happens, we'll be able to remove the Precise and Wheezy sysroots and use the Jessie sysroot instead. However, since there are no tests run by our compile-only ozone bot, this doesn't matter for us. We only need to test the build which should succeed using the Jessie sysroot, and should run on our dev machines. https://codereview.chromium.org/2645523003/diff/1/build/config/sysroot.gni File build/config/sysroot.gni (right): https://codereview.chromium.org/2645523003/diff/1/build/config/sysroot.gni#ne... build/config/sysroot.gni:51: import("//ui/ozone/ozone.gni") Move the added code into the else block (!is_chromeos), and keep the chromeos build always using precise. I can add libxkbcommon* to the precise sysroot since those packages are actually available: http://packages.ubuntu.com/search?suite=precise&searchon=names&keywords=libxk... https://codereview.chromium.org/2645523003/diff/1/build/linux/sysroot_scripts... File build/linux/sysroot_scripts/install-sysroot.py (right): https://codereview.chromium.org/2645523003/diff/1/build/linux/sysroot_scripts... build/linux/sysroot_scripts/install-sysroot.py:129: InstallSysroot('Jessie', 'amd64') remove this
I should give you some background about what we have planned with the sysroots. We're currently upgrading our infrastructure so all of the trybots are on Trusty instead of Precise (I know, it's long overdue!). Once this finally happens, we'll be able to remove the Precise and Wheezy sysroots and use the Jessie sysroot instead. However, since there are no tests run by our compile-only ozone bot, this doesn't matter for us. We only need to test the build which should succeed using the Jessie sysroot, and should run on our dev machines.
Description was changed from
==========
[NOT FOR REVIEW] Build ozone/chromeos with jessie sysroot
Build Ozone/ChromeOS builds with Debian/jessie sysroot. This is
needed because Ubuntu/precise (currently used), does not support
libxkbcommon0/libxkbcommon-dev packages, needed for input support
on Ozone/Wayland.
BUG=681562
==========
to
==========
Build ozone/linux_os with jessie sysroot when use_xkbcommon=true
This is needed because Debian/wheezy (currently used) does not support
libxkbcommon0/libxkbcommon-dev packages, needed for keyboard support on
Ozone/Wayland.
BUG=681562
==========
Addressed both comments, Tom. do we need a way to enforce pulling debian/jessie sysroot for amd64/ozone/linuxos builds on the bots? https://codereview.chromium.org/2645523003/diff/1/build/config/sysroot.gni File build/config/sysroot.gni (right): https://codereview.chromium.org/2645523003/diff/1/build/config/sysroot.gni#ne... build/config/sysroot.gni:51: import("//ui/ozone/ozone.gni") On 2017/01/18 19:02:18, Tom Anderson wrote: > Move the added code into the else block (!is_chromeos), and keep the chromeos > build always using precise. > > I can add libxkbcommon* to the precise sysroot since those packages are actually > available: > http://packages.ubuntu.com/search?suite=precise&searchon=names&keywords=libxk... Done. https://codereview.chromium.org/2645523003/diff/1/build/linux/sysroot_scripts... File build/linux/sysroot_scripts/install-sysroot.py (right): https://codereview.chromium.org/2645523003/diff/1/build/linux/sysroot_scripts... build/linux/sysroot_scripts/install-sysroot.py:129: InstallSysroot('Jessie', 'amd64') On 2017/01/18 19:02:18, Tom Anderson wrote: > remove this Done.
On 2017/01/18 19:02:19, Tom Anderson wrote: > I should give you some background about what we have planned with the sysroots. > (..) > However, since there are no tests run by our compile-only ozone bot, this > doesn't matter for us. We only need to test the build which should succeed > using the Jessie sysroot, and should run on our dev machines. (Let me rephrase my question in comment #9) On my machine, I run: $ ./build/linux/sysroot_scripts/install-sysroot.py --arch amd64 Debian Wheezy amd64 root image already up to date: /<path>/src/build/linux/debian_wheezy_amd64-sysroot I can tweak the script to pull the jessie sysroot instead, but how would the bot do it?
thomasanderson@google.com changed reviewers: + dpranke@chromium.org
thanks this lgtm. You'll also need dpranke@'s approval I'll be making the necessary changes in //build and install-sysroot.py today
https://codereview.chromium.org/2645523003/diff/20001/build/config/sysroot.gni File build/config/sysroot.gni (right): https://codereview.chromium.org/2645523003/diff/20001/build/config/sysroot.gn... build/config/sysroot.gni:61: import("//ui/base/ui_features.gni") The //build files are used in other repos, so we can't just directly depend on //ui. You need to guard this under the "build_with_chromium" flag in //build_overrides/build.gni.
https://codereview.chromium.org/2645523003/diff/20001/build/config/sysroot.gni File build/config/sysroot.gni (right): https://codereview.chromium.org/2645523003/diff/20001/build/config/sysroot.gn... build/config/sysroot.gni:61: import("//ui/base/ui_features.gni") On 2017/01/23 21:59:48, Dirk Pranke wrote: > The //build files are used in other repos, so we can't just directly depend on > //ui. > > You need to guard this under the "build_with_chromium" flag in > //build_overrides/build.gni. > Done. I actually removed the check for use_xkbcommon, since thomasanderson's CL landed with it it (https://codereview.chromium.org/2645733006).
On 2017/01/24 03:28:03, tonikitoo wrote: > https://codereview.chromium.org/2645523003/diff/20001/build/config/sysroot.gni > File build/config/sysroot.gni (right): > > https://codereview.chromium.org/2645523003/diff/20001/build/config/sysroot.gn... > build/config/sysroot.gni:61: import("//ui/base/ui_features.gni") > On 2017/01/23 21:59:48, Dirk Pranke wrote: > > The //build files are used in other repos, so we can't just directly depend on > > //ui. > > > > You need to guard this under the "build_with_chromium" flag in > > //build_overrides/build.gni. > > > > Done. I actually removed the check for use_xkbcommon, since thomasanderson's CL > landed with it it (https://codereview.chromium.org/2645733006). without it*
Description was changed from
==========
Build ozone/linux_os with jessie sysroot when use_xkbcommon=true
This is needed because Debian/wheezy (currently used) does not support
libxkbcommon0/libxkbcommon-dev packages, needed for keyboard support on
Ozone/Wayland.
BUG=681562
==========
to
==========
Build ozone/linux_os with jessie sysroot when use_ozone=true
This is needed because Debian/wheezy (currently used) does not support
libxkbcommon0/libxkbcommon-dev packages, needed for keyboard support on
Ozone/Wayland.
BUG=681562
==========
Friendly ping :-)
sorry for the delay, I'll try to be faster on the next cycle. https://codereview.chromium.org/2645523003/diff/40001/build/config/sysroot.gni File build/config/sysroot.gni (right): https://codereview.chromium.org/2645523003/diff/40001/build/config/sysroot.gn... build/config/sysroot.gni:62: if (current_cpu == "x64" && use_ozone && build_with_chromium) { You need to move the second import() statement inside the if (build_with_chromium) clause, otherwise that defeats the point. The //ui file won't exist in a non-chromium checkout.
On 2017/01/24 22:49:21, Dirk Pranke wrote: > sorry for the delay, I'll try to be faster on the next cycle. > > https://codereview.chromium.org/2645523003/diff/40001/build/config/sysroot.gni > File build/config/sysroot.gni (right): > > https://codereview.chromium.org/2645523003/diff/40001/build/config/sysroot.gn... > build/config/sysroot.gni:62: if (current_cpu == "x64" && use_ozone && > build_with_chromium) { > You need to move the second import() statement inside the if > (build_with_chromium) clause, otherwise that defeats the point. The //ui file > won't exist in a non-chromium checkout. Done.
https://codereview.chromium.org/2645523003/diff/60001/build/config/sysroot.gni File build/config/sysroot.gni (right): https://codereview.chromium.org/2645523003/diff/60001/build/config/sysroot.gn... build/config/sysroot.gni:69: } you need else clauses here for !build_with_chromium and !use_ozone, right? :)
https://codereview.chromium.org/2645523003/diff/60001/build/config/sysroot.gni File build/config/sysroot.gni (right): https://codereview.chromium.org/2645523003/diff/60001/build/config/sysroot.gn... build/config/sysroot.gni:69: } On 2017/01/25 00:46:18, Dirk Pranke wrote: > you need else clauses here for !build_with_chromium and !use_ozone, right? :) I did it so that line 59 actually sets the default wheezy sysroot before hand. Later we only override 'sysroot' in case of build_with_chromium && use_ozone. wdyt?
lgtm. https://codereview.chromium.org/2645523003/diff/60001/build/config/sysroot.gni File build/config/sysroot.gni (right): https://codereview.chromium.org/2645523003/diff/60001/build/config/sysroot.gn... build/config/sysroot.gni:69: } On 2017/01/25 01:09:03, tonikitoo wrote: > On 2017/01/25 00:46:18, Dirk Pranke wrote: > > you need else clauses here for !build_with_chromium and !use_ozone, right? :) > > I did it so that line 59 actually sets the default wheezy sysroot before hand. > Later we only override 'sysroot' in case of build_with_chromium && use_ozone. > > wdyt? > Oh, I see. Right, sorry.
The CQ bit was checked by tonikitoo@igalia.com
The patchset sent to the CQ was uploaded after l-g-t-m from thomasanderson@google.com Link to the patchset: https://codereview.chromium.org/2645523003/#ps60001 (title: "addressed moar dpranke's feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by tonikitoo@igalia.com
The patchset sent to the CQ was uploaded after l-g-t-m from thomasanderson@google.com, dpranke@chromium.org Link to the patchset: https://codereview.chromium.org/2645523003/#ps80001 (title: "excluding chromecast builds")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1485362826179220,
"parent_rev": "8499f3a55d732d50c2a743fc3a499c43cc086234", "commit_rev":
"58f836955cb7ca30cffc60a95bc337a5b6fa092c"}
Message was sent while issue was closed.
Description was changed from
==========
Build ozone/linux_os with jessie sysroot when use_ozone=true
This is needed because Debian/wheezy (currently used) does not support
libxkbcommon0/libxkbcommon-dev packages, needed for keyboard support on
Ozone/Wayland.
BUG=681562
==========
to
==========
Build ozone/linux_os with jessie sysroot when use_ozone=true
This is needed because Debian/wheezy (currently used) does not support
libxkbcommon0/libxkbcommon-dev packages, needed for keyboard support on
Ozone/Wayland.
BUG=681562
Review-Url: https://codereview.chromium.org/2645523003
Cr-Commit-Position: refs/heads/master@{#446081}
Committed:
https://chromium.googlesource.com/chromium/src/+/58f836955cb7ca30cffc60a95bc3...
==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/58f836955cb7ca30cffc60a95bc3...
Message was sent while issue was closed.
brettw@chromium.org changed reviewers: + brettw@chromium.org
Message was sent while issue was closed.
I really don't think that build should be depending on non-build directories. Putting things behind flags just hides the layering violation. In source code, we wouldn't allow //base to depend on //chrome, even in circumstances where //chrome is guaranteed to exist. What other designs were considered?
Message was sent while issue was closed.
On 2017/01/26 22:48:36, brettw (ping after 24h) wrote: > I really don't think that build should be depending on non-build directories. > Putting things behind flags just hides the layering violation. In source code, > we wouldn't allow //base to depend on //chrome, even in circumstances where > //chrome is guaranteed to exist. > > What other designs were considered? You are correct that we shouldn't have this. It is a temporary thing that will go away when we can upgrade the remaining Precise builders to Trusty, so I think this is an acceptable tradeoff. The only obvious alternative at the moment is to add a separate build flag that conveys the exact same concept as "use_ozone" but is defined in a file in //build.
Message was sent while issue was closed.
On 2017/01/26 22:51:42, Dirk Pranke wrote: > On 2017/01/26 22:48:36, brettw (ping after 24h) wrote: > > I really don't think that build should be depending on non-build directories. > > Putting things behind flags just hides the layering violation. In source code, > > we wouldn't allow //base to depend on //chrome, even in circumstances where > > //chrome is guaranteed to exist. > > > > What other designs were considered? > > You are correct that we shouldn't have this. It is a temporary thing that will > go away > when we can upgrade the remaining Precise builders to Trusty, so I think this is > an acceptable tradeoff. > > The only obvious alternative at the moment is to add a separate build flag that > conveys the exact same concept as "use_ozone" but is defined in a file in > //build. Hi brettw. Dirk is right, and this is a provisional, non-ideal set up. I will be personally watching it, so that I can clean it up. Can I get cc'ed to https://bugs.chromium.org/p/chromium/issues/detail?id=666825 please (It tracks the wheezy -> jessie sysroot upgrade, but seems restricted for non-googlers).
Message was sent while issue was closed.
On 2017/01/27 13:53:25, tonikitoo wrote: > Can I get cc'ed to https://bugs.chromium.org/p/chromium/issues/detail?id=666825 > please (It tracks the wheezy -> jessie sysroot upgrade, but seems restricted for > non-googlers). Unfortunately, no :(. That bug contains a bunch of discussion of upgrading Google-internal systems as well. But, don't worry, we'll let you know when enough things are upgraded and it's safe to remove this. |
