1370535 - "ac_add_options --enable-geckodriver" breaks artifact builds

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Using "ac_add_options --enable-geckodriver" in the mozconfig breaks artifact builds in a way that no-one can determine what's wrong:

 0:21.41 Traceback (most recent call last):
 0:21.41   File "/Volumes/data/code/gecko/configure.py", line 124, in <module>
 0:21.41     sys.exit(main(sys.argv))
 0:21.41   File "/Volumes/data/code/gecko/configure.py", line 34, in main
 0:21.41     return config_status(config)
 0:21.41   File "/Volumes/data/code/gecko/configure.py", line 119, in config_status
 0:21.41     return config_status(args=[], **encode(sanitized_config, encoding))
 0:21.41   File "/Volumes/data/code/gecko/python/mozbuild/mozbuild/config_status.py", line 150, in config_status
 0:21.41     the_backend.consume(definitions)
 0:21.41   File "/Volumes/data/code/gecko/python/mozbuild/mozbuild/backend/base.py", line 126, in consume
 0:21.41     for obj in objs:
 0:21.41   File "/Volumes/data/code/gecko/python/mozbuild/mozbuild/frontend/emitter.py", line 183, in emit
 0:21.41     objs = list(emitfn(out))
 0:21.41   File "/Volumes/data/code/gecko/python/mozbuild/mozbuild/frontend/emitter.py", line 1026, in emit_from_context
 0:21.41     for obj in self._handle_linkables(context, passthru, generated_files):
 0:21.41   File "/Volumes/data/code/gecko/python/mozbuild/mozbuild/frontend/emitter.py", line 563, in _handle_linkables
 0:21.41     self._binaries[program] = cls(context, program, cargo_file)
 0:21.41   File "/Volumes/data/code/gecko/python/mozbuild/mozbuild/frontend/data.py", line 449, in __init__
 0:21.41     cargo_dir = cargo_output_directory(context, self.TARGET_SUBST_VAR)
 0:21.41   File "/Volumes/data/code/gecko/python/mozbuild/mozbuild/frontend/data.py", line 430, in cargo_output_directory
 0:21.41     return mozpath.join(context.config.substs[target_var], rust_build_kind)
 0:21.41 KeyError: u'RUST_TARGET'
 0:21.72 *** Fix above errors and then restart with               "/Applications/Xcode.app/Contents/Developer/usr/bin/make -f client.mk build"
 0:21.72 make[2]: *** [configure] Error 1
 0:21.73 make[1]: *** [/Volumes/data/code/gecko/obj/debug/Makefile] Error 2
 0:21.73 make: *** [build] Error 2

If we do not support artifact builds with geckodriver enabled, we should at least make it clear with a clean failure message.

Comment 1

[Nathan Froyd [:froydnj]]

RustLibrary in data.py has an explicit check for COMPILE_ENVIRONMENT; it may be worth investigating if a similar check would make sense for BaseRustProgram.  Might have to check to see if anything uses the paths defined in BaseRustProgram.

Comment 2

[Andreas Tolfsen ❲:ato❳]

