Rooting analysis mozconfig should be in the tree

RESOLVED FIXED in B2G C4 (2jan on)

Status

Release Engineering
General Automation
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: gps, Assigned: sfink)

Tracking

other
B2G C4 (2jan on)
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 6 obsolete attachments)

1.64 KB, patch
sfink
: checked-in+
Details | Diff | Splinter Review
1.43 KB, patch
aki
: review+
sfink
: checked-in+
Details | Diff | Splinter Review
1.31 KB, patch
glandium
: review+
sfink
: checked-in+
Details | Diff | Splinter Review
1.73 KB, patch
glandium
: review+
sfink
: checked-in+
Details | Diff | Splinter Review
19.53 KB, patch
aki
: review+
sfink
: checked-in+
Details | Diff | Splinter Review
(Reporter)

Description

4 years ago
https://hg.mozilla.org/build/mozharness/file/d416937ec90a/scripts/spidermonkey/build.browser defines a mozconfig file. I thought we put automation's mozconfig files in the tree so people could adjust things during try pushes among other things.
(Reporter)

Comment 1

4 years ago
The reason this is blocking 961258 is because build/mozconfig.common is not sourced by this job. This is the only job on try not sourcing that file. That's a bug.
(Assignee)

Comment 2

4 years ago
Created attachment 8363289 [details] [diff] [review]
Add the rootanalysis mozconfig to the tree

I'm not entirely sure where to put it. This patch puts it in browser/config/mozconfigs/rootanalysis. Note that we only run the rootanalysis build on linux64, so I could put it in the linux64 directory. But there's not that much of a reason for that restriction. Alternatively, it could live in js/src/devtools/rootAnalysis like the rest of the rootanalysis stuff does. I dunno.
Attachment #8363289 - Flags: review?(gps)
(Assignee)

Updated

4 years ago
Assignee: nobody → sphink
Status: NEW → ASSIGNED
(Assignee)

Comment 3

4 years ago
Created attachment 8363295 [details] [diff] [review]
Switch to in-tree mozconfig and new path for JS binary

Terrence: there's nothing here that really needs any special review. Basic sanity checking is fine.
Attachment #8363295 - Flags: review?(terrence)
Comment on attachment 8363295 [details] [diff] [review]
Switch to in-tree mozconfig and new path for JS binary

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

Sure! r=me
Attachment #8363295 - Flags: review?(terrence) → review+
(Reporter)

Comment 5

4 years ago
Comment on attachment 8363289 [details] [diff] [review]
Add the rootanalysis mozconfig to the tree

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

A short comment at the top of the file might prevent future head scratching.

::: browser/config/mozconfigs/rootanalysis
@@ +1,5 @@
> +ac_add_options --enable-debug
> +ac_add_options --enable-tests
> +ac_add_options --enable-optimize
> +ac_add_options --disable-elf-hack
> +. $topsrcdir/browser/config/mozconfig

Might want to source the file first.

@@ +9,5 @@
> +mk_add_options MOZ_OBJDIR=$ANALYZED_OBJDIR
> +
> +export CFLAGS=-Wno-attributes
> +export CPPFLAGS=-Wno-attributes
> +export CXXFLAGS=-Wno-attributes

You shouldn't need export here.

We should append to these variables, not overwrite them, right?

e.g. CFLAGS="$CFLAGS -Wno-attributes"
Attachment #8363289 - Flags: review?(gps) → review+
Duplicate of this bug: 965148
Comment on attachment 8363289 [details] [diff] [review]
Add the rootanalysis mozconfig to the tree

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

You need to source at least $topsrcdir/build/unix/mozconfig.linux and 
$topsrcdir/build/mozconfig.common.override.

::: browser/config/mozconfigs/rootanalysis
@@ +1,4 @@
> +ac_add_options --enable-debug
> +ac_add_options --enable-tests
> +ac_add_options --enable-optimize
> +ac_add_options --disable-elf-hack

Can you add a comment as to why elfhack needs to be disabled?
Attachment #8363289 - Flags: review+ → review-
Also, something is setting PATH to contain /tools/gcc-4.7.2-0moz1/bin. That should be removed.
(Assignee)

Comment 9

