Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1434453002:
Improve install-build-deps mesa handling. (Closed)
DescriptionImprove install-build-deps mesa handling.
Instead of hardcoding a list of which LTS variants of mesa might be
installed, just query for any package that matches the expected pattern
and if exactly one is installed, use that. If zero are installed, use
the base package. There shouldn't be more than one as these packages
conflict with each other, so just abort if that happens.
This avoids needing to extend the loop in this script every time a new
Ubuntu backport package is added.
BUG=
Committed: https://crrev.com/8a6eb6958e85d742f70a8cc4c85a24c5efa93cee
Cr-Commit-Position: refs/heads/master@{#358039}
Patch Set 1 #Patch Set 2 : Replace list with a better approach #
Total comments: 2
Patch Set 3 : fix array length count #Messages
Total messages: 31 (6 generated)
Description was changed from ========== Check for Vivid LTS packages in install-build-deps. Some Trusty machines have the -lts-vivid variant of mesa/etc installed, which isn't currently checked for in the script and causes package installation to fail. Extend the list to include this. BUG= ========== to ========== Improve install-build-deps mesa handling. Instead of hardcoding a list of which LTS variants of mesa might be installed, just query for any package that matches the expected pattern and if exactly one is installed, use that. If zero are installed, use the base package. There shouldn't be more than one as these packages conflict with each other, so just abort if that happens. This avoids needing to extend the loop in this script every time a new Ubuntu backport package is added. BUG= ==========
torne@chromium.org changed reviewers: + primiano@chromium.org
Seems like some people have -lts-vivid installed now. Rather than update this list, I came up with this, what do you think?
torne@chromium.org changed reviewers: + thestig@chromium.org
also +thestig who also has messed with this if i recall.
Tried it on my machine (Thinkpad X1 Carbon Ubuntu 14.04 LTS) but see this: ERROR: unable to determine which libgl1-mesa-glx variant is installed.
On 2015/11/04 11:57:28, henrika wrote:
> Tried it on my machine (Thinkpad X1 Carbon Ubuntu 14.04 LTS) but see this:
>
> ERROR: unable to determine which libgl1-mesa-glx variant is installed.
Can you modify it to print ${mesa_packages[@]} and see what's in there?
misch@google.com changed reviewers: + misch@google.com
https://codereview.chromium.org/1434453002/diff/20001/build/install-build-dep... File build/install-build-deps.sh (right): https://codereview.chromium.org/1434453002/diff/20001/build/install-build-dep... build/install-build-deps.sh:185: libgl1-mesa-glx-lts-\* | \ You have to filter out packages with the i386 arch here.
On 2015/11/04 12:22:07, misch wrote: > https://codereview.chromium.org/1434453002/diff/20001/build/install-build-dep... > File build/install-build-deps.sh (right): > > https://codereview.chromium.org/1434453002/diff/20001/build/install-build-dep... > build/install-build-deps.sh:185: libgl1-mesa-glx-lts-\* | \ > You have to filter out packages with the i386 arch here. We're going to install packages based on this for both arm64 and i386, though, and currently it's assumed those will be the same. If they can be different then I'll have to do this twice, once for each. :/
You want to identify the mesa_variant as you call it. For this you only need to list the installed mesa packages of the system architecture. With the current code you also get packages of a foreign arch in the list and hence some people have more than one mesa installed. On Wed, Nov 4, 2015 at 1:27 PM <torne@chromium.org> wrote: > On 2015/11/04 12:22:07, misch wrote: > > > https://codereview.chromium.org/1434453002/diff/20001/build/install-build-dep... > > File build/install-build-deps.sh (right): > > > > https://codereview.chromium.org/1434453002/diff/20001/build/install-build-dep... > > build/install-build-deps.sh:185: libgl1-mesa-glx-lts-\* | \ > > You have to filter out packages with the i386 arch here. > > We're going to install packages based on this for both arm64 and i386, > though, > and currently it's assumed those will be the same. If they can be different > then > I'll have to do this twice, once for each. :/ > > https://codereview.chromium.org/1434453002/ > -- *Michael SchallerSystems Administratormisch@google.com <misch@google.com>Google Germany GmbHDienerstraße 1280331 MünchenGeschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle* *Registergericht und -nummer: Hamburg, HRB 86891Sitz der Gesellschaft: HamburgDiese E-Mail ist vertraulich. Wenn Sie nicht der richtige Adressat sind, leiten Sie diese bitte nicht weiter, informieren Sie den Absender und löschen Sie die E-Mail und alle Anhänge. Vielen Dank. This e-mail is confidential. If you are not the right addressee please do not forward it, please inform the sender, and please erase this e-mail including any attachments. Thanks.* To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/11/04 12:34:18, chromium-reviews wrote: > You want to identify the mesa_variant as you call it. For this you only > need to list the installed mesa packages of the system architecture. > With the current code you also get packages of a foreign arch in the list > and hence some people have more than one mesa installed. No, we also need to determine the installed mesa packages for i386 as well, so that we know which packages to install when we are installing packages for i386. On my machine, both architectures have the same variant installed, and the script dedupes them. If it's okay/expected for there to be different ones installed on different architectures then I need to do this whole thing twice, once for each architecture, and set two different variables to be used in different places :/
I print out ${mesa_packages[@] and see:
libgl1-mesa-glx-lts-vivid
On 2015/11/04 12:36:25, Torne wrote: > On 2015/11/04 12:34:18, chromium-reviews wrote: > > You want to identify the mesa_variant as you call it. For this you only > > need to list the installed mesa packages of the system architecture. > > With the current code you also get packages of a foreign arch in the list > > and hence some people have more than one mesa installed. > > No, we also need to determine the installed mesa packages for i386 as well, so > that we know which packages to install when we are installing packages for i386. > On my machine, both architectures have the same variant installed, and the > script dedupes them. If it's okay/expected for there to be different ones > installed on different architectures then I need to do this whole thing twice, > once for each architecture, and set two different variables to be used in > different places :/ The point is that you can only install one mesa version and if you install mesa packages of a foreign architecture then these packages must match the installed mesa version. If you try something else then Apt has to fail. So in order to identify the installed mesa variant you just need to look at the installed mesa packages of the system architecture.
On 2015/11/04 12:42:40, henrika wrote:
> I print out ${mesa_packages[@] and see:
>
> libgl1-mesa-glx-lts-vivid
That doesn't make sense, then - there's only one entry in the list :/
Let me think some more.
On 2015/11/04 12:45:00, misch wrote: > On 2015/11/04 12:36:25, Torne wrote: > > On 2015/11/04 12:34:18, chromium-reviews wrote: > > > You want to identify the mesa_variant as you call it. For this you only > > > need to list the installed mesa packages of the system architecture. > > > With the current code you also get packages of a foreign arch in the list > > > and hence some people have more than one mesa installed. > > > > No, we also need to determine the installed mesa packages for i386 as well, so > > that we know which packages to install when we are installing packages for > i386. > > On my machine, both architectures have the same variant installed, and the > > script dedupes them. If it's okay/expected for there to be different ones > > installed on different architectures then I need to do this whole thing twice, > > once for each architecture, and set two different variables to be used in > > different places :/ > > The point is that you can only install one mesa version and if you install mesa > packages of a foreign architecture then these packages must match the installed > mesa version. If you try something else then Apt has to fail. So in order to > identify the installed mesa variant you just need to look at the installed mesa > packages of the system architecture. If you can only install one mesa version regardless of architecture then I don't see how what I'm doing now is wrong (also see henrika's response: they only have one package in the list).
On 2015/11/04 12:48:14, Torne wrote:
> On 2015/11/04 12:45:00, misch wrote:
> > On 2015/11/04 12:36:25, Torne wrote:
> > > On 2015/11/04 12:34:18, chromium-reviews wrote:
> > > > You want to identify the mesa_variant as you call it. For this you only
> > > > need to list the installed mesa packages of the system architecture.
> > > > With the current code you also get packages of a foreign arch in the
list
> > > > and hence some people have more than one mesa installed.
> > >
> > > No, we also need to determine the installed mesa packages for i386 as
well,
> so
> > > that we know which packages to install when we are installing packages for
> > i386.
> > > On my machine, both architectures have the same variant installed, and the
> > > script dedupes them. If it's okay/expected for there to be different ones
> > > installed on different architectures then I need to do this whole thing
> twice,
> > > once for each architecture, and set two different variables to be used in
> > > different places :/
> >
> > The point is that you can only install one mesa version and if you install
> mesa
> > packages of a foreign architecture then these packages must match the
> installed
> > mesa version. If you try something else then Apt has to fail. So in order to
> > identify the installed mesa variant you just need to look at the installed
> mesa
> > packages of the system architecture.
>
> If you can only install one mesa version regardless of architecture then I
don't
> see how what I'm doing now is wrong (also see henrika's response: they only
have
> one package in the list).
I've tested the code and you are correct that this isn't the issue. The issue is
that mesa_packages isn't an array but an ordinary string. Because of that
"${#mesa_packages}" doesn't return the number of array items but rather the
number of characters in the string.
https://codereview.chromium.org/1434453002/diff/20001/build/install-build-dep... File build/install-build-deps.sh (right): https://codereview.chromium.org/1434453002/diff/20001/build/install-build-dep... build/install-build-deps.sh:185: libgl1-mesa-glx-lts-\* | \ On 2015/11/04 12:22:06, misch wrote: > You have to filter out packages with the i386 arch here. Done.
On 2015/11/04 13:42:36, misch wrote:
> On 2015/11/04 12:48:14, Torne wrote:
> > On 2015/11/04 12:45:00, misch wrote:
> > > On 2015/11/04 12:36:25, Torne wrote:
> > > > On 2015/11/04 12:34:18, chromium-reviews wrote:
> > > > > You want to identify the mesa_variant as you call it. For this you
only
> > > > > need to list the installed mesa packages of the system architecture.
> > > > > With the current code you also get packages of a foreign arch in the
> list
> > > > > and hence some people have more than one mesa installed.
> > > >
> > > > No, we also need to determine the installed mesa packages for i386 as
> well,
> > so
> > > > that we know which packages to install when we are installing packages
for
> > > i386.
> > > > On my machine, both architectures have the same variant installed, and
the
> > > > script dedupes them. If it's okay/expected for there to be different
ones
> > > > installed on different architectures then I need to do this whole thing
> > twice,
> > > > once for each architecture, and set two different variables to be used
in
> > > > different places :/
> > >
> > > The point is that you can only install one mesa version and if you install
> > mesa
> > > packages of a foreign architecture then these packages must match the
> > installed
> > > mesa version. If you try something else then Apt has to fail. So in order
to
> > > identify the installed mesa variant you just need to look at the installed
> > mesa
> > > packages of the system architecture.
> >
> > If you can only install one mesa version regardless of architecture then I
> don't
> > see how what I'm doing now is wrong (also see henrika's response: they only
> have
> > one package in the list).
>
> I've tested the code and you are correct that this isn't the issue. The issue
is
> that mesa_packages isn't an array but an ordinary string. Because of that
> "${#mesa_packages}" doesn't return the number of array items but rather the
> number of characters in the string.
No, it is an array, the issue is that i meant ${#mesa_packages[@]} :)
Will fix it in a bit.
On 2015/11/04 13:45:45, Torne wrote:
> On 2015/11/04 13:42:36, misch wrote:
> > On 2015/11/04 12:48:14, Torne wrote:
> > > On 2015/11/04 12:45:00, misch wrote:
> > > > On 2015/11/04 12:36:25, Torne wrote:
> > > > > On 2015/11/04 12:34:18, chromium-reviews wrote:
> > > > > > You want to identify the mesa_variant as you call it. For this you
> only
> > > > > > need to list the installed mesa packages of the system architecture.
> > > > > > With the current code you also get packages of a foreign arch in the
> > list
> > > > > > and hence some people have more than one mesa installed.
> > > > >
> > > > > No, we also need to determine the installed mesa packages for i386 as
> > well,
> > > so
> > > > > that we know which packages to install when we are installing packages
> for
> > > > i386.
> > > > > On my machine, both architectures have the same variant installed, and
> the
> > > > > script dedupes them. If it's okay/expected for there to be different
> ones
> > > > > installed on different architectures then I need to do this whole
thing
> > > twice,
> > > > > once for each architecture, and set two different variables to be used
> in
> > > > > different places :/
> > > >
> > > > The point is that you can only install one mesa version and if you
install
> > > mesa
> > > > packages of a foreign architecture then these packages must match the
> > > installed
> > > > mesa version. If you try something else then Apt has to fail. So in
order
> to
> > > > identify the installed mesa variant you just need to look at the
installed
> > > mesa
> > > > packages of the system architecture.
> > >
> > > If you can only install one mesa version regardless of architecture then I
> > don't
> > > see how what I'm doing now is wrong (also see henrika's response: they
only
> > have
> > > one package in the list).
> >
> > I've tested the code and you are correct that this isn't the issue. The
issue
> is
> > that mesa_packages isn't an array but an ordinary string. Because of that
> > "${#mesa_packages}" doesn't return the number of array items but rather the
> > number of characters in the string.
>
> No, it is an array, the issue is that i meant ${#mesa_packages[@]} :)
> Will fix it in a bit.
Ah. I've missed the extra pair of brackets. :-/
This one liner worked for me:
mesa_packages=($(dpkg-query -Wf '${package} ${status}\n' libgl1-mesa-glx-lts-\*
2>/dev/null | grep "install ok installed" | cut -d " " -f 1 | sort -u)) ; echo
"package count: ${#mesa_packages[@]}" ; echo "packages: ${mesa_packages[@]}"
OK, fixed that now. misch@, can you confirm the script now works for you?
Works for me now. Many thanks. Great job ;-)
Not sure if is related but after running the new script (works), I can't do
client runhooks on my machine.
Just checking if more parts in the script needs modifications.
Updating projects from gyp files...
/bin/sh: c++: command not found
compiler_version.py failed to execute: c++ -Xassembler --version -x assembler -c
/dev/null
Command 'c++ -Xassembler --version -x assembler -c /dev/null' returned non-zero
exit status 127
Exception: Failed to extract compiler version for args: ['target', 'assembler']
Traceback (most recent call last):
File "/home/henrika/src/chromium/src/tools/gyp/pylib/gyp/input.py", line 496,
in CallLoadTargetBuildFile
includes, depth, check, False)
File "/home/henrika/src/chromium/src/tools/gyp/pylib/gyp/input.py", line 417,
in LoadTargetBuildFile
build_file_data, PHASE_EARLY, variables, build_file_path)
File "/home/henrika/src/chromium/src/tools/gyp/pylib/gyp/input.py", line 1214,
in ProcessVariablesAndConditionsInDict
variables, build_file, 'variables')
File "/home/henrika/src/chromium/src/tools/gyp/pylib/gyp/input.py", line 1266,
in ProcessVariablesAndConditionsInDict
ProcessConditionsInDict(the_dict, phase, variables, build_file)
File "/home/henrika/src/chromium/src/tools/gyp/pylib/gyp/input.py", line 1145,
in ProcessConditionsInDict
variables, build_file)
File "/home/henrika/src/chromium/src/tools/gyp/pylib/gyp/input.py", line 1266,
in ProcessVariablesAndConditionsInDict
ProcessConditionsInDict(the_dict, phase, variables, build_file)
File "/home/henrika/src/chromium/src/tools/gyp/pylib/gyp/input.py", line 1145,
in ProcessConditionsInDict
variables, build_file)
File "/home/henrika/src/chromium/src/tools/gyp/pylib/gyp/input.py", line 1221,
in ProcessVariablesAndConditionsInDict
expanded = ExpandVariables(value, phase, variables, build_file)
File "/home/henrika/src/chromium/src/tools/gyp/pylib/gyp/input.py", line 891,
in ExpandVariables
replacement = str(py_module.DoMain(parsed_contents[1:])).rstrip()
File "/home/henrika/src/chromium/src/build/compiler_version.py", line 122, in
DoMain
raise Exception("Failed to extract compiler version for args: %s" % args)
Exception: Failed to extract compiler version for args: ['target', 'assembler']
torne@chromium.org changed reviewers: + dpranke@chromium.org, thakis@chromium.org
Oh, just realised that build actually has OWNERS now. So, er, +dpranke I guess. ;)
lgtm
The CQ bit was checked by torne@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1434453002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1434453002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/8a6eb6958e85d742f70a8cc4c85a24c5efa93cee Cr-Commit-Position: refs/heads/master@{#358039} |
