616542 - Shorten file path length of mochitest

Status: NEW

(Firefox Build System :: General, defect)

Description

[Rail Aliiev [:rail]]

Releng hit a problem while building mozilla-central on windows. The error message is:

Could not create the directory:
..\..\..\..\..\..\_tests\testing\mochitest\tests\parser\htmlparser\tests\mochitest\html5lib_tree_construction\scripted:
No such file or directory

It would be great to shorten the path of mochitest.

Comment 1

[Rail Aliiev [:rail]]

Attachment: shorteh.sh

A simple bash script replacing hardcoded paths.

It scans Makefile.in files only. Other 2 files will be patched in the next patch.

Comment 2

[Rail Aliiev [:rail]]

Attachment: Manual fixes

Comment 3

[Rail Aliiev [:rail]]

Attachment: Manual fixes

Files under testing/mochitest require additional intervention, I realized after a try build.

* js/src/config/config.mk synced with config/config.mk
* Adjusted relativesrcdir for files in testing/mochitest

This patch should be applied after running shorten.sh

There are 6 oranges, hopefully unrelated to the changes made by the patch. It would be great to recheck this.

Here is a summary of the oranges: http://pastebin.mozilla.org/877061

Comment 4

[Justin Wood (:Callek)]

rail, I think your two Manual Fixes patches are disjoint and we would need them both? am I wrong?

Comment 5

[Rail Aliiev [:rail]]

Attachment: Manual fixes

(In reply to comment #4)
> rail, I think your two Manual Fixes patches are disjoint and we would need them
> both? am I wrong?

Oh, yeah. The result of staying too late. :)

Thanks for the catch.

Comment 6

[(not currently active) Ted Mielczarek]

Comment on attachment 495461 [details] [diff] [review]
Manual fixes

>--- a/testing/testsuite-targets.mk
>+++ b/testing/testsuite-targets.mk
>@@ -48,17 +48,17 @@ endif
> SYMBOLS_PATH := --symbols-path=$(DIST)/crashreporter-symbols
> 
> # Usage: |make [TEST_PATH=...] [EXTRA_TEST_ARGS=...] mochitest*|.
> MOCHITESTS := mochitest-plain mochitest-chrome mochitest-a11y mochitest-ipcplugins
> mochitest:: $(MOCHITESTS)
> 
> RUN_MOCHITEST = \
> 	rm -f ./$@.log && \
>-	$(PYTHON) _tests/testing/mochitest/runtests.py --autorun --close-when-done \
>+	$(PYTHON) $(mochitestdir)/runtests.py --autorun --close-when-done \
> 	  --console-level=INFO --log-file=./$@.log --file-level=INFO \
> 	  $(SYMBOLS_PATH) $(TEST_PATH_ARG) $(EXTRA_TEST_ARGS)
> 
> ifndef NO_FAIL_ON_TEST_ERRORS
> define CHECK_TEST_ERROR
>   @errors=`grep "TEST-UNEXPECTED-" $@.log` ;\
>   if test "$$errors" ; then \
> 	  echo "$@ failed:"; \
>@@ -135,16 +135,17 @@ xpcshell-tests:
> 	  --manifest=$(DEPTH)/_tests/xpcshell/all-test-dirs.list \
> 	  --no-logfiles \
>           $(SYMBOLS_PATH) \
> 	  $(TEST_PATH_ARG) $(EXTRA_TEST_ARGS) \
> 	  $(DIST)/bin/xpcshell
> 
> # Package up the tests and test harnesses
> include $(topsrcdir)/toolkit/mozapps/installer/package-name.mk
>+include $(topsrcdir)/config/config.mk

This config.mk include seems unnecessary. (We clearly already use a bunch of stuff from config.mk in this file.)

Despite being a large scary invasive change, this seems ok.

Comment 7

[Marco Castelluccio [:marco]]

(In reply to comment #6)
> Despite being a large scary invasive change, this seems ok.

Ted Mielczarek, to resolve this bug we need only to delete that line from the patch?
This bug is preventing my build to work, so I would like to see it resolved.

Comment 8

[(not currently active) Ted Mielczarek]

Yes, with just that change we should be able to land this. Does this patch still apply to the latest source?

Comment 9

[Marco Castelluccio [:marco]]

I can't build Firefox because of the same problem reported by Rail Aliiev, so I think it still applies.

Comment 10

[Marco Castelluccio [:marco]]

This bug is also for Windows 7 64 bit, not only for Linux.

Comment 11

[Marco Castelluccio [:marco]]

Attachment: Manual fixes corrected

Removed the useless line.

Comment 12

[Ed Morley [:emorley]]

Marco, there's something up with the patch as it doesn't import without a parse error into mercurial. I tried to fix it up, but it then still doesn't apply cleanly to tip - and manually resolving made me notice new instances (eg RUN_MOCHITEST_REMOTE) that need shortening now as well. 

Also, the diff of attached patch vs my fixed up patch is huge, which makes me a bit uneasy. The attached patch didn't have the full amount of context, which is probably the main reason why, but I don't have the time to pick through and check that something didn't break during fixing it up.

Please can you:
- check for new instances to shorten
- update to tip
- add author and commit message
- make sure the patch isn't malformed
- use the format/context settings here: http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed
- send to try
...and then re-request checkin-needed.

Thanks :-)

Comment 13

[Teemu Mannermaa (:wicked)]

Attachment: Shorten mochitest paths for m-c, V4

I'm not sure if Rail/Marco was going to work on this but I happened to refresh the patch while I was introducing myself to mochitests. Patch is updated to tip and should handle current mochitests in m-c (there seems to have been a lot new tests created during last year!) as I run the attached shorteh.sh script and also verified that _tests/testing directory no longer contains anything (that's the old location) after a build. I tested running Windows mochitests locally and at least that started and run the tests through.

Additionally this patch removes the _mochitest directory at the same time _tests directory is removed for rebuilds. This way it should match the old behaviour and make sure automationutils.py and tests are refreshed without clobbering builds manually all the time.

I don't have try-server access so would be nice if somebody could run the patch through try for me, thanks!

Comment 14

[Ed Morley [:emorley]]

Pushed to try, but the specialpowers changes landed in the last day or so have bitrotted the patch in a few places. Did the best I could to fixed locally, but not sure if I got everything:
https://tbpl.mozilla.org/?tree=Try&rev=a69d2e0bfd28

Comment 15

[Teemu Mannermaa (:wicked)]

Attachment: Shorten mochitest paths for m-c, V4.1

Thanks for the try-run. I've fixed the one Moth failure that seems to have been due to two new DOM Worker test extensions makefiles and also otherwise unrotted this version.

Comment 16

[(not currently active) Ted Mielczarek]

Comment on attachment 567546 [details] [diff] [review]
Shorten mochitest paths for m-c, V4.1

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

I didn't look at every single line of this patch, but I reviewed this in the past so I'm just going to r+ this. I did spot check a bunch of places and they look fine.

Comment 17

[Teemu Mannermaa (:wicked)]

Attachment: Shorten mochitest paths for c-c, V4.1

This is the required changes in the comm-central repo.

Comment 18

[Justin Wood (:Callek)]

Comment on attachment 567894 [details] [diff] [review]
Shorten mochitest paths for c-c, V4.1

If this was tested on c-c (not suite has horrid orange anyway) I give this patch a+ even if SeaMonkey tree is still closed, so we can avoid bitrot

Comment 19

[Teemu Mannermaa (:wicked)]

Attachment: Shorten mochitest paths for m-c, V4.2

Simple refresh due to recent code suffling. Carrying over r=ted since changes are minimal (or try to).

Comment 20

[Teemu Mannermaa (:wicked)]

Rail did all the heavy lifting here. I did run c-c suite tests locally and they did run with only few failures.

Comment 21

[Rail Aliiev [:rail]]

@ted: I have not enough privileges to land the patch to m-i/m-c. Could you land it?

Comment 22

[Ed Morley [:emorley]]

Comment on attachment 570627 [details] [diff] [review]
Shorten mochitest paths for m-c, V4.2

This didn't apply cleanly to inbound, but I've fixed locally & added the missing author/commit message (for future patches, please see http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed).

Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=9bcd060f8d1b

Thanks :-)

Comment 23

[Ed Morley [:emorley]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4997c6f8b24d

Comment 24

[Ed Morley [:emorley]]

Attachment: m-c part followup

dom/battery/test/Makefile.in was created since the initial patch, so needs changing too:
http://mxr.mozilla.org/mozilla-central/source/dom/battery/test/Makefile.in#59

Comment 25

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/4997c6f8b24d

Comment 26

[Mihai Sucan [:msucan]]

I am having problems executing the mochitests:
http://pastebin.mozilla.org/1374664

If I do make -C ff-obj mochitest-browser-chrome it tells me firefox-bin doesn't exist.

Why does the script look for the firefox-bin in the src folder instead of the ff-obj directory?

I tried running the mochitests from the root src folder and from the ff-obj dir. Same error.

Comment 27

[Rob Campbell [:rc] (:robcee)]

ditto what mihai said in comment #26.

Comment 28

[Rob Campbell [:rc] (:robcee)]

why are we landing massive build system changes on the weekend before a merge?

Comment 29

[Ed Morley [:emorley]]

Backed out preemptively in case this was the cause (and yet works on try/m-c somehow), given that if it is, it's stopping people test locally leading up to the merge:

https://hg.mozilla.org/mozilla-central/rev/e697147121b3

Comment 30

[Ed Morley [:emorley]]

Comment on attachment 572084 [details] [diff] [review]
m-c part followup

Is obsolete, as will be added to the main patch when it relands.

Comment 31

[Rob Campbell [:rc] (:robcee)]

confirmed that this was the cause of the problem.

Please wait until after Tuesday (November 8th) before relanding this. We can deal with breakage then.

thanks.

Comment 32

[Ed Morley [:emorley]]

Thanks for confirming Rob, I'll leave it to Rail/wicked to fix, test locally and re-request review from ted.

Comment 33

[Rail Aliiev [:rail]]

I'm not sure if I'm the best person to be an assignee. :( Back to the pool of Core/Build Config.

Comment 34

[Teemu Mannermaa (:wicked)]

Rail, I assigned this to you to mark the fact that you did the initial/most work (and thus should get credit for fixing this, not sure if you follow that principle, though) but I guess that'll work better when this is actually fixed. :) Anyway, I'll update the patch as soon as I figure out what's wrong.

Comment 35

[Rail Aliiev [:rail]]

Teemu, I just wanted to keep my "Assigned bugs" list shorter :) and make sure that Core/Build Config "unassigned bugs" (tirage?) query lists this bug. Thanks a lot for your hard work keeping the patch up to date and testing it!

Comment 36

[Teemu Mannermaa (:wicked)]

Actually, I doubt I'll get to this ever but this seems to be taken care of slowly by other bugs anyway..