4 years ago
(In reply to Mike Hommey [:glandium] from comment #7)
> Comment on attachment 8363289 [details] [diff] [review]
> Add the rootanalysis mozconfig to the tree
> 
> Review of attachment 8363289 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You need to source at least $topsrcdir/build/unix/mozconfig.linux and 
> $topsrcdir/build/mozconfig.common.override.
> 
> ::: browser/config/mozconfigs/rootanalysis
> @@ +1,4 @@
> > +ac_add_options --disable-elf-hack
> 
> Can you add a comment as to why elfhack needs to be disabled?

I think it failed, so I eliminated it immediately since it's irrelevant here. (If I had a build target that skipped linking entirely, I would use it.) But here's a push with it included:

12:59:09     INFO -  21:27.01 elfhack
12:59:10     INFO -  21:27.76 ===
12:59:10     INFO -  21:27.76 === If you get failures below, please file a bug describing the error
12:59:10     INFO -  21:27.76 === and your environment (compiler and linker versions), and use
12:59:10     INFO -  21:27.77 === --disable-elf-hack until this is fixed.
12:59:10     INFO -  21:27.77 ===
12:59:10     INFO -  21:27.82 ===
12:59:10     INFO -  21:27.82 === If you get failures below, please file a bug describing the error
12:59:10     INFO -  21:27.82 === and your environment (compiler and linker versions), and use
12:59:10     INFO -  21:27.82 === --disable-elf-hack until this is fixed.
12:59:10     INFO -  21:27.82 ===
12:59:10     INFO -  21:27.83  0x0000000000000019 (INIT_ARRAY)         0x2073e0
12:59:10     INFO -  21:27.83  0x000000000000000c (INIT)               0x3fe8
12:59:10     INFO -  21:27.83 /builds/slave/l64-br-haz_try_dep-00000000000/build/obj-analyzed/build/unix/elfhack/elfhack: /usr/lib64/libstdc++.so.6: version `GLIBCXX_3.4.15' not found (required by /builds/slave/l64-br-haz_try_dep-00000000000/build/obj-analyzed/build/unix/elfhack/elfhack)
12:59:10     INFO -  21:27.83 /builds/slave/l64-br-haz_try_dep-00000000000/build/obj-analyzed/build/unix/elfhack/elfhack: /usr/lib64/libstdc++.so.6: version `GLIBCXX_3.4.15' not found (required by /builds/slave/l64-br-haz_try_dep-00000000000/build/obj-analyzed/build/unix/elfhack/elfhack)
(Assignee)

Comment 10

4 years ago
(In reply to Mike Hommey [:glandium] from comment #8)
> Also, something is setting PATH to contain /tools/gcc-4.7.2-0moz1/bin. That
> should be removed.

That will be a problem.

gcc 4.7.3 has a bug that causes the static analysis to fail (a type lookup from the plugin code throws an error that kills the compile.) gcc 4.7.2 for mysterious reasons does not hit the problem (the code is the same; I'm not sure why it doesn't fail.) gcc 4.8+ appear to have fixed the bug, fwiw.

But if the mozconfig gcc is 4.7.3, that's going to break the analysis build.

From bug 965148, it looks like gcc 4.7.2 might now break the build?
(Assignee)

Comment 11

4 years ago
Created attachment 8369618 [details] [diff] [review]
Add the rootanalysis mozconfig to the tree

This is a mozconfig that works for me. Note that with the other mozconfigs included, I no longer need to disable elfhack. But I still need to use GCC 4.7.2.
Attachment #8369618 - Flags: review?(mh+mozilla)
(Assignee)

Updated

4 years ago
Attachment #8363289 - Attachment is obsolete: true
(In reply to Steve Fink [:sfink] from comment #10)
> From bug 965148, it looks like gcc 4.7.2 might now break the build?

The problem is not gcc 4.7.2, it's binutils. And the tooltool gcc comes with a newer binutils.

(In reply to Steve Fink [:sfink] from comment #11)
> Created attachment 8369618 [details] [diff] [review]
> Add the rootanalysis mozconfig to the tree
> 
> This is a mozconfig that works for me. Note that with the other mozconfigs
> included, I no longer need to disable elfhack.

That's because of --enable-stdcxx-compat

> But I still need to use GCC 4.7.2.

Maybe you could get a gcc 4.7.2+newer binutils build in tooltool, then?
(Assignee)

Comment 13

4 years ago
(In reply to Mike Hommey [:glandium] from comment #12)
> (In reply to Steve Fink [:sfink] from comment #10)
> > From bug 965148, it looks like gcc 4.7.2 might now break the build?
> 
> The problem is not gcc 4.7.2, it's binutils. And the tooltool gcc comes with
> a newer binutils.

What breaks? Is it anything I need to care about for the analysis builds? (eg if it's pgo-only, I don't care.)

> (In reply to Steve Fink [:sfink] from comment #11)
> > But I still need to use GCC 4.7.2.
> 
> Maybe you could get a gcc 4.7.2+newer binutils build in tooltool, then?

Ok, I have one in bug 967383. But can I understand what this is for? Why is switching to tooltool a good thing? Does it give me something different from building an RPM and installing it in my mock environment? I still don't really know how to use it, either. Should I change my build scripts to fetch it via tooltool now? Where should I put my analysis-specific gcc tooltool manifest file? Somewhere in js/src/devtools/rootAnalysis with the rest of the analysis code, or in browser/config/tooltool, or ? If I were to make an osx version of the analysis build, where does the logic to select the right tooltool manifest go?

/me is missing the big picture here.
(In reply to Steve Fink [:sfink] from comment #13)
> (In reply to Mike Hommey [:glandium] from comment #12)
> > (In reply to Steve Fink [:sfink] from comment #10)
> > > From bug 965148, it looks like gcc 4.7.2 might now break the build?
> > 
> > The problem is not gcc 4.7.2, it's binutils. And the tooltool gcc comes with
> > a newer binutils.
> 
> What breaks? Is it anything I need to care about for the analysis builds?
> (eg if it's pgo-only, I don't care.)

New webrtc code uses avx assembly that the old binutils doesn't know.

> > (In reply to Steve Fink [:sfink] from comment #11)
> > > But I still need to use GCC 4.7.2.
> > 
> > Maybe you could get a gcc 4.7.2+newer binutils build in tooltool, then?
> 
> Ok, I have one in bug 967383. But can I understand what this is for? Why is
> switching to tooltool a good thing? Does it give me something different from
> building an RPM and installing it in my mock environment? I still don't
> really know how to use it, either. Should I change my build scripts to fetch
> it via tooltool now? Where should I put my analysis-specific gcc tooltool
> manifest file? Somewhere in js/src/devtools/rootAnalysis with the rest of
> the analysis code, or in browser/config/tooltool, or ? If I were to make an
> osx version of the analysis build, where does the logic to select the right
> tooltool manifest go?
> 
> /me is missing the big picture here.

I'll leave this to some releng people to answer because i really don't know how root analysis is hooked with buildbot.
Note an obvious advantage of tooltool over anything else is that as long as you have packages pushed to the tooltool server, you can test them on try without requiring changes to anything else than the tree you push to try.
(Assignee)

Comment 16

4 years ago
(In reply to Mike Hommey [:glandium] from comment #15)
> Note an obvious advantage of tooltool over anything else is that as long as
> you have packages pushed to the tooltool server, you can test them on try
> without requiring changes to anything else than the tree you push to try.

That's a wash. With tooltool, I have to get stuff pushed to the tooltool server before I can test with it. With rpm/mock, I have to get stuff pushed to the RPM server before I can test with it. Both kinda suck when you're not sure whether your package is final.

I usually test with my own slave first, and I know how to point to an additional yum repo where I can put trial packages. I haven't figured out how to add an additional tooltool server, though I guess I can probably just replace the one and only server with my own since I don't need very much from it. (Or I could do a server-side redirect for unknown hashes.)
(Reporter)

Comment 17

4 years ago
(In reply to Steve Fink [:sfink] from comment #16)
> (In reply to Mike Hommey [:glandium] from comment #15)
> > Note an obvious advantage of tooltool over anything else is that as long as
> > you have packages pushed to the tooltool server, you can test them on try
> > without requiring changes to anything else than the tree you push to try.
> 
> That's a wash. With tooltool, I have to get stuff pushed to the tooltool
> server before I can test with it. With rpm/mock, I have to get stuff pushed
> to the RPM server before I can test with it. Both kinda suck when you're not
> sure whether your package is final.
> 
> I usually test with my own slave first, and I know how to point to an
> additional yum repo where I can put trial packages. I haven't figured out
> how to add an additional tooltool server, though I guess I can probably just
> replace the one and only server with my own since I don't need very much
> from it. (Or I could do a server-side redirect for unknown hashes.)

That sounds like a developer productivity issue. It sounds like we need to lower the barrier to testing custom packages in automation.

CC Taras.
Blocks: 834909
(Assignee)

Comment 18

4 years ago
Created attachment 8371919 [details] [diff] [review]
Switch to using tooltool for gcc and sixgill

Hoo boy. Switching over my mozharness script to use tooltool turned out to require some changes. I tried not to make too much of a mess, but... well, it's still a mess.

This still feels like it's missing a level of indirection. My tooltool-ized sixgill links against an older libgmp than I have on my laptop, so I really need some sort of manifest selection or something. And I may have to make yet another version for the b2g analysis, since it needs to work with a prebuild Android NDK gcc.
Attachment #8371919 - Flags: review?(aki)
(Assignee)

Updated

4 years ago
Depends on: 969157, 967383
(Assignee)

Comment 19

4 years ago
Created attachment 8371967 [details] [diff] [review]
tooltool manifests for hazard analysis (gcc and sixgill)

Current tooltool manifests that seem to work with my build slave.
(Assignee)

Comment 20

4 years ago
Created attachment 8371993 [details] [diff] [review]
Switch to using tooltool for gcc and sixgill

Rebased past 28 changesets.
Attachment #8371993 - Flags: review?(aki)
(Assignee)

Updated

4 years ago
Attachment #8371919 - Attachment is obsolete: true
Attachment #8371919 - Flags: review?(aki)

Comment 21

4 years ago
Comment on attachment 8371993 [details] [diff] [review]
Switch to using tooltool for gcc and sixgill

I'm kind of wary of adding an unused 'privileged' kwarg to run_command().
Does it work if we add **kwargs for ignored kwargs?
(Assignee)

Comment 22

4 years ago
Created attachment 8372045 [details] [diff] [review]
Switch to using tooltool for gcc and sixgill

Yeah, that's probably better. I was tempted to prefix the non-mock command with 'sudo'. :-)
Attachment #8372045 - Flags: review?(aki)
(Assignee)

Updated

4 years ago
Attachment #8371993 - Attachment is obsolete: true
Attachment #8371993 - Flags: review?(aki)

Updated

4 years ago
Attachment #8372045 - Flags: review?(aki) → review+
(Assignee)

Comment 23

4 years ago
Created attachment 8372068 [details] [diff] [review]
Pass through tooltool_url_list to mozharness

The buildbot configs seem to be the current canonical source for the tooltool server URL.

It would be nice to split out "slave (build network) environment" stuff from the rest of the buildbot configuration, since it's really not tied to buildbot specifically and yet it's what I keep ending up having to thread through misc.py. But I guess that's out of scope for me right now.
Attachment #8372068 - Flags: review?(aki)
(Assignee)

Comment 24

4 years ago
Created attachment 8372069 [details] [diff] [review]
Add the rootanalysis mozconfig to the tree

You were right. The ANALYZED_OBJDIR was totally unnecessary. I removed it and everything's fine -- it builds into the default objdir but still deposits its output where I expect to find it.

Down to a very simple mozconfig now. I'm switching to using tooltool via a (large) set of other patches in this bug.
Attachment #8372069 - Flags: review?(mh+mozilla)
(Assignee)

Updated

4 years ago
Attachment #8369618 - Attachment is obsolete: true
Attachment #8369618 - Flags: review?(mh+mozilla)

Comment 25

4 years ago
Comment on attachment 8372068 [details] [diff] [review]
Pass through tooltool_url_list to mozharness

Sure. Looks like it's the same everywhere, so we're not gaining much.
Attachment #8372068 - Flags: review?(aki) → review+
(Assignee)

Updated

4 years ago
Attachment #8372068 - Flags: checked-in+
in production
Comment on attachment 8372069 [details] [diff] [review]
Add the rootanalysis mozconfig to the tree

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

I think this file should be in the linux64 directory.

::: browser/config/mozconfigs/hazards
@@ +6,5 @@
> +
> +# The hazard analysis build breaks with a plugin bug in gcc 4.7.3. Use $PATH to
> +# pass through the appropriate compiler.
> +unset CC
> +unset CXX

If you're using tooltool, your gcc 4.7.2 is going to have the same path as gcc 4.7.3 used on other linux builds. So keeping that CC/CXX is going to work.

@@ +8,5 @@
> +# pass through the appropriate compiler.
> +unset CC
> +unset CXX
> +
> +. "$topsrcdir/browser/config/mozconfig"

Not really needed (none of the other ones include this)
Attachment #8372069 - Flags: review?(mh+mozilla) → review+
(Assignee)

Updated

4 years ago
Attachment #8363295 - Flags: checked-in+
(Assignee)

Updated

4 years ago
Attachment #8372045 - Flags: checked-in+
https://hg.mozilla.org/mozilla-central/rev/0d48fce3ffa2
https://hg.mozilla.org/mozilla-central/rev/2603ed1bbfa1
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED

Comment 32

4 years ago
Mozharness change live in production.
(Assignee)

Updated

4 years ago
Depends on: 971852
(Assignee)

Comment 33

4 years ago
Backing out this whole mess for now.

http://hg.mozilla.org/build/mozharness/rev/0cf80abef167 - backed out f1062df29526
http://hg.mozilla.org/build/mozharness/rev/8183d22eee5a - backed out a17a46cd5d2e
(Assignee)

Comment 34

4 years ago
Created attachment 8375772 [details] [diff] [review]
Make hazard mozconfig avoid the stuff that breaks the analysis

I made some last-minute changes to the mozconfig to incorporate review comments, and really *thought* I'd tested them on my slave, but whatever I tested was not what landed.

Here's a followup patch that avoids specifying the compiler and hopefully documents why I'm being so nonconformist. I've also switched my mozharness to do things more like the in-tree mozconfigs expect (eg putting gcc in $topsrcdir/gcc instead of /tools/gcc), even though it'd probably work either way now with this mozconfig change. (Took me a while to figure out what was going wrong.)
Attachment #8375772 - Flags: review?(mh+mozilla)
(Assignee)

Comment 35

4 years ago
Created attachment 8375838 [details] [diff] [review]
Switch to in-tree mozconfig and new path for JS binary* * *

Ok, this one passes a full end-to-end test.

Note that this leaves in the 'privileged' boolean kwarg, but no longer uses it. I'm not sure if it's generally useful. Let me know if you'd rather I ripped it out; it's easy to do.
Attachment #8375838 - Flags: review?(aki)

Comment 36

4 years ago
Comment on attachment 8375838 [details] [diff] [review]
Switch to in-tree mozconfig and new path for JS binary* * *

>-    "exes": { 'hgtool.py': 'tools/buildfarm/utils/hgtool.py' },
>+    "exes": { 'hgtool.py': 'tools/buildfarm/utils/hgtool.py',
>+              'tooltool.py': '/tools/tooltool.py',
>+              },

Nit: we'll get PEP 8 warnings if there's a space after the initial {; if you can remove that space (and a corresponding space in each of the two following lines) that'll avoid the warning.

I think leaving 'privileged' is fine.
Attachment #8375838 - Flags: review?(aki) → review+
(Assignee)

Updated

4 years ago
Attachment #8363295 - Attachment is obsolete: true
Attachment #8363295 - Flags: checked-in+ → checked-in-
(Assignee)

Updated

4 years ago
Attachment #8372045 - Attachment is obsolete: true
Attachment #8372045 - Flags: checked-in+ → checked-in-
(Assignee)

Updated

4 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 8375772 [details] [diff] [review]
Make hazard mozconfig avoid the stuff that breaks the analysis

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

::: browser/config/mozconfigs/linux64/hazards
@@ +9,5 @@
> +
> +. "$topsrcdir/build/mozconfig.common"
> +ac_add_options --enable-elf-hack
> +ac_add_options --enable-stdcxx-compat
> +

Doesn't it work to augment CC and CXX instead of using a wrapper script?
Attachment #8375772 - Flags: review?(mh+mozilla) → review+
(Assignee)

Comment 38

4 years ago
(In reply to Mike Hommey [:glandium] from comment #37)
> Comment on attachment 8375772 [details] [diff] [review]
> Make hazard mozconfig avoid the stuff that breaks the analysis
> 
> Review of attachment 8375772 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/config/mozconfigs/linux64/hazards
> @@ +9,5 @@
> > +
> > +. "$topsrcdir/build/mozconfig.common"
> > +ac_add_options --enable-elf-hack
> > +ac_add_options --enable-stdcxx-compat
> > +
> 
> Doesn't it work to augment CC and CXX instead of using a wrapper script?

Yes, totally. But it would mean moving some of the wrapping logic into the build system instead of keeping it separate. Right now, it's set up to work with any build system with only PATH modification. And the logic seems to need to be adapted fairly frequently, so it's kind of nice to keep it as upstream as possible.

There's probably a happier middle road. CC="/abs/wrapper/script $CC" or something, as you suggested. It could probably live in parallel with the other wrapping mechanism. For now, though, this is tested and works. I'm willing to change it again in the future, but I've already spent a bunch of time adapting this to a changing environment already and need to get to some other stuff.

I'll file a bug for it.
(Assignee)

Updated

4 years ago
Blocks: 974006
(Assignee)

Updated

4 years ago
Attachment #8375838 - Flags: checked-in+
https://hg.mozilla.org/mozilla-central/rev/3afcb38ad947
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → B2G C4 (2jan on)
(Assignee)

Comment 41

4 years ago
Finally got around to landing this again. Let me see if I can get all the patch flags fixed up...

https://hg.mozilla.org/build/mozharness/rev/7abfed3a7c28
(Assignee)

Updated

4 years ago
Attachment #8371967 - Flags: checked-in+
(Assignee)

Updated

4 years ago
Attachment #8372069 - Flags: checked-in+
(Assignee)

Updated

4 years ago
Attachment #8375772 - Flags: checked-in+
(Assignee)

Updated

4 years ago
Blocks: 972089
You need to log in before you can comment on or make changes to this bug.