(In reply to Nathan Froyd [:froydnj] from comment #1)
> RustLibrary in data.py has an explicit check for COMPILE_ENVIRONMENT; it may
> be worth investigating if a similar check would make sense for
> BaseRustProgram.  Might have to check to see if anything uses the paths
> defined in BaseRustProgram.

Making a similar check for BaseRustProgram certainly makes the KeyError:
u'RUST_TARGET' to away:

> diff --git a/python/mozbuild/mozbuild/frontend/data.py b/python/mozbuild/mozbuild/frontend/data.py
> index f9328a426ddc..1ea8f75327c6 100644
> --- a/python/mozbuild/mozbuild/frontend/data.py
> +++ b/python/mozbuild/mozbuild/frontend/data.py
> @@ -445,10 +445,12 @@ class BaseRustProgram(ContextDerived):
>      def __init__(self, context, name, cargo_file):
>          ContextDerived.__init__(self, context)
>          self.name = name
> -        self.cargo_file = cargo_file
> -        cargo_dir = cargo_output_directory(context, self.TARGET_SUBST_VAR)
> -        exe_file = '%s%s' % (name, context.config.substs.get(self.SUFFIX_VAR, ''))
> -        self.location = mozpath.join(cargo_dir, exe_file)
> +
> +        if context.config.substs.get('COMPILE_ENVIRONMENT'):
> +            self.cargo_file = cargo_file
> +            cargo_dir = cargo_output_directory(context, self.TARGET_SUBST_VAR)
> +            exe_file = '%s%s' % (name, context.config.substs.get(self.SUFFIX_VAR, ''))
> +            self.location = mozpath.join(cargo_dir, exe_file)
>  
>  
>  class RustProgram(BaseRustProgram):

However, I find it is confusing that no geckodriver binary or error is
emitted when --enable-geckodriver is switched on for artifact builds.  What
is the approach we have taken in similar cases in the past?

Should it maybe return an error, and if so where, when asked to build
geckodriver in a non-compilation environment?

Comment 3

[Nathan Froyd [:froydnj]]

(In reply to Andreas Tolfsen ‹:ato› from comment #2)
> However, I find it is confusing that no geckodriver binary or error is
> emitted when --enable-geckodriver is switched on for artifact builds.  What
> is the approach we have taken in similar cases in the past?

I don't think we have prior art here, because --enable-geckodriver is a little unusual in building a specific program.  But --enable-artifact builds are designed to require zero compilation, so not producing geckodriver is expected.

> Should it maybe return an error, and if so where, when asked to build
> geckodriver in a non-compilation environment?

If --enable-geckodriver also toggles prefs or other settings, I think that means --enable-geckodriver should be an error with artifact builds.  Unless we could make --enable-geckodriver artifact builds look for artifacts from a normal --enable-geckodriver build?  Chris would be the person to ask about that.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Maybe we could have something like `mach build geckodriver` for those users who want to work with artifact builds?

Comment 5

[Andreas Tolfsen ❲:ato❳]

(In reply to Henrik Skupin (:whimboo) from comment #4)
> Maybe we could have something like `mach build geckodriver` for those users
> who want to work with artifact builds?

We already have `./mach build testing/geckodriver`, but I’m not sure it is
possible to override the check for no compile environment in that case.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

That might be. For my artifact build config it runs too quick:

 0:00.40 /usr/bin/make -C /Volumes/data/code/gecko/obj/debug -j4 -s backend
 0:00.55 /usr/bin/make -j4 -s testing
 0:00.75 /usr/bin/make -j4 -s testing/geckodriver
 0:00.95 Your build was successful!

So I think we abort without any failure too :/

Comment 7

[Chris Manchester (limited bugmail, email directly)]

(In reply to Nathan Froyd [:froydnj] from comment #3)
> (In reply to Andreas Tolfsen ‹:ato› from comment #2)
> > However, I find it is confusing that no geckodriver binary or error is
> > emitted when --enable-geckodriver is switched on for artifact builds.  What
> > is the approach we have taken in similar cases in the past?
> 
> I don't think we have prior art here, because --enable-geckodriver is a
> little unusual in building a specific program.  But --enable-artifact builds
> are designed to require zero compilation, so not producing geckodriver is
> expected.

We should either make the option itself conditional with "when='--enable-compile-environment'" in the option definition or add it as a input to the option so we can error out with a better error message.

> 
> > Should it maybe return an error, and if so where, when asked to build
> > geckodriver in a non-compilation environment?
> 
> If --enable-geckodriver also toggles prefs or other settings, I think that
> means --enable-geckodriver should be an error with artifact builds.  Unless
> we could make --enable-geckodriver artifact builds look for artifacts from a
> normal --enable-geckodriver build?  Chris would be the person to ask about
> that.

We should be able to treat this like we do other test support binaries, by putting them in the test package definitions here: http://searchfox.org/mozilla-central/rev/20963d7269b1b14d455f47bc0260d0653015bf84/python/mozbuild/mozbuild/action/test_archive.py#34 and adding "test_artifact_patterns" in python/mozbuild/mozbuild/artifacts.py where appropriate. The only caveat here will be that the local artifact build will need to be on a platform where we build geckodriver in automation.

Comment 8

[Chris Manchester (limited bugmail, email directly)]

Attachment: Bug 1370535 - Install the geckodriver binary in artifact builds when available.

Review commit: https://reviewboard.mozilla.org/r/150516/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/150516/

Comment 9

[Chris Manchester (limited bugmail, email directly)]

Attachment: Bug 1370535 - Provide a useful error message when passing --enable-geckodriver in artifact bulds.

Review commit: https://reviewboard.mozilla.org/r/150518/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/150518/

Comment 10

[Nathan Froyd [:froydnj]]

Comment on attachment 8879183 [details]
Bug 1370535 - Provide a useful error message when passing --enable-geckodriver in artifact bulds.

https://reviewboard.mozilla.org/r/150518/#review155194

Comment 11

[Nathan Froyd [:froydnj]]

Comment on attachment 8879182 [details]
Bug 1370535 - Install the geckodriver binary in artifact builds when available.

https://reviewboard.mozilla.org/r/150516/#review155214

::: python/mozbuild/mozbuild/artifacts.py:134
(Diff revision 1)
>          ('bin/BadCertServer', ('bin', 'bin')),
>          ('bin/GenerateOCSPResponse', ('bin', 'bin')),
>          ('bin/OCSPStaplingServer', ('bin', 'bin')),
>          ('bin/certutil', ('bin', 'bin')),
>          ('bin/fileid', ('bin', 'bin')),
> +        ('bin/geckodriver', ('bin', 'bin')),

By my reading, this just means that `geckodriver` will be copied if it exists, not that things will error if it does not exist, correct?

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c58982c971e6
Install the geckodriver binary in artifact builds when available. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/d4d573d44528
Provide a useful error message when passing --enable-geckodriver in artifact bulds. r=froydnj

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c58982c971e6
https://hg.mozilla.org/mozilla-central/rev/d4d573d44528

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

So I run artifact builds, but i cannot find the binary of geckodriver anywhere under obj/dist/. At which place do we exactly download the binary?

Comment 15

[Chris Manchester (limited bugmail, email directly)]

It will be under $objdir/dist/bin

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Chris Manchester (:chmanchester) from comment #15)
> It will be under $objdir/dist/bin

I cannot find it there. So what does 'downloaded when available' mean? Don't we build it by default for all builds now?

Comment 17

[Chris Manchester (limited bugmail, email directly)]

(In reply to Henrik Skupin (:whimboo) [partly available 07/10 -07/14] from comment #16)
> (In reply to Chris Manchester (:chmanchester) from comment #15)
> > It will be under $objdir/dist/bin
> 
> I cannot find it there. So what does 'downloaded when available' mean? Don't
> we build it by default for all builds now?

It means if the upstream automation build we're installing from has a geckodriver binary we'll install it, but we can't if it doesn't.

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Chris Manchester (:chmanchester) from comment #17)
> It means if the upstream automation build we're installing from has a
> geckodriver binary we'll install it, but we can't if it doesn't.

Andreas, aren't we creating a geckodriver binary all the time now? I wonder why it cannot be found.

Comment 19

[Andreas Tolfsen ❲:ato❳]

(In reply to Henrik Skupin (:whimboo) [away 07/21 - 07/31] from
comment #18)

> (In reply to Chris Manchester (:chmanchester) from comment #17)
>
> > It means if the upstream automation build we're installing from
> > has a geckodriver binary we'll install it, but we can't if it
> > doesn't.
>
> Andreas, aren't we creating a geckodriver binary all the time
> now? I wonder why it cannot be found.

We are on environments that don’t cross-compile.  If you’re on
macOS, I’m afraid that’s a cross-compilation environment.

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Andreas Tolfsen ‹:ato› from comment #19)
> We are on environments that don’t cross-compile.  If you’re on
> macOS, I’m afraid that’s a cross-compilation environment.

Oh, darn. I am on that platform, and totally forgot about this situation. That perfectly explains it. Thanks.

Comment 21

[Andreas Tolfsen ❲:ato❳]

FWIW I can’t find the bug right now (and I’m a bit rushed) but
it’s very likely a silly bug with not setting up the Rust compile
environment correctly that prevents us from compiling geckodriver
for a macOS target on a Linux host system.  I random guess would be
that we’re not seeing all the necessary output variables.