Closed Bug 1468547 Opened 5 years ago Closed 5 years ago

Build the gtest libxul in the Tup backend

Categories

(Firefox Build System :: General, enhancement)

Product:
Firefox Build System
Component:
General
Version:
3 Branch
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox63 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: chmanchester, Assigned: chmanchester)

References

Details

Attachments

(3 files)

Bug 1468547 - Add a default output group to the Tup backend.
59 bytes, text/x-review-board-request
mshal
: review+
Details
Bug 1468547 - Build the gtest libxul in the tup backend.
59 bytes, text/x-review-board-request
mshal
: review+
Details
Bug 1468547 - Re-factor gtest mach command to not invoke make when not necessary.
59 bytes, text/x-review-board-request
mshal
: review+
Details 
This is one of the last things skipped after bug 1319228. We'll need to aggregate the dependencies between the two rust libraries so Tup doesn't end up generating duplicate rules.
Assignee: nobody → cmanchester
As a note to self, the second patch here should be tested on Windows before it lands.
Comment on attachment 8987734 [details]
Bug 1468547 - Add a default output group to the Tup backend.

https://reviewboard.mozilla.org/r/252998/#review259786

::: python/mozbuild/mozbuild/backend/tup.py:716
(Diff revision 2)
>          })
>          return env
>  
>      def _gen_cargo_rules(self, backend_file, build_plan, cargo_env):
> +
> +        if not self._rust_backend_file:

Is the ordering of the two rust directories guaranteed? If not, we could swap between writing out to toolkit/library/gtest/rust/Tupfile and toolkit/library/rust/Tupfile depending on which one gets processed first.

We may just want to do rust_backend_file = self._get_backend_file('toolkit/library/rust') here to ensure a consistent output directory. (On my machine, everything was written to the gtest/rust/Tupfile, which also may be confusing if people are expecting that to only be for things related to gtest).
Attachment #8987734 - Flags: review+
Attachment #8987734 - Flags: review?(core-build-config-reviews)
Comment on attachment 8987735 [details]
Bug 1468547 - Build the gtest libxul in the tup backend.

https://reviewboard.mozilla.org/r/253000/#review259798

