Closed Bug 1091118 Opened 10 years ago Closed 10 years ago

non-clobber build failure related to UniquePtr's copy ctor

Categories

(Firefox Build System :: General, defect)

Product:
Firefox Build System
Component:
General
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox33 wontfix, firefox34 fixed, firefox35 fixed, firefox36 fixed, firefox-esr31 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla36
Tracking Flags:
Tracking Status
firefox33 --- wontfix
firefox34 --- fixed
firefox35 --- fixed
firefox36 --- fixed
firefox-esr31 --- fixed

People

(Reporter: ehsan.akhgari, Assigned: glandium)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

part 1 - Remove $topsrcdir/gcc/bin from PATH on android builds
1.24 KB, patch
gps
: review+
lmandel
: approval-mozilla-aurora+
lmandel
: approval-mozilla-beta+
lmandel
: approval-mozilla-esr31+
Details | Diff | Splinter Review
part 2 - Do not use the top-level cache file for freetype2 subconfigure
2.62 KB, patch
gps
: review+
lmandel
: approval-mozilla-aurora+
lmandel
: approval-mozilla-beta+
lmandel
: approval-mozilla-esr31+
Details | Diff | Splinter Review 
We hit a build bustage today on inbound that went away with clobbering:

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=52ed8be78a65

Example log:

https://tbpl.mozilla.org/php/getParsedLog.php?id=51336403&tree=Mozilla-Inbound&full=1

../../dist/include/mozilla/UniquePtr.h:490: error: 'mozilla::UniquePtr::UniquePtr(const mozilla::UniquePtr&) [with T = char, D = mozilla::DefaultDelete]' is private
/builds/slave/m-in-and-000000000000000000000/build/mozglue/linker/Zip.h:73: error: within this context
../../dist/include/mozilla/UniquePtr.h:490: error: deleted function 'mozilla::UniquePtr::UniquePtr(const mozilla::UniquePtr&) [with T = char, D = mozilla::DefaultDelete]'
/builds/slave/m-in-and-000000000000000000000/build/mozglue/linker/Zip.h:73: error: used here
../../dist/include/mozilla/UniquePtr.h:490: error: 'mozilla::UniquePtr::UniquePtr(const mozilla::UniquePtr&) [with T = char, D = mozilla::DefaultDelete]' is private
/builds/slave/m-in-and-000000000000000000000/build/mozglue/linker/Zip.h:73: error: within this context
../../dist/include/mozilla/UniquePtr.h:490: error: deleted function 'mozilla::UniquePtr::UniquePtr(const mozilla::UniquePtr&) [with T = char, D = mozilla::DefaultDelete]'
/builds/slave/m-in-and-000000000000000000000/build/mozglue/linker/Zip.h:73: error: used here
make[5]: *** [host_SeekableZStream.o] Error 1
make[4]: *** [mozglue/linker/host] Error 2
make[4]: *** Waiting for unfinished jobs....
make[3]: *** [compile] Error 2
make[2]: *** [default] Error 2
make[1]: *** [realbuild] Error 2
make: *** [build] Error 2
The error makes sense. What doesn't make sense is that it doesn't happen all the time.
(In reply to Mike Hommey [:glandium] from comment #1)
> The error makes sense. What doesn't make sense is that it doesn't happen all
> the time.

That's exactly what we said!
So, one part of the problem is that bug 971841 added $topsrcdir/gcc/bin to $PATH (although it's unrelated to ant), and there actually may or may not be something there depending on what previous build happened on the slave.

Another part of the problem is that for some reason, "c++" is the host compiler configure got from config.cache, instead of /tools/gcc-4.7.2-0moz1/bin/g++.

So the version of gcc used to build host tools depends on what happened on the build slaves in the past.
(In reply to Mike Hommey [:glandium] from comment #1)
> The error makes sense.

Actually, I'll retract that. The error only makes sense if the compiler doesn't support implicit move constructors properly, which is probably the case of "c++" in some cases (which may barely support C++11 at all in the worst case scenario on our build slaves).
Reliable way to trigger the error on try:

diff --git a/mobile/android/config/mozconfigs/common b/mobile/android/config/mozconfigs/common
--- a/mobile/android/config/mozconfigs/common
+++ b/mobile/android/config/mozconfigs/common
@@ -39,17 +39,17 @@ ac_add_options --enable-warnings-as-erro
 ac_add_options --with-mozilla-api-keyfile=/builds/mozilla-api.key
 
 # Package js shell.
 export MOZ_PACKAGE_JSSHELL=1
 
 # Use ccache
 . "$topsrcdir/build/mozconfig.cache"
 
-HOST_CC="/tools/gcc-4.7.2-0moz1/bin/gcc"
-HOST_CXX="/tools/gcc-4.7.2-0moz1/bin/g++"
+HOST_CC=cc
+HOST_CXX=c++
 
 # Avoid dependency on libstdc++ 4.7
 ac_add_options --enable-stdcxx-compat
 
 mk_add_options "export ANT_HOME=$topsrcdir/apache-ant"
 
-mk_add_options "export PATH=$topsrcdir/gcc/bin:$topsrcdir/apache-ant/bin:$PATH"
+mk_add_options "export PATH=$topsrcdir/apache-ant/bin:$PATH"
Example of a series of event on inbound that can end up in the same situation is as follows:
- build A is a clobber
- toplevel configure picks /tools/gcc-4.7.2-0moz1/bin/g++ as HOST_CXX
- this happens before entering js/src's configure:
     Removing /builds/slave/m-in-and-000000000000000000000/build/obj-firefox/config.cache because of CPPFLAGS value change from:
      -idirafter /builds/slave/m-in-and-000000000000000000000/build/android-ndk/platforms/android-9/arch-arm/usr/include  -g
     to:
  
- js/src configure picks c++ as HOST_CXX and stores that in config.cache
- build B, on the same slave is incremental
- toplevel configure picks c++ as HOST_CXX from config.cache
- the build succeeds if there's a gcc/bin leftover from a previous linux build on the same slave, fails otherwise.

There are two things to fix here:
- PATH, which would turn the error as a perma red on incremental builds
- The resetting of config.cache when entering js/src.
$topsrcdir/gcc/bin was mistakenly added in bug 971841, but is not provided by
anything the tooltool manifest for android builds pulls. It however is a path
that /may/ exist in the source tree when the slave ran a linux build before.
When it does exist, the meaning of non-path-prefixed commands change,
influencing what particular compiler is used in some cases.
Attachment #8514033 - Flags: review?(gps)
Assignee: nobody → mh+mozilla
Status: NEW → ASSIGNED
We modify the environment before running freetype2 configure. When it uses
the same cache file, it stores knowledge about that environment in the cache
file. The cache file is then reused to configure in js/src, with yet again a
different environment, which makes subconfigure.py clear the cache because
of the differences.

The configure in js/src is however invoked with the same environment as the
main configure was invoked with (mostly), so without freetype2 on the way,
reusing the cache for it works as expected. In fact, it works better with the
cache because of things coming from mozconfig that are not exported.

With freetype2 on the way, as mentioned above, the cache is cleared. Without
the cache, js/src/configure does new detections with a possibly different
environment, and stores that in the cache. Until the next build, which then
uses that different cache for the top-level configure.

This results in subtle differences in the HOST_CC/HOST_CXX variables on
android builds because those variables are not exported from mozconfig,
depending on PATH, what the builder was building before, and if the build
is a clobber.

Avoiding the freetype2 subconfigure writing its environment variables change
to the top-level cache makes the cache never invalidate for js/src.
Attachment #8514034 - Flags: review?(gps)
Attachment #8514033 - Flags: review?(gps) → review+
Attachment #8514034 - Flags: review?(gps) → review+
Blocks: 1059797
Comment on attachment 8514033 [details] [diff] [review]
part 1 - Remove $topsrcdir/gcc/bin from PATH on android builds

Approval Request Comment
[Feature/regressing bug #]: Unexpected change from bug 971841
[User impact if declined]: Makes the build randomly pick a different compiler for "host" files on android builds depending on what the build slave has been doing beforehand.
[Describe test coverage new/current, TBPL]: Landed on m-i today.
[Risks and why]: Low
[String/UUID change made/needed]: None
Attachment #8514033 - Flags: approval-mozilla-beta?
Attachment #8514033 - Flags: approval-mozilla-aurora?
Comment on attachment 8514034 [details] [diff] [review]
part 2 - Do not use the top-level cache file for freetype2 subconfigure

Approval Request Comment
[User impact if declined]: Bug 1059797 breaks incremental builds without this change.
[Describe test coverage new/current, TBPL]: Landed on m-i today. Requires clobber to actually unbreak builders in the broken state, but besides that, works as expected.
[Risks and why]: Low.
[String/UUID change made/needed]: None.
Attachment #8514034 - Flags: approval-mozilla-beta?
Attachment #8514034 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/ed34888e6a48
https://hg.mozilla.org/mozilla-central/rev/0fe6218eec33
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Does this impact ARMv6 builds? Should we take the fix on ESR31?
status-firefox33: --- → wontfix
status-firefox34: --- → affected
status-firefox35: --- → affected
status-firefox36: --- → affected
status-firefox-esr31: --- → ?
Flags: needinfo?(mh+mozilla)
Comment on attachment 8514033 [details] [diff] [review]
part 1 - Remove $topsrcdir/gcc/bin from PATH on android builds

Beta+
Aurora+
Attachment #8514033 - Flags: approval-mozilla-beta?
Attachment #8514033 - Flags: approval-mozilla-beta+
Attachment #8514033 - Flags: approval-mozilla-aurora?
Attachment #8514033 - Flags: approval-mozilla-aurora+
Comment on attachment 8514034 [details] [diff] [review]
part 2 - Do not use the top-level cache file for freetype2 subconfigure

Beta+
Aurora+
Attachment #8514034 - Flags: approval-mozilla-beta?
Attachment #8514034 - Flags: approval-mozilla-beta+
Attachment #8514034 - Flags: approval-mozilla-aurora?
Attachment #8514034 - Flags: approval-mozilla-aurora+
(In reply to Carsten Book [:Tomcat] from comment #14)

> https://hg.mozilla.org/mozilla-central/rev/0fe6218eec33

This changeset broke local builds for me on OSX (backing it out fixed the issue). Here is the error I got:
http://pastebin.mozilla.org/7007614

Perhaps there's just something wrong with my build environment? This is my mozconfig:
http://pastebin.mozilla.org/7007742
gwagner was also complaining about broken builds on #b2g because of this.
Followup for the mac issue.
https://hg.mozilla.org/integration/mozilla-inbound/rev/881ed839d891

Can someone land this on beta and aurora?
Flags: needinfo?(mh+mozilla)
Whiteboard: [checkin-needed-aurora][checkin-needed-beta]
(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #15)
> Does this impact ARMv6 builds? Should we take the fix on ESR31?

I'd tend to say yes.
(In reply to Mike Hommey [:glandium] from comment #22)
> (In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #15)
> > Does this impact ARMv6 builds? Should we take the fix on ESR31?
> 
> I'd tend to say yes.

Both patches?
Flags: needinfo?(mh+mozilla)
All three of them (the two attached and the followup)
Flags: needinfo?(mh+mozilla)
Comment on attachment 8514033 [details] [diff] [review]
part 1 - Remove $topsrcdir/gcc/bin from PATH on android builds

Approving for ESR31 based on comment 25. As Glandium notes, ESR31 needs the 3 patches landed in this bug.
Attachment #8514033 - Flags: approval-mozilla-esr31+
Attachment #8514034 - Flags: approval-mozilla-esr31+
Depends on: 1092292
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: