Local hazard builds have bitrotted a bit

RESOLVED FIXED in Firefox 63

Status

()

enhancement
RESOLVED FIXED
11 months ago
8 months ago

People

(Reporter: sfink, Assigned: sfink)

Tracking

(Blocks 1 bug)

unspecified
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(10 attachments)

846 bytes, patch
jonco
: review+
Details | Diff | Splinter Review
737 bytes, patch
sfink
: review+
Details | Diff | Splinter Review
1.07 KB, patch
sfink
: review+
Details | Diff | Splinter Review
2.39 KB, patch
jonco
: review+
Details | Diff | Splinter Review
1.21 KB, patch
jonco
: review+
Details | Diff | Splinter Review
2.66 KB, patch
jonco
: review+
Details | Diff | Splinter Review
1.69 KB, patch
sfink
: review+
Details | Diff | Splinter Review
1.77 KB, patch
jonco
: review+
Details | Diff | Splinter Review
1.13 KB, patch
jonco
: review+
Details | Diff | Splinter Review
2.85 KB, patch
jonco
: review+
Details | Diff | Splinter Review
Assignee

Description

11 months ago
Various minor things. There's one more critically important fix to be able to run local builds, necessary because of the default build switching to clang, but that's dependent on another bug so I'll leave it for later.
Assignee

Comment 1

11 months ago
This option is used for release builds to ensure that they run on ancient versions of linux distros, but it causes checker errors on local builds and We Just Don't Care.
Attachment #8996203 - Flags: review?(jcoppeard)
Assignee

Comment 2

11 months ago
Not worth a review. My laptop can have problems running this many parallel compile jobs.
Assignee

Comment 3

11 months ago
Not worth reviewing. This will print out commands for running the various substeps, which is really handy for cutting & pasting to rerun them. (Expected usage: run the whole thing locally, then cut & paste these commands to run the parts, possibly under a JS debugger.)
Assignee

Comment 4

11 months ago
It's slightly nicer for using the JS debugger if output doesn't go via shell redirection. (Without this change, it's common to run computeCallgraph.js under the JS debugger, and have it spend >90% of its runtime printing the output to the terminal. With this change, it will go straight to the appropriate output file.)
Attachment #8996212 - Flags: review?(jcoppeard)

Updated

11 months ago
Attachment #8996203 - Flags: review?(jcoppeard) → review+

Updated

11 months ago
Attachment #8996212 - Flags: review?(jcoppeard) → review+
Assignee

Comment 5

11 months ago
I guess I'll borrow this bug for some more random stuff.
Attachment #8996495 - Flags: review?(jcoppeard)
Assignee

Comment 6

11 months ago
Ok, this is an abuse of this bug. This is really a fix to the test suite; when parsing the results, it wasn't working for unquoted field calls.
Attachment #8996496 - Flags: review?(jcoppeard)
Assignee

Comment 7

11 months ago
No need to review; with the callgraph.txt output change, this really isn't needed anymore, but it also contains a switch to using template strings that looks better. So why not.
Assignee

Comment 8

11 months ago
When running locally under rr, I was baffled for a while as to why cc1plus was running twice. I thought there must be some bug in my test harness that compiled everything twice.

It turns out that it's intentional behavior for this particular test, which handles a subtlety related to exception handling. (Constructors can be interrupted with exceptions, and will run cleanup code in that case. But not if you're compiling with -fno-exceptions.)
Attachment #8996500 - Flags: review?(jcoppeard)
Assignee

Comment 9

11 months ago
This is a tricky one, and I need to run it through try now that I *think* the prerequisite bug has landed.

The problem is that I was clearing out any $CC/$CXX that might be set to a wrapper, and letting it use the default that will at this point in the processing be found in $PATH.

Except that now the default compiler is clang, so it'll try to use that and fail.

But... the rust compilation setup had a bug where it processes --with-compiler-wrapper stuff and mangles it if it's not in its whitelist, and somehow that gets involved here. I *think* that is fixed in bug 1409276. But until it is, this breaks the gecko tree rust compilation stuff. (Not the main rust compilation, it's when the rust compilation setup tries to compile c/c++ code.)

Anyway, it's messy and hopefully fixed, but I haven't tried it out yet. I'm not really happy with the particular fix here; I should really be doing something smarter with any existing CC/CXX settings. But this fixes the main problem, so as long as it doesn't introduce another one, it seems good enough.
Attachment #8996886 - Flags: review?(jcoppeard)

Comment 10

