Closed Bug 889078 Opened 7 years ago Closed 7 years ago

Strip cpp tests prior to packaging

Categories

(Testing :: General, defect)

x86
All
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla25

People

(Reporter: nthomas, Assigned: dminor)

References

Details

Attachments

(1 file, 1 obsolete file)

Bug 811404 added cpp unittests to the test archives produced by RelEng buidbot systems, and the file sizes increased like this:
  linux32:  82 --> 220MB
  linux64:  85 --> 230MB
  mac:      69 --> 118MB
  win32:    77 -->  78MB

In bug 811404 comment #16 ted says we can (and should) strip the cpp tests to reduce the file size.
Try run here shows reduced test zip size: https://tbpl.mozilla.org/?tree=Try&rev=9bd12ff7f744.
Assignee: nobody → dminor
Status: NEW → ASSIGNED
Attachment #770260 - Flags: review?(ted)
Comment on attachment 770260 [details] [diff] [review]
Patch to run strip on cpp unit tests

Review of attachment 770260 [details] [diff] [review]:
-----------------------------------------------------------------

Punting to glandium because I'd like his input.
Attachment #770260 - Flags: review?(ted) → review?(mh+mozilla)
Comment on attachment 770260 [details] [diff] [review]
Patch to run strip on cpp unit tests

Review of attachment 770260 [details] [diff] [review]:
-----------------------------------------------------------------

::: config/rules.mk
@@ +171,5 @@
>  LIBS += $(XPCOM_GLUE_LDOPTS) $(NSPR_LIBS) $(MOZ_JS_LIBS) $(if $(JS_SHARED_LIBRARY),,$(MOZ_ZLIB_LIBS))
>  
>  ifndef MOZ_PROFILE_GENERATE
>  libs:: $(CPP_UNIT_TEST_BINS) $(call mkdir_deps,$(DIST)/cppunittests)
> +	$(STRIP) $(CPP_UNIT_TEST_BINS)

I'm pretty sure this will fail on systems where STRIP expands to nothing (e.g. Windows).
You might as well add -Wl,-S to the LDFLAGS for these files. This would make the build slightly faster, instead of making it slower.
Either way, the result is that the binaries won't have debugging symbols, which, while it may not matter much for the build infrastructure (although it might make some crash reports during build useless), it might become an inconvenience to developers.
So it would actually be better to do what the bug summary says: strip before packaging. And more specifically, objcopy before packaging (strip would strip the existing files, what you'd really want is to make a stripped copy while keeping the unstripped file around, and package the stripped copy).
Attachment #770260 - Flags: review?(mh+mozilla) → review-
Comment on attachment 771399 [details] [diff] [review]
Patch to use objcopy to strip cppunittests prior to packaging.

Review of attachment 771399 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/testsuite-targets.mk
@@ +494,4 @@
>  stage-cppunittests:
>  	$(NSINSTALL) -D $(PKG_STAGE)/cppunittests
> +ifdef OBJCOPY
> +	$(foreach bin,$(CPP_UNIT_TEST_BINS),$(OBJCOPY) --strip-unneeded $(bin) $(subst $(DIST),$(PKG_STAGE),$(bin));) 

trailing whitespace

I'd prefer if you'd use $(bin:$(DIST)/%=$(PKG_STAGE)/%)

@@ +495,5 @@
>  	$(NSINSTALL) -D $(PKG_STAGE)/cppunittests
> +ifdef OBJCOPY
> +	$(foreach bin,$(CPP_UNIT_TEST_BINS),$(OBJCOPY) --strip-unneeded $(bin) $(subst $(DIST),$(PKG_STAGE),$(bin));) 
> +else
> +	cp -RL $(DIST)/cppunittests $(PKG_STAGE) 

trailing whitespace
Attachment #771399 - Flags: review?(mh+mozilla) → review+
Linux test archives are 105MB now, thanks!
https://hg.mozilla.org/mozilla-central/rev/1769cad800db
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Depends on: 920055
You need to log in before you can comment on or make changes to this bug.