::: python/mozbuild/mozbuild/mach_commands.py:598
(Diff revision 2)
>                debugger_args):
>  
>          # We lazy build gtest because it's slow to link
> -        self._run_make(directory="testing/gtest", target='gtest',
> -                       print_directory=False, ensure_exit_code=True)
> +        self.run_process([mozpath.join(self.topsrcdir, 'mach'), 'build',
> +                          mozpath.join(self.topobjdir, 'toolkit', 'library', 'gtest', 'rust')],
> +                         append_env={b'LINK_GTEST_DURING_COMPILE': b'1'},

Is this behavior of only building gtest/libxul.so during 'mach gtest' something we want to preserve in the tup backend?

We could probably hack something in to only link if this environment variable is set (coupled with an 'export LINK_GTEST_DURING_COMPILE' in the gtest Tupfile), and otherwise just create a stub libxul.so file with touch or something.

Another alternative is to put all non-gtest files in a group, and then we invoke tup with 'tup objdir/<firefox>', which invokes all rules except for gtest.

I'm also fine addressing this as a followup if/when it's an issue.
Attachment #8987735 - Flags: review?(core-build-config-reviews)
Comment on attachment 8987734 [details]
Bug 1468547 - Add a default output group to the Tup backend.

https://reviewboard.mozilla.org/r/252998/#review259786

> Is the ordering of the two rust directories guaranteed? If not, we could swap between writing out to toolkit/library/gtest/rust/Tupfile and toolkit/library/rust/Tupfile depending on which one gets processed first.
> 
> We may just want to do rust_backend_file = self._get_backend_file('toolkit/library/rust') here to ensure a consistent output directory. (On my machine, everything was written to the gtest/rust/Tupfile, which also may be confusing if people are expecting that to only be for things related to gtest).

I also get `gtest/rust`, I'll take this suggestion because it makes logs less confusing.
Comment on attachment 8987735 [details]
Bug 1468547 - Build the gtest libxul in the tup backend.

https://reviewboard.mozilla.org/r/253000/#review259798

> Is this behavior of only building gtest/libxul.so during 'mach gtest' something we want to preserve in the tup backend?
> 
> We could probably hack something in to only link if this environment variable is set (coupled with an 'export LINK_GTEST_DURING_COMPILE' in the gtest Tupfile), and otherwise just create a stub libxul.so file with touch or something.
> 
> Another alternative is to put all non-gtest files in a group, and then we invoke tup with 'tup objdir/<firefox>', which invokes all rules except for gtest.
> 
> I'm also fine addressing this as a followup if/when it's an issue.

On Linux it's fairly quick to link, I think it's probably not worth it until we have a Windows Tup build.
"fairly" as in 30s with BFD ld, 15s with gold. That's rather a lot of overhead...
Comment on attachment 8987735 [details]
Bug 1468547 - Build the gtest libxul in the tup backend.

For some reason this flag didn't get reset.
Attachment #8987735 - Flags: review?(mshal)
Comment on attachment 8987735 [details]
Bug 1468547 - Build the gtest libxul in the tup backend.

https://reviewboard.mozilla.org/r/253000/#review260118

::: python/mozbuild/mozbuild/mach_commands.py:598
(Diff revision 3)
>                debugger_args):
>  
>          # We lazy build gtest because it's slow to link
> -        self._run_make(directory="testing/gtest", target='gtest',
> -                       print_directory=False, ensure_exit_code=True)
> +        os.environ[b'LINK_GTEST_DURING_COMPILE'] = b'1'
> +        gtest_build_path = mozpath.relpath(mozpath.join(self.topobjdir,
> +                                                        'toolkit', 'library', 'gtest', 'rust'),

Shouldn't this just be toolkit/library/gtest instead of toolkit/library/gtest/rust? It seems to work fine with this in the make backend, but in the tup backend it doesn't end up building the gtest libxul.so since it is one directory up.
Attachment #8987735 - Flags: review?(mshal) → review+
Comment on attachment 8987735 [details]
Bug 1468547 - Build the gtest libxul in the tup backend.

https://reviewboard.mozilla.org/r/253000/#review260118

> Shouldn't this just be toolkit/library/gtest instead of toolkit/library/gtest/rust? It seems to work fine with this in the make backend, but in the tup backend it doesn't end up building the gtest libxul.so since it is one directory up.

Right, this works for make and we get away with it in Tup because we're building the gtest libxul by default. I was trying to not make this code conditional on the backend used, but the result is pretty awkward because partial tree builds don't really make sense in the make backend. I'll try to see how else we can do this...
Comment on attachment 8987735 [details]
Bug 1468547 - Build the gtest libxul in the tup backend.

https://reviewboard.mozilla.org/r/253000/#review260118

> Right, this works for make and we get away with it in Tup because we're building the gtest libxul by default. I was trying to not make this code conditional on the backend used, but the result is pretty awkward because partial tree builds don't really make sense in the make backend. I'll try to see how else we can do this...

Ahh, that makes sense about the Tup backend. For the make backend, the Makefile.in had: '$(MAKE) -C $(DEPTH) toolkit/library/gtest/target LINK_GTEST_DURING_COMPILE=1'. How come we needed to change the make call with LINK_GTEST_DURING_COMPILE=1 from toolkit/library/gtest to toolkit/library/gtest/rust? I think for parity we would not have the 'rust' path here, but I might be missing something.
Comment on attachment 8987735 [details]
Bug 1468547 - Build the gtest libxul in the tup backend.

https://reviewboard.mozilla.org/r/253000/#review260118

> Ahh, that makes sense about the Tup backend. For the make backend, the Makefile.in had: '$(MAKE) -C $(DEPTH) toolkit/library/gtest/target LINK_GTEST_DURING_COMPILE=1'. How come we needed to change the make call with LINK_GTEST_DURING_COMPILE=1 from toolkit/library/gtest to toolkit/library/gtest/rust? I think for parity we would not have the 'rust' path here, but I might be missing something.

Right, the gtest libxul fails to link in the make backend if we do this because it doesn't build the necessary rust code. Partial tree builds don't really make sense in the make backend.
(In reply to Chris Manchester (:chmanchester) from comment #17)
> Right, the gtest libxul fails to link in the make backend if we do this
> because it doesn't build the necessary rust code. Partial tree builds don't
> really make sense in the make backend.

Ahh, I see - that's a bummer. Nevermind then!
I hacked up a version of this to not link the gtest libxul by default. I also made a patch to allow specifying labels for outputs to not be included in the default build, but the changes to the make backend for that were sort of extensive so I'll post that in a separate bug.
Comment on attachment 8987734 [details]
Bug 1468547 - Add a default output group to the Tup backend.

https://reviewboard.mozilla.org/r/252998/#review262592

Looks good! I'm kinda surprised this didn't need to be more complicated.
Attachment #8987734 - Flags: review?(mshal) → review+
Comment on attachment 8987735 [details]
Bug 1468547 - Build the gtest libxul in the tup backend.

https://reviewboard.mozilla.org/r/253000/#review262588

::: python/mozbuild/mozbuild/backend/tup.py:728
(Diff revision 4)
>                                      ' '.join(self.environment.substs['RUSTFLAGS'])),
>          })
>          return env
>  
>      def _gen_cargo_rules(self, backend_file, build_plan, cargo_env):
> +

