Closed
Bug 1268913
Opened 5 years ago
Closed 5 years ago
cppunittests not getting packaged on cross-mac builds
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(firefox49 fixed)
RESOLVED
FIXED
mozilla49
Tracking | Status | |
---|---|---|
firefox49 | --- | fixed |
People
(Reporter: ted, Assigned: wcosta)
References
Details
Attachments
(1 file, 1 obsolete file)
1.15 KB,
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
Something goofy is going on here: https://dxr.mozilla.org/mozilla-central/rev/6adc822f5e27a55551faeb6c47a9bd8b0859a23b/testing/testsuite-targets.mk#302 In this cross-mac build: https://tools.taskcluster.net/task-inspector/#b12N6ICXQH-MknDiVDlIIA/0 The cppunittests.zip is tiny. Looking at the log shows that the tests are built, but the packaging step seems to be doing the wrong thing: ``` 10:22:31 INFO - : -x -S dist/cppunittests/ShowAlignments dist/test-stage/cppunittest/ShowAlignments; ``` Apparently we're setting OBJCOPY to `:`, so STRIP_CPP_UNITTESTS gets set, but then obviously we can't execute it.
Assignee | ||
Comment 1•5 years ago
|
||
workspace/build/src/js/src/old-configure:if eval "test \"`echo '$''{'ac_cv_prog_OBJCOPY'+set}'`\" = set"; then workspace/build/src/js/src/old-configure: if test -n "$OBJCOPY"; then workspace/build/src/js/src/old-configure: ac_cv_prog_OBJCOPY="$OBJCOPY" # Let the user override the test. workspace/build/src/js/src/old-configure: ac_cv_prog_OBJCOPY="$ac_prog" workspace/build/src/js/src/old-configure:OBJCOPY="$ac_cv_prog_OBJCOPY" workspace/build/src/js/src/old-configure:if test -n "$OBJCOPY"; then workspace/build/src/js/src/old-configure: echo "$ac_t""$OBJCOPY" 1>&6 workspace/build/src/js/src/old-configure:test -n "$OBJCOPY" && break workspace/build/src/js/src/old-configure:test -n "$OBJCOPY" || OBJCOPY=":" -------> WTF?!?!?! workspace/build/src/js/src/old-configure: (''' OBJCOPY ''', r''' $OBJCOPY ''')
Assignee | ||
Comment 2•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0853d71e9a8c
Assignee | ||
Comment 3•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=017bcd6ce881
Assignee | ||
Comment 4•5 years ago
|
||
It feels like this is the guilty piece of code https://dxr.mozilla.org/mozilla-central/source/build/autoconf/toolchain.m4#102 Any particular reason to set semicolon as a default value?
Assignee | ||
Comment 6•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e63b927daa59
Assignee | ||
Comment 7•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=00eb7ddcb81a
Comment 8•5 years ago
|
||
It's a double edged sword: if you set it to nothing, then things doing `$(OBJDUMP) -option` blindly will be trying to execute `-option`. It's not always clear which is better.
Flags: needinfo?(mh+mozilla)
Comment 9•5 years ago
|
||
That said, it feels to me the real thing that should be happening is to set OBJCOPY to something that works, although that seems weird a cross mac build. What do normal mac builds use? objcopy?
Reporter | ||
Comment 10•5 years ago
|
||
Presumably nothing, because they don't really need to strip the binaries because the debug info doesn't get linked into them. The cctools we're using does have a `x86_64-apple-darwin10-strip` binary.
Reporter | ||
Comment 11•5 years ago
|
||
My local Mac build doesn't have OBJCOPY set in config.status, presumably this is a cross-compile quirk.
Assignee | ||
Comment 12•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2eb7499731c0
Assignee | ||
Comment 13•5 years ago
|
||
If objcopt is not found, it is intentionaly set to semicolon. We compare it to semicolon value for testing it for existence.
Assignee | ||
Updated•5 years ago
|
Assignee: nobody → wcosta
Status: NEW → ASSIGNED
Assignee | ||
Comment 14•5 years ago
|
||
Comment on attachment 8748165 [details] [diff] [review] Fix test for existence of objcopy. r=ted This is a simple patch to fix the issue (hopefully) without side effects.
Attachment #8748165 -
Flags: review?(ted)
Attachment #8748165 -
Flags: feedback?(mh+mozilla)
Reporter | ||
Comment 15•5 years ago
|
||
Comment on attachment 8748165 [details] [diff] [review] Fix test for existence of objcopy. r=ted Review of attachment 8748165 [details] [diff] [review]: ----------------------------------------------------------------- So we actually only use this definition in two places. Here, and in symbolstore.py (passed down in the buildsymbols target): https://dxr.mozilla.org/mozilla-central/source/Makefile.in#301 https://dxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/tools/symbolstore.py#848 You can just remove the colon from the check so that it winds up empty if it's missing, and things will be fine.
Attachment #8748165 -
Flags: review?(ted) → review-
Assignee | ||
Comment 16•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3d8d0ea30e5
Assignee | ||
Updated•5 years ago
|
Attachment #8748165 -
Attachment is obsolete: true
Attachment #8748165 -
Flags: feedback?(mh+mozilla)
Assignee | ||
Comment 17•5 years ago
|
||
If objcopy utility is not found, we set it to empty string so it can be tested with ifdef make statement.
Assignee | ||
Updated•5 years ago
|
Attachment #8748374 -
Flags: review?(mh+mozilla)
Comment 18•5 years ago
|
||
Comment on attachment 8748374 [details] [diff] [review] Set OBJCOPY to empty if not found. r=ted Review of attachment 8748374 [details] [diff] [review]: ----------------------------------------------------------------- ::: build/autoconf/toolchain.m4 @@ +98,5 @@ > AC_CHECK_PROGS(LD, "${TOOLCHAIN_PREFIX}ld", :) > AC_CHECK_PROGS(STRIP, "${TOOLCHAIN_PREFIX}strip", :) > AC_CHECK_PROGS(WINDRES, "${TOOLCHAIN_PREFIX}windres", :) > AC_CHECK_PROGS(OTOOL, "${TOOLCHAIN_PREFIX}otool", :) > +AC_CHECK_PROGS(OBJCOPY, "${TOOLCHAIN_PREFIX}objcopy") This going to hit us back in the head like a boomerang, because python configure's equivalent template sets it to a colon too. Looking at the code, it seems to me the ifdef OBJCPY in testing/testsuite-targets.mk can be entirely removed.
Attachment #8748374 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 19•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5923cd037057
Assignee | ||
Comment 20•5 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #18) > Comment on attachment 8748374 [details] [diff] [review] > Set OBJCOPY to empty if not found. r=ted > > Review of attachment 8748374 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: build/autoconf/toolchain.m4 > @@ +98,5 @@ > > AC_CHECK_PROGS(LD, "${TOOLCHAIN_PREFIX}ld", :) > > AC_CHECK_PROGS(STRIP, "${TOOLCHAIN_PREFIX}strip", :) > > AC_CHECK_PROGS(WINDRES, "${TOOLCHAIN_PREFIX}windres", :) > > AC_CHECK_PROGS(OTOOL, "${TOOLCHAIN_PREFIX}otool", :) > > +AC_CHECK_PROGS(OBJCOPY, "${TOOLCHAIN_PREFIX}objcopy") > > This going to hit us back in the head like a boomerang, because python > configure's equivalent template sets it to a colon too. > > Looking at the code, it seems to me the ifdef OBJCPY in > testing/testsuite-targets.mk can be entirely removed. Unfortunately it didn't work, as we not test for existance of OBJCOPY, it will always try to strip the binaries, even if objcopy is not there, resulting in the original bug. My original patch just admitted defeat and added a comparison to ':', but got r-, as setting OBJCOPY set empty string is neither a solution, I am out of ideas now ¯\_(ツ)_/¯. Unless we track the python files where it is defined to ':' and set it to empty string either. Do you know where is python configure located? "git grep" didn't help...
Flags: needinfo?(mh+mozilla)
Comment 21•5 years ago
|
||
AAAAhhhhh now that I actually read the code in testing/testsuite-targets.mk, I understand what's going on. It's not that they are being stripped, it's that they never end up in $(PKG_STAGE)/cppunittest in the first place. I'd rather go with the patch that was r-ed.
Flags: needinfo?(mh+mozilla)
Assignee | ||
Comment 22•5 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #21) > AAAAhhhhh now that I actually read the code in testing/testsuite-targets.mk, > I understand what's going on. It's not that they are being stripped, it's > that they never end up in $(PKG_STAGE)/cppunittest in the first place. I'd > rather go with the patch that was r-ed. deadlock. Let's wait :ted come back from PTO and see if we all come with an agreement :)
Reporter | ||
Comment 24•5 years ago
|
||
Comment on attachment 8748165 [details] [diff] [review] Fix test for existence of objcopy. r=ted Review of attachment 8748165 [details] [diff] [review]: ----------------------------------------------------------------- If that's what glandium wants, fine with me.
Attachment #8748165 -
Flags: review- → review+
Assignee | ||
Updated•5 years ago
|
Attachment #8748374 -
Attachment is obsolete: true
Assignee | ||
Updated•5 years ago
|
Attachment #8748165 -
Attachment is obsolete: false
Assignee | ||
Comment 25•5 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/701b27c4e27fa54b542064a170832465f15e7daf Bug 1268913: Fix test for existence of objcopy. r=ted
Comment 26•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/701b27c4e27f
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Reporter | ||
Updated•5 years ago
|
Flags: needinfo?(ted)
Updated•3 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•