If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

"ac_add_options --enable-geckodriver" breaks artifact builds

RESOLVED FIXED in Firefox 56

Status

()

Core
Build Config
RESOLVED FIXED
4 months ago
3 months ago

People

(Reporter: whimboo, Assigned: chmanchester)

Tracking

(Blocks: 1 bug)

54 Branch
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

4 months ago
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.
Flags: needinfo?(ato)
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.
(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?
Flags: needinfo?(ato)
(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.
(Reporter)

Comment 4

4 months ago
Maybe we could have something like `mach build geckodriver` for those users who want to work with artifact builds?
(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.
(Reporter)

Comment 6

4 months ago
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 :/
(Assignee)

Comment 7

4 months ago
(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.
(Assignee)

Updated

4 months ago
Assignee: nobody → cmanchester
Blocks: 1278699
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 10

4 months ago
mozreview-review
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
Attachment #8879183 - Flags: review?(nfroyd) → review+

Comment 11

4 months ago
mozreview-review
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?
Attachment #8879182 - Flags: review?(nfroyd) → review+

Comment 12

4 months ago
Pushed by cmanchester@mozilla.com:
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

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c58982c971e6
https://hg.mozilla.org/mozilla-central/rev/d4d573d44528
Status: NEW → RESOLVED
Last Resolved: 4 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
(Reporter)

Comment 14

3 months ago
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?
Flags: needinfo?(cmanchester)
(Assignee)

Comment 15

3 months ago
It will be under $objdir/dist/bin
Flags: needinfo?(cmanchester)
(Reporter)

Comment 16

3 months ago
(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?
Flags: needinfo?(cmanchester)
(Assignee)

Comment 17

3 months ago
(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.
Flags: needinfo?(cmanchester)
(Reporter)

Comment 18

3 months ago
(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.
Flags: needinfo?(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.
Flags: needinfo?(ato)
(Reporter)

Comment 20

3 months ago
(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.
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.
You need to log in before you can comment on or make changes to this bug.