549427 - tests tarball should be zip files

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[John Ford [:jhford] CET/CEST Berlin Time]

We are looking at running unit test on our talos machines.  It is a bit of a pain to get tar and bzip2 running on the slaves without using cygwin, which we don't want on our talos slaves.

Comment 1

[John Ford [:jhford] CET/CEST Berlin Time]

i am thinking that the relevant line of code is http://mxr.mozilla.org/mozilla-central/source/testing/testsuite-targets.mk#134

Comment 2

[(not currently active) Ted Mielczarek]

Yes, that's the only place that would need changing in the build system. Presumably you'll also have to fix the buildbot code that looks for the test package to download it for test runs.

Comment 3

[John Ford [:jhford] CET/CEST Berlin Time]

Attachment: potential patch

I am not sure if i have the ifeq at the proper indentation or if i am detecting windows in the proper method

Comment 4

[John Ford [:jhford] CET/CEST Berlin Time]

Comment on attachment 429597 [details] [diff] [review]
potential patch

>+ifeq ($(OS),Windows_NT)
>+	@(cd $(PKG_STAGE) && zip -r "$(DIST)/$(PKG_PATH)$(TEST_PACKAGE)" *
>+else
> 	@(cd $(PKG_STAGE) && tar $(TAR_CREATE_FLAGS) - *) | bzip2 -f > "$(DIST)/$(PKG_PATH)$(TEST_PACKAGE)"
>-
>+endif # test OS Windows_NT

Just noticed that i had left out the recursion option for zip

Comment 5

[(not currently active) Ted Mielczarek]

Is there any reason not to just use zip on all platforms? We already do that with the symbol package.

Comment 6

[John Ford [:jhford] CET/CEST Berlin Time]

i thought that there was a preference to keep them as tarballs.  It would be easier on our end to not special case windows when extracting tests.

Comment 7

[(not currently active) Ted Mielczarek]

There's no preference, I arbitrarily chose .tar.bz2 when I wrote that code. If you want zip files, just switch it to zip files.

Comment 8

[(not currently active) Ted Mielczarek]

Comment on attachment 429597 [details] [diff] [review]
potential patch

>diff --git a/testing/testsuite-targets.mk b/testing/testsuite-targets.mk
>--- a/testing/testsuite-targets.mk
>+++ b/testing/testsuite-targets.mk
>@@ -122,26 +122,29 @@
> include $(topsrcdir)/toolkit/mozapps/installer/package-name.mk
> 
> ifndef UNIVERSAL_BINARY
> PKG_STAGE = $(DIST)/test-package-stage
> package-tests: stage-mochitest stage-reftest stage-xpcshell stage-jstests
> else
> # This staging area has been built for us by universal/flight.mk
> PKG_STAGE = $(DIST)/universal/test-package-stage
> endif
> 
> package-tests:
> 	$(NSINSTALL) -D $(DIST)/$(PKG_PATH)
>+ifeq ($(OS),Windows_NT)
>+	@(cd $(PKG_STAGE) && zip "$(DIST)/$(PKG_PATH)$(TEST_PACKAGE)" *

You want zip -r9D.

>+else
> 	@(cd $(PKG_STAGE) && tar $(TAR_CREATE_FLAGS) - *) | bzip2 -f > "$(DIST)/$(PKG_PATH)$(TEST_PACKAGE)"
>-
>+endif # test OS Windows_NT

Just use zip on all platforms, there's no reason to do otherwise.

Comment 9

[John Ford [:jhford] CET/CEST Berlin Time]

Attachment: zip tests instead of tar+bzip2

Comment 10

[(not currently active) Ted Mielczarek]

Comment on attachment 430653 [details] [diff] [review]
zip tests instead of tar+bzip2

Can't land this until you ensure that the build automation will handle zip packages instead of .tar.bz2.

Comment 11

[John Ford [:jhford] CET/CEST Berlin Time]

Attachment: updated patch

I feel bad, but I don't think that I tested that patch well enough before asking for review, sorry Ted!

I found that an unfortunate side effect of the patch was that instead of getting a zip file of the packaged tests, I was zipping the entire objdir/dist directory.  I have two approaches to fix this, 

package-tests:
	$(NSINSTALL) -D $(DIST)/$(PKG_PATH)
	@rm -f "$(DIST)/$(PKG_PATH)$(TEST_PACKAGE)"
	@TEMP_WD=$(PWD)
	@cd $(PKG_STAGE)
	zip -r9D "$(PKG_PATH)$(TEST_PACKAGE)" *
	@cd $(TEMP_WD)
	@mv $(PKG_STAGE)/$(PKG_PATH)$(TEST_PACKAGE) $(DIST)

or

package-tests:
	$(NSINSTALL) -D $(DIST)/$(PKG_PATH)
	@rm -f "$(DIST)/$(PKG_PATH)$(TEST_PACKAGE)"
	cd $(PKG_STAGE) && \
	  zip -r9D "../$(PKG_PATH)$(TEST_PACKAGE)" *

Based on http://mxr.mozilla.org/mozilla-central/source/Makefile.in#192 it looks like the second approach would follow existing conventions and is the approach that I think this patch should take.  The rm -f is to ensure that the archive is recreated from scratch.  From my reading of the man page, the default action is to update the archive not to overwrite.

This patch is going to have to land on all branches at the same time but only after our automation is prepared for this patch.  Who would I ask for approval to get this patch landed in such a fashion?

Comment 12

[(not currently active) Ted Mielczarek]

The latter sounds like the correct approach, yes. Um, I don't know, we don't normally do coordinated branch landings like that. Ask Beltzner?

Comment 13

[(not currently active) Ted Mielczarek]

Comment on attachment 431770 [details] [diff] [review]
updated patch

Looks fine. You'll have to sort out the branch landing situation yourself, ask drivers. Alternately, if you can figure out a way to make the build automation handle both cases, you can land this on trunk and ask for approval later.

Comment 14

[alice nodelman [:alice] [:anode]]

Is this just waiting on a downtime plan?

Comment 15

[John Ford [:jhford] CET/CEST Berlin Time]

(In reply to comment #14)
> Is this just waiting on a downtime plan?

This is waiting on the automation changes to use explicit filenames in the sendchanges.  I am going to be working on this today/tomorrow

Comment 16

[John Ford [:jhford] CET/CEST Berlin Time]

Comment on attachment 431770 [details] [diff] [review]
updated patch

This is infrastructure only and required for us to do unit test runs on our talos pool of slaves.

Comment 17

[John Ford [:jhford] CET/CEST Berlin Time]

unmarking approval*? because I need to land this on mozilla-central before I ask for branch approval.

Comment 18

[(not currently active) Ted Mielczarek]

Pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/7886dc6ae0c8

Comment 19

[(not currently active) Ted Mielczarek]

Backed out:
http://hg.mozilla.org/mozilla-central/rev/f843e0928e11

John says there's a problem with the buildbot side of things.

Comment 20

[John Ford [:jhford] CET/CEST Berlin Time]

Attachment: updated patch

Is it ok to carry the r+ from the previous patch?  This update is for bitrot.

Comment 21

[(not currently active) Ted Mielczarek]

As long as you don't materially change the meaning of the patch it's fine to carry forward review.

(I merged your patch when I committed it to accommodate.)

Comment 22

[John Ford [:jhford] CET/CEST Berlin Time]

Thanks :)

in which case, could you please land it?

Comment 23

[[e-mail]]

Comment on attachment 437716 [details] [diff] [review]
updated patch

http://hg.mozilla.org/mozilla-central/rev/1b2e5a9c5214

Comment 24

[John Ford [:jhford] CET/CEST Berlin Time]

Comment on attachment 437716 [details] [diff] [review]
updated patch

Each branch is going to have to have this patch landed to have unit tests run on hardware.

Comment 25

[[e-mail] (:bhearsum)]

Comment on attachment 437716 [details] [diff] [review]
updated patch

This patch completely broke uploading of mac test packages. Removing approval? flags because of that.

Comment 26

[[e-mail] (:bhearsum)]

Had to land this patch as a bustage fix: http://hg.mozilla.org/mozilla-central/rev/cdb18820629c

Comment 27

[John Ford [:jhford] CET/CEST Berlin Time]

Attachment: mozilla-1.9.2 version of the patch

This has been baking on Mozilla-Central for a while now.  Can we please land this on mozilla-1.9.2?

This patch is a requirement for running windows unit test on the talos test farm.  If this is not ok for 1.9.2.4 can we land on 1.9.2.5?

Comment 28

[John Ford [:jhford] CET/CEST Berlin Time]

Comment on attachment 441525 [details] [diff] [review]
mozilla-1.9.2 version of the patch

I should have asked for approval on 1.9.2.5.

Comment 29

[John Ford [:jhford] CET/CEST Berlin Time]

Attachment: mozilla-1.9.1 version of the patch

not sure which version of 1.9.1 I should ask for review for after review.  I included the core_abspath function from http://mxr.mozilla.org/mozilla1.9.2/source/config/config.mk#86

Comment 30

[John Ford [:jhford] CET/CEST Berlin Time]

(In reply to comment #29)
> not sure which version of 1.9.1 I should ask for *approval* for after review.

sorry for the monday morning bugspam.

Comment 31

[Aki Sasaki (not active)]

Not having this landed on 1.9.2 is breaking a lot of mobile builds + tests...

Comment 32

[LegNeato]

Comment on attachment 441525 [details] [diff] [review]
mozilla-1.9.2 version of the patch

a=LegNeato for 1.9.2.5

Comment 33

[:Gavin Sharp [email: [e-mail]]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/8d401e3574b1

Comment 34

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

ted: gentle ping on the r? for mozilla-1.9.1