889078 - Strip cpp tests prior to packaging

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[Nick Thomas [:nthomas] (UTC+12)]

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.

Comment 1

[Dan Minor [:dminor]]

Attachment: Patch to run strip on cpp unit tests

Try run here shows reduced test zip size: https://tbpl.mozilla.org/?tree=Try&rev=9bd12ff7f744.

Comment 2

[(not currently active) Ted Mielczarek]

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.

Comment 3

[Mike Hommey [:glandium]]

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).

Comment 4

[Dan Minor [:dminor]]

Attachment: Patch to use objcopy to strip cppunittests prior to packaging.

Try run here: https://tbpl.mozilla.org/?tree=Try&rev=c884e632f5ad

Comment 5

[Mike Hommey [:glandium]]

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

Comment 6

[Dan Minor [:dminor]]

With comments addressed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/1769cad800db

Comment 7

[Nick Thomas [:nthomas] (UTC+12)]

Linux test archives are 105MB now, thanks!

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1769cad800db