nit: stray newline inserted

::: python/mozbuild/mozbuild/backend/tup.py:814
(Diff revision 4)
> -                display='%s %s' % (header, display_name(invocation)),
> +                    display='%s %s' % (header, display_name(invocation)),
> -            )
> +                )
>  
>              for dst, link in invocation['links'].iteritems():
> -                backend_file.symlink_rule(link, dst, self._rust_libs)
> +                output_key = (dst,)
> +                if output_key not in self._rust_outputs:

Instead of adding the symlinks as separate outputs to _rust_outputs, the 'for dst, link' loop could just be inside the 'if output_key not in self.rust_outputs' conditional above:

if output_key not in self._rust_outputs:
    self._rust_outputs.add(output_key)
    self._rust_backend_file.rule(...)

    for dst, link in invocation['links'].iteritems():
        self._rust_backend_file.symlink_rule(link, dst, self._rust_libs)
Comment on attachment 8990466 [details]
Bug 1468547 - Re-factor gtest mach command to not invoke make when not necessary.

https://reviewboard.mozilla.org/r/255538/#review262596

::: python/mozbuild/mozbuild/mach_commands.py:603
(Diff revision 1)
> +        except Exception:
> +            print("Please run |./mach build| before |./mach gtest|.")
> +            return 1
> +
> +        active_backend = config.substs.get('BUILD_BACKENDS', [None])[0]
> +        if 'Tup' in active_backend:

I think this would be easier if we added a gtest() method to the backend class, similar to what we do for build(): https://dxr.mozilla.org/mozilla-central/rev/085cdfb90903d4985f0de1dc7786522d9fb45596/python/mozbuild/mozbuild/controller/building.py#1033

So this could be:

backend_cls = get_backend_class(active_backend)(config)
backend_cls.gtest(...)

So the make backend can set the LINK_GTEST_DURING_COMPILE environment variable and call build with its specific target, and the tup backend can call build with its specific target.

This is fine for now though if you want to get it landed.
Attachment #8990466 - Flags: review?(mshal) → review+
Comment on attachment 8987735 [details]
Bug 1468547 - Build the gtest libxul in the tup backend.

https://reviewboard.mozilla.org/r/253000/#review262588

> Instead of adding the symlinks as separate outputs to _rust_outputs, the 'for dst, link' loop could just be inside the 'if output_key not in self.rust_outputs' conditional above:
> 
> if output_key not in self._rust_outputs:
>     self._rust_outputs.add(output_key)
>     self._rust_backend_file.rule(...)
> 
>     for dst, link in invocation['links'].iteritems():
>         self._rust_backend_file.symlink_rule(link, dst, self._rust_libs)

Good catch, thanks!
Comment on attachment 8990466 [details]
Bug 1468547 - Re-factor gtest mach command to not invoke make when not necessary.

https://reviewboard.mozilla.org/r/255538/#review262596

> I think this would be easier if we added a gtest() method to the backend class, similar to what we do for build(): https://dxr.mozilla.org/mozilla-central/rev/085cdfb90903d4985f0de1dc7786522d9fb45596/python/mozbuild/mozbuild/controller/building.py#1033
> 
> So this could be:
> 
> backend_cls = get_backend_class(active_backend)(config)
> backend_cls.gtest(...)
> 
> So the make backend can set the LINK_GTEST_DURING_COMPILE environment variable and call build with its specific target, and the tup backend can call build with its specific target.
> 
> This is fine for now though if you want to get it landed.

I'll leave this to bug 1474028, I have a patch there that simplifies things a bit here and gets rid of the environment variable.
Pushed by cmanchester@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b9937e7b0995
Add a default output group to the Tup backend. r=mshal
https://hg.mozilla.org/integration/autoland/rev/f211edf86a42
Build the gtest libxul in the tup backend. r=mshal
https://hg.mozilla.org/integration/autoland/rev/b18846ed7d05
Re-factor gtest mach command to not invoke make when not necessary. r=mshal
Version: Version 3 → 3 Branch
You need to log in before you can comment on or make changes to this bug.