Closed
Bug 1243233
Opened 9 years ago
Closed 9 years ago
Upgrade mulet compiler from gcc 4.7.3 to gcc 4.9.3
Categories
(Core :: JavaScript: GC, defect)
Core
JavaScript: GC
Tracking
()
RESOLVED
FIXED
mozilla47
Tracking | Status | |
---|---|---|
firefox47 | --- | fixed |
People
(Reporter: sfink, Assigned: sfink)
References
Details
Attachments
(2 files, 5 obsolete files)
(deleted),
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
The hazard analysis would really like to move to using the same compiler as other builds, and my latest version of the plugin really needs gcc 4.9, so it seems like a good time to upgrade mulet to gcc 4.9 for the b2g hazard builds at least.
Assignee | ||
Comment 1•9 years ago
|
||
I have a very ugly try push https://treeherder.mozilla.org/#/jobs?repo=try&revision=745bab0a287c that should say whether this horribly breaks anything. So far, it has only gotten to failing everything Windows because of the new version of pip (which I really thought I had rebased onto the fix for, but perhaps that's another tree.)
I guess I'll need to either upgrade the browser to 4.9 as well, or continue using my own compiler for the browser hazard job.
Attachment #8712454 -
Flags: review?(mh+mozilla)
Comment 2•9 years ago
|
||
Comment on attachment 8712454 [details] [diff] [review]
Upgrade mulet compiler from gcc 4.7.3 to gcc 4.9.1
Review of attachment 8712454 [details] [diff] [review]:
-----------------------------------------------------------------
Please use at least GCC 4.9.3. See bug 1029245.
Attachment #8712454 -
Flags: review?(mh+mozilla) → review-
Assignee | ||
Comment 3•9 years ago
|
||
This is a fix to NSS that I need in order to upgrade to gcc 4.9.3. It's really a bugfix for the change from bug 1216318, which glandium pointed out was incorrect.
Attachment #8713762 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 4•9 years ago
|
||
Actually do the upgrade. I haven't done a full try run on this latest set of patches yet, but an earlier one with more hackish stuff was green: https://treeherder.mozilla.org/#/jobs?repo=try&author=sfink%40mozilla.com
Attachment #8713763 -
Flags: review?(mh+mozilla)
Assignee | ||
Updated•9 years ago
|
Attachment #8712454 -
Attachment is obsolete: true
Updated•9 years ago
|
Summary: Upgrade mulet compiler from gcc 4.7.3 to gcc 4.9.1 → Upgrade mulet compiler from gcc 4.7.3 to gcc 4.9.3
Comment 5•9 years ago
|
||
Comment on attachment 8713762 [details] [diff] [review]
Test ALLOW_COMPILER_WARNINGS instead of WARNINGS_AS_ERRORS, and move it to after it is set
Review of attachment 8713762 [details] [diff] [review]:
-----------------------------------------------------------------
::: config/external/nss/Makefile.in
@@ +340,5 @@
>
> include $(topsrcdir)/config/rules.mk
>
> +ifeq ($(OS_TARGET),Android)
> +ALLOW_COMPILER_WARNINGS := 1
This is already set in moz.build.
Attachment #8713762 -
Flags: review?(mh+mozilla) → feedback+
Comment 6•9 years ago
|
||
Comment on attachment 8713763 [details] [diff] [review]
Upgrade mulet compiler from gcc 4.7.3 to gcc 4.9.3
Review of attachment 8713763 [details] [diff] [review]:
-----------------------------------------------------------------
::: b2g/dev/config/tooltool-manifests/linux64/releng.manifest
@@ +40,5 @@
> +{
> +"size": 2552852,
> +"digest": "299ab5058f060610a962d3379338f27a63161435909f4fa5559711a18d274f39aa9e48d1be993ea6eb94cd6f566e817f84f799d783b8f3aa98482e62f785f0c1",
> +"algorithm": "sha512",
> +"filename": "sixgill.tar.xz",
How is this built?
Attachment #8713763 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 8•9 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #6)
> Comment on attachment 8713763 [details] [diff] [review]
> Upgrade mulet compiler from gcc 4.7.3 to gcc 4.9.3
>
> Review of attachment 8713763 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: b2g/dev/config/tooltool-manifests/linux64/releng.manifest
> > +"filename": "sixgill.tar.xz",
>
> How is this built?
With a script included in my fork of the sixgill repo. I just cleaned up my patch series and uploaded it all to http://hg.mozilla.org/users/sfink_mozilla.com/sixgill
I maintain the sixgill used in the hazard analysis. Sixgill is from Brian Hackett <bhackett@mozilla.com> and I have him review the stuff I am unsure of. Mostly, I try to keep it more or less working with gcc changes and occasional breakage from new constructs used in gecko code.
Assignee | ||
Comment 9•9 years ago
|
||
Good, that makes it nicer.
Attachment #8717247 -
Flags: review?(mh+mozilla)
Assignee | ||
Updated•9 years ago
|
Attachment #8713762 -
Attachment is obsolete: true
Updated•9 years ago
|
Attachment #8717247 -
Flags: review?(mh+mozilla) → review+
Comment 10•9 years ago
|
||
Comment 11•9 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Assignee | ||
Comment 12•9 years ago
|
||
glandium: see comment 8 for how this was built. This new version of sixgill is built with 5b7f97c1b547 from http://hg.mozilla.org/users/sfink_mozilla.com/sixgill
I have tested it with a new taskcluster job and it works, except that it finds a new hazard. (And it's a nasty one, but it's a false positive. I have a very explicit mechanism to avoid it that I guess I'll need to land.)
Attachment #8717964 -
Flags: review?(mh+mozilla)
Assignee | ||
Updated•9 years ago
|
Attachment #8713763 -
Attachment is obsolete: true
Assignee | ||
Comment 13•9 years ago
|
||
Oops. Forgot [leave-open].
Assignee | ||
Comment 14•9 years ago
|
||
Comment on attachment 8717964 [details] [diff] [review]
Upgrade mulet compiler from gcc 4.7.3 to gcc 4.9.3
Cancelling review request for now. I need to build this in a proper environment that doesn't have stuff that won't be in the docker image. (Such as, say, the docker image.)
Attachment #8717964 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 15•9 years ago
|
||
Ok, this one is working for me. I required an additional change to be compatible with the taskcluster browser hazard build, because its docker container uses a hostname that does not resolve, which confused the server that collects analysis results. I made it fall back to using 'localhost'.
Admittedly, I've only tested this one so far with the regular browser, not mulet. But I'm not asking for review of the correctness of sixgill anyway, just for things like are you ok with having sixgill fetched by tooltool for all builds, not just hazard builds (it'll never be called by other builds, and it's tiny in comparison to the other stuff.)
Attachment #8718159 -
Flags: review?(mh+mozilla)
Assignee | ||
Updated•9 years ago
|
Attachment #8717964 -
Attachment is obsolete: true
Comment 16•9 years ago
|
||
Comment on attachment 8718159 [details] [diff] [review]
Upgrade mulet compiler from gcc 4.7.3 to gcc 4.9.3
Review of attachment 8718159 [details] [diff] [review]:
-----------------------------------------------------------------
r=me with visibility removed.
::: js/src/devtools/rootAnalysis/build/sixgill.manifest
@@ +8,4 @@
> "filename" : "sixgill.tar.xz",
> + "size" : 2684108,
> + "unpack" : true,
> + "visibility" : "public"
visibility is only useful for uploads.
Attachment #8718159 -
Flags: review?(mh+mozilla) → review+
Comment 17•9 years ago
|
||
Comment on attachment 8718159 [details] [diff] [review]
Upgrade mulet compiler from gcc 4.7.3 to gcc 4.9.3
Review of attachment 8718159 [details] [diff] [review]:
-----------------------------------------------------------------
Actually. Why add sixgill on normal mulet builds?
::: js/src/devtools/rootAnalysis/build/sixgill.manifest
@@ +8,4 @@
> "filename" : "sixgill.tar.xz",
> + "size" : 2684108,
> + "unpack" : true,
> + "visibility" : "public"
visibility is only useful for uploads.
Attachment #8718159 -
Flags: review+
Assignee | ||
Comment 18•9 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #17)
> Comment on attachment 8718159 [details] [diff] [review]
> Upgrade mulet compiler from gcc 4.7.3 to gcc 4.9.3
>
> Review of attachment 8718159 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> Actually. Why add sixgill on normal mulet builds?
Only because it's easiest. The sixgill download is relatively small (<3MB) and does no harm just sitting there. The alternative would be to either duplicate releng.manifest for the analysis, or run tooltool twice with an additional manifest. The latter is a somewhat larger change to the build scripts (build-mulet-linux.sh calls desktop-setup.sh calls install-packages.sh). But it's totally doable if you'd prefer to avoid the spurious download.
>
> ::: js/src/devtools/rootAnalysis/build/sixgill.manifest
> @@ +8,4 @@
> > "filename" : "sixgill.tar.xz",
> > + "size" : 2684108,
> > + "unpack" : true,
> > + "visibility" : "public"
>
> visibility is only useful for uploads.
Oops, missed another one. My current patch stack does not contain this. (Though my current patch stack no longer uses this particular file at all, but it'll require some more reviews.) My automation for rebuilding sixgill and re-uploading inserts this.
Assignee | ||
Comment 19•9 years ago
|
||
Actually, doing a separate tooltool run turns out to be quite easy if you don't mind setting TOOLTOOL_MANIFEST and re-rerunning the install-packages.sh script, and I kind of like using it that way. (I'd probably rather have it define a shell function and then have the calling script invoke the function with the proper manifest, but this is good enough.)
It does mean that whenever the compiler version is different between the analysis, firefox browser, and mulet, then I'll need to use a separate sixgill.manifest file. But for now I'd like to move towards getting them on a single one, and forking the file later when it's needed.
Attachment #8722147 -
Flags: review?(mh+mozilla)
Assignee | ||
Updated•9 years ago
|
Attachment #8718159 -
Attachment is obsolete: true
Comment 20•9 years ago
|
||
Comment on attachment 8722147 [details] [diff] [review]
Upgrade mulet compiler from gcc 4.7.3 to gcc 4.9.3,
Review of attachment 8722147 [details] [diff] [review]:
-----------------------------------------------------------------
I'm not sure why the compiler has to be updated for non hazard mulet builds but meh.
Attachment #8722147 -
Flags: review?(mh+mozilla) → review+
Comment 21•9 years ago
|
||
Comment 22•9 years ago
|
||
bugherder |
Comment 23•9 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #20)
> Comment on attachment 8722147 [details] [diff] [review]
> Upgrade mulet compiler from gcc 4.7.3 to gcc 4.9.3,
>
> Review of attachment 8722147 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> I'm not sure why the compiler has to be updated for non hazard mulet builds
> but meh.
You didn't answer to this.
Assignee | ||
Comment 24•9 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #23)
> (In reply to Mike Hommey [:glandium] from comment #20)
> > Comment on attachment 8722147 [details] [diff] [review]
> > Upgrade mulet compiler from gcc 4.7.3 to gcc 4.9.3,
> >
> > Review of attachment 8722147 [details] [diff] [review]:
> > -----------------------------------------------------------------
> >
> > I'm not sure why the compiler has to be updated for non hazard mulet builds
> > but meh.
>
> You didn't answer to this.
It didn't sound like a question. "meh" sounded like you didn't see a reason for it but didn't mind it happening.
There was no strong reason for it. It meant I didn't need to have a separate manifest for mulet hazard browser builds vs mulet browser builds. It also matches where all of the other b2g builds are going right now; see bug 1087161. That could be done in a separate bug, using a separate manifest file, by someone else at a later time.
So if you'd rather I not touch the non-hazard build, let me know, and I'll do a followup patch that creates a separate manifest for the hazard build and reverts regular mulet to gcc 4.7.3 for now. It does mean having more total configurations to manage, but that's your call.
Assignee | ||
Comment 25•9 years ago
|
||
Oops, another leave-open leavebehind.
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
Keywords: leave-open
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•