11 months ago
Pushed by sfink@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e438cdb3c51
Remove --enable-stdcxx-compat option from JS shell being analyzed for hazards, r=jonco
Assignee

Updated

11 months ago
Keywords: leave-open
Assignee

Comment 11

11 months ago
Comment on attachment 8996204 [details] [diff] [review]
Reduce the number of parallel compiles for JS shell hazard analysis

Marking trivial patches r=me
Attachment #8996204 - Flags: review+
Assignee

Updated

11 months ago
Attachment #8996205 - Flags: review+
Assignee

Updated

11 months ago
Attachment #8996497 - Flags: review+
Assignee

Comment 13

11 months ago
With this, it is now possible to run build-haz-linux.sh with no environment variables set and have it work!

Well, once you have all of the prerequisites downloaded and placed into the directory you pass in, anyway.

Usage after this patch (quick version, checking only the JS shell):

  taskcluster/scripts/builder/build-haz-linux.sh --no-tooltool --project shell /some/work/dir

with /some/work/dir containing installs of gcc, sixgill, clang, and rustc (I'm not sure if the latter two are used at all yet.)

Those can be installed by running once by setting something like

MOZ_TOOLCHAINS="public/build/clang.tar.xz@FQ6FV3QdRI-SvJ-OyT_GeQ public/build/gcc.tar.xz@BYmAixHnT-mG2qmATEonbw public/build/sixgill.tar.xz@e8vnR-YETq6YR9B3c76oBg public/build/rustc.tar.xz@dA6thFKRQEum6HxaV9p2NA public/build/node.tar.xz@T0hi2BEGS7GPDVJY3FujQg"

and then running without the --no-tooltool option, though those will change with later updates. There's supposed to be a mach artifact toolchain command for finding the latest of these by name, but it's broken for reasons I forget, and I'm failing to find the bug number right now.
Attachment #8997567 - Flags: review?(jcoppeard)

Updated

11 months ago
Attachment #8996495 - Flags: review?(jcoppeard) → review+

Updated

11 months ago
Attachment #8996496 - Flags: review?(jcoppeard) → review+

Updated

11 months ago
Attachment #8996500 - Flags: review?(jcoppeard) → review+
Comment on attachment 8996886 [details] [diff] [review]
Force use of gcc for hazard analysis

Review of attachment 8996886 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/devtools/rootAnalysis/run_complete
@@ +251,5 @@
>  
>          # update the PATH so that the build will see the wrappers.
>          if (exists $ENV{CC}) {
>              $ENV{PATH} = dirname($ENV{CC}) . ":$ENV{PATH}";
> +            $ENV{CC} = "gcc";

This seems strange that we're adding the directory containing $ENV{CC} to the path but then not using it, but if you say so...
Attachment #8996886 - Flags: review?(jcoppeard) → review+
Comment on attachment 8997567 [details] [diff] [review]
Remove need to set GECKO_DIR when running build-haz-linux.sh, and fix shell lint warnings

Review of attachment 8997567 [details] [diff] [review]:
-----------------------------------------------------------------

::: taskcluster/scripts/builder/build-haz-linux.sh
@@ +52,5 @@
>  export MOZ_OBJDIR="$WORKSPACE/obj-analyzed"
>  mkdir -p "$MOZ_OBJDIR"
>  
>  if [ -n "$DO_TOOLTOOL" ]; then
> +  ( cd "$TOOLTOOL_DIR"; "$GECKO_DIR"/mach artifact toolchain -v${TOOLTOOL_MANIFEST:+ --tooltool-url https://tooltool.mozilla-releng.net/ --tooltool-manifest "$GECKO_DIR/$TOOLTOOL_MANIFEST"} --cache-dir "$TOOLTOOL_CACHE"${MOZ_TOOLCHAINS:+ ${MOZ_TOOLCHAINS}} )

If it's possible to format this so it's more readable that would be great.
Attachment #8997567 - Flags: review?(jcoppeard) → review+
Assignee

Comment 16

11 months ago
(In reply to Jon Coppeard (:jonco) from comment #14)
> Comment on attachment 8996886 [details] [diff] [review]
> Force use of gcc for hazard analysis
> 
> Review of attachment 8996886 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/devtools/rootAnalysis/run_complete
> @@ +251,5 @@
> >  
> >          # update the PATH so that the build will see the wrappers.
> >          if (exists $ENV{CC}) {
> >              $ENV{PATH} = dirname($ENV{CC}) . ":$ENV{PATH}";
> > +            $ENV{CC} = "gcc";
> 
> This seems strange that we're adding the directory containing $ENV{CC} to
> the path but then not using it, but if you say so...

It's complicated and I'm not sure I completely understand it.

But 2 lines after this is

        $ENV{"PATH"} = "$wrap_dir:" . $ENV{"PATH"};

So actually, it's *not* invoking the original $ENV{CC}; we're invoking $wrap_dir/gcc. Though I wonder if $ENV{CC} = basename($ENV{CC}) would have been better here...? Anyway, $wrap_dir/gcc is a shell script that is going to look for the real compiler and invoke it with additional arguments. It finds the compiler by removing its own directory from $PATH and then basically doing `which gcc` (it gets "gcc" from whatever name you invoked the wrapper script with.)

The simpler approach would be to set $ENV{WRAPPED_CC} to $ENV{CC}, and have the wrapper script look for $WRAPPED_CC. I wonder why I didn't do that in the first place... it's tricky, because this stuff gets invoked in a number of different ways (via mach build, via plain make, via this run_complete wrapper script that sets up a common server to receive results then invokes whatever actual build command you're using, ... maybe more? We don't actually need all of these, but I didn't want to break them with any changes in the sixgill repo.)

Anyway, I will at least comment what's going on here, and maybe I'll take a stab at a $WRAPPED_CC. It won't handle all the cases, but as long as it handles this one then it doesn't really matter; the other ones won't be running this script and as long as they don't depend on $WRAPPED_CC, they shouldn't change.
Assignee

Comment 17

11 months ago
Oh, right. $WRAPPED_CC doesn't handle everything that's going on, though it would simplify things a bit.

The problem is that we need to run $wrap_dir/gcc, but the build will probably run whatever $CC says, so we either have to set up $PATH and blow away $CC, or set $CC to $wrap_dir/gcc and arrange for the wrapper compiler to find the original compiler (whether it started out in $CC or CC was unset and it was found via $PATH).
Assignee

Comment 18

11 months ago
Turns out there already is something like $WRAPPED_CC: $GCCDIR. So I switched to that, which makes the sequence of actions much simpler to follow. And it seems to work both locally and on try, so I'll use it instead.
Assignee

Comment 19

11 months ago
(In reply to Jon Coppeard (:jonco) from comment #15)
> > +  ( cd "$TOOLTOOL_DIR"; "$GECKO_DIR"/mach artifact toolchain -v${TOOLTOOL_MANIFEST:+ --tooltool-url https://tooltool.mozilla-releng.net/ --tooltool-manifest "$GECKO_DIR/$TOOLTOOL_MANIFEST"} --cache-dir "$TOOLTOOL_CACHE"${MOZ_TOOLCHAINS:+ ${MOZ_TOOLCHAINS}} )
> 
> If it's possible to format this so it's more readable that would be great.

What do you mean? That's beautiful! The very picture of clarity and simplicity! What more could you possibly ask for?

Anyway... yeah, I backslash-newlined my way to something a little more readable.
Assignee

Updated

11 months ago
Keywords: leave-open

Comment 20

11 months ago
Pushed by sfink@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbbf72dcf175
Reduce the number of parallel compiles for JS shell hazard analysis. r=me
https://hg.mozilla.org/integration/mozilla-inbound/rev/45b699f010a6
More verbose output from hazard commands. r=me
https://hg.mozilla.org/integration/mozilla-inbound/rev/07e7080fdf6c
Force use of gcc for hazard analysis, r=jonco

Comment 21

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fbbf72dcf175
https://hg.mozilla.org/mozilla-central/rev/45b699f010a6
https://hg.mozilla.org/mozilla-central/rev/07e7080fdf6c
Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63

Comment 22

8 months ago
Pushed by sfink@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d67c4217db0b
Write callgraph.txt via JS shell redirection instead of bash redirection, r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/5dc260a940fb
Remove need to set GECKO_DIR when running build-haz-linux.sh, and fix shell lint warnings, r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/8308ccadecfa
Report which test failed, and suppress the spew on saying subprocess.CalledProcessError with a messy stack
https://hg.mozilla.org/integration/mozilla-inbound/rev/de0d7eda05f7
Quote all GC functions, even if field calls, r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/a38d3c565e75
Use template string for compact code, r=me
https://hg.mozilla.org/integration/mozilla-inbound/rev/47d9c24f99d7
Comment what is going on in the exceptions test so I don't waste time figuring it out again, r=me
You need to log in before you can comment on or make changes to this bug.