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

RESOLVED FIXED in Firefox 34

Status

defect
RESOLVED FIXED
5 years ago
a year ago

People

(Reporter: Ehsan, Assigned: glandium)

Tracking

(Blocks 1 bug)

unspecified
mozilla36
x86
macOS
Dependency tree / graph

Firefox Tracking Flags

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

Details

Attachments

(2 attachments)

Reporter

Description

5 years ago
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
Assignee

Comment 1

5 years ago
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!
Assignee

Comment 3

5 years ago
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.
Assignee

Comment 4

5 years ago
(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).
Assignee

Comment 5

5 years ago
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"
Assignee

Comment 6

5 years ago
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.
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Assignee

Comment 9

5 years ago
$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

Updated

5 years ago
Assignee: nobody → mh+mozilla
Status: NEW → ASSIGNED
Assignee

Comment 10

5 years ago
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+
Assignee

Updated

5 years ago
Blocks: 1059797
Assignee

Comment 12

5 years ago
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?
Assignee

Comment 13

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Does this impact ARMv6 builds? Should we take the fix on 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.
Assignee

Comment 21

5 years ago
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]
Assignee

Comment 22

5 years ago
(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)
Assignee

Comment 25

5 years ago
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

Updated

a year ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.