Closed Bug 549427 Opened 10 years ago Closed 10 years ago

tests tarball should be zip files

Categories

(Firefox Build System :: General, defect)

x86
macOS
defect
Not set

Tracking

(status1.9.2 .5-fixed)

RESOLVED FIXED
Tracking Status
status1.9.2 --- .5-fixed

People

(Reporter: jhford, Assigned: jhford)

References

Details

Attachments

(3 files, 3 obsolete files)

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.
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.
Product: Firefox → Core
QA Contact: build.config → build-config
Attached patch potential patch (obsolete) — Splinter Review
I am not sure if i have the ifeq at the proper indentation or if i am detecting windows in the proper method
Attachment #429597 - Flags: review?(ted.mielczarek)
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
Blocks: 548768
Is there any reason not to just use zip on all platforms? We already do that with the symbol package.
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.
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.
Blocks: 550383
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.
Attachment #429597 - Flags: review?(ted.mielczarek) → review-
Attached patch zip tests instead of tar+bzip2 (obsolete) — Splinter Review
Assignee: nobody → jhford
Attachment #429597 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #430653 - Flags: review?(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.
Attachment #430653 - Flags: review?(ted.mielczarek) → review+
Attached patch updated patch (obsolete) — Splinter Review
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?
Attachment #430653 - Attachment is obsolete: true
Attachment #431770 - Flags: review?(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 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.
Attachment #431770 - Flags: review?(ted.mielczarek) → review+
No longer blocks: 550383
Is this just waiting on a downtime plan?
Summary: tests tarball for windows should be zip files → tests tarball should be zip files
(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 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.
Attachment #431770 - Flags: approval1.9.2.3?
Attachment #431770 - Flags: approval1.9.1.10?
Attachment #431770 - Flags: approval1.9.2.3?
Attachment #431770 - Flags: approval1.9.1.10?
unmarking approval*? because I need to land this on mozilla-central before I ask for branch approval.
Pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/7886dc6ae0c8
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a4
Backed out:
http://hg.mozilla.org/mozilla-central/rev/f843e0928e11

John says there's a problem with the buildbot side of things.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla1.9.3a4 → ---
Depends on: 557854
Attached patch updated patchSplinter Review
Is it ok to carry the r+ from the previous patch?  This update is for bitrot.
Attachment #431770 - Attachment is obsolete: true
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.)
Thanks :)

in which case, could you please land it?
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.
Attachment #437716 - Flags: approval1.9.2.4?
Attachment #437716 - Flags: approval1.9.1.10?
Attachment #437716 - Flags: approval1.9.0.19?
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
No longer blocks: 558258
Depends on: 558258
Comment on attachment 437716 [details] [diff] [review]
updated patch

This patch completely broke uploading of mac test packages. Removing approval? flags because of that.
Attachment #437716 - Flags: approval1.9.2.4?
Attachment #437716 - Flags: approval1.9.1.10?
Attachment #437716 - Flags: approval1.9.0.19?
No longer depends on: 558258
No longer blocks: 548768
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?
Attachment #441525 - Flags: approval1.9.2.4?
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.
Attachment #441525 - Flags: approval1.9.2.4? → approval1.9.2.5?
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
Attachment #441534 - Flags: review?(ted.mielczarek)
(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.
Not having this landed on 1.9.2 is breaking a lot of mobile builds + tests...
Comment on attachment 441525 [details] [diff] [review]
mozilla-1.9.2 version of the patch

a=LegNeato for 1.9.2.5
Attachment #441525 - Flags: approval1.9.2.5? → approval1.9.2.5+
ted: gentle ping on the r? for mozilla-1.9.1
Attachment #441534 - Flags: review?(ted.mielczarek) → review+
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.