cppunittests not getting packaged on cross-mac builds

RESOLVED FIXED in Firefox 49

Status

Product:
Firefox Build System
Component:
General
Type:
defect
Status:
RESOLVED FIXED
Opened:
3 years ago
Updated:
Last year

People

(Reporter: ted, Assigned: wcosta)

Tracking

Version:
unspecified
Target:
mozilla49
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Fix test for existence of objcopy. r=ted
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.
Blocks: 1244150
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 ''')
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?
Forgot to NI...
Flags: needinfo?(mh+mozilla)
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)
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?
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.
My local Mac build doesn't have OBJCOPY set in config.status, presumably this is a cross-compile quirk.
If objcopt is not found, it is intentionaly set to semicolon. We compare
it to semicolon value for testing it for existence.
Assignee: nobody → wcosta
Status: NEW → ASSIGNED
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)
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-
Attachment #8748165 - Attachment is obsolete: true
Attachment #8748165 - Flags: feedback?(mh+mozilla)
If objcopy utility is not found, we set it to empty string so it can be
tested with ifdef make statement.
Attachment #8748374 - Flags: review?(mh+mozilla)
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)
(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)
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)
(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 :)
Ted - can you weigh in? Thanks.
Flags: needinfo?(ted)
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+
Attachment #8748374 - Attachment is obsolete: true
Attachment #8748165 - Attachment is obsolete: false
https://hg.mozilla.org/mozilla-central/rev/701b27c4e27f
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Flags: needinfo?(ted)
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.