Last Comment Bug 525047 - 'make clean' deletes automationutils.py
: 'make clean' deletes automationutils.py
Status: RESOLVED FIXED
: verified1.9.1, verified1.9.2
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: x86_64 Linux
: -- normal (vote)
: mozilla1.9.3a1
Assigned To: neil@parkwaycc.co.uk
:
:
Mentors:
Depends on: 539394
Blocks:
  Show dependency treegraph
 
Reported: 2009-10-28 12:45 PDT by Enno
Modified: 2010-03-22 17:18 PDT (History)
8 users (show)
bugzillamozillaorg_serge_20140323: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
.2-fixed
.9-fixed


Attachments
Possible patch (1.05 KB, patch)
2009-10-29 03:39 PDT, neil@parkwaycc.co.uk
no flags Details | Diff | Splinter Review
Possible patch (2.15 KB, patch)
2009-11-16 04:49 PST, neil@parkwaycc.co.uk
ted: review+
Details | Diff | Splinter Review
Stupid Python (3.97 KB, patch)
2009-12-22 06:17 PST, neil@parkwaycc.co.uk
ted: review-
Details | Diff | Splinter Review
Stupid Python (3.62 KB, patch)
2009-12-22 14:12 PST, neil@parkwaycc.co.uk
ted: review-
Details | Diff | Splinter Review
Even better [Checkin: Comment 29 & 34 & 36] (2.71 KB, patch)
2010-01-11 06:18 PST, neil@parkwaycc.co.uk
ted: review+
dveditz: approval1.9.2.2+
dveditz: approval1.9.1.9+
Details | Diff | Splinter Review

Description Enno 2009-10-28 12:45:16 PDT
User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.8.1.21) Gecko/20090410 SUSE/1.1.16-1.1 SeaMonkey/1.1.16
Build Identifier: SEAMONKEY_2_0rc2_RELEASE

'make clean' deletes mozilla/build/automationutils.py which is a proper and necessary source file and not eg. generated from automationutils.py.in 

Should not happen

Reproducible: Always
Comment 1 R P Mozley 2009-10-28 18:07:31 PDT
I've run into this problem also with SeaMonkey 2.0 release on Mac OS X
Comment 2 Ted Mielczarek [:ted.mielczarek] 2009-10-29 03:18:30 PDT
I assume you're doing a srcdir build?
Comment 3 neil@parkwaycc.co.uk 2009-10-29 03:33:24 PDT
build/automation-build.mk copies automation.py to the objdir (why?) and adds it to the garbage. This is of course unhelpful for a srcdir build ;-)
Comment 4 neil@parkwaycc.co.uk 2009-10-29 03:39:40 PDT
Created attachment 409058 [details] [diff] [review]
Possible patch
Comment 5 Ted Mielczarek [:ted.mielczarek] 2009-10-29 03:45:10 PDT
(In reply to comment #3)
> build/automation-build.mk copies automation.py to the objdir (why?) and adds it
> to the garbage. This is of course unhelpful for a srcdir build ;-)

Because PYTHONPATH mangling can be a bit of a pain, and our preprocessing of Python files doesn't make it any easier. Ideally we wouldn't preprocess any of our Python, and we could stop copying these files around.
Comment 6 Enno 2009-10-29 04:05:48 PDT
(In reply to comment #2)
> I assume you're doing a srcdir build?

Yes, if that means not using objdir
Comment 7 Enno 2009-10-29 06:19:26 PDT
I know little about the python preprocessing but could a solution be introducing a automationutils.py.in with nill reprocessing?
Comment 8 Ted Mielczarek [:ted.mielczarek] 2009-10-29 06:23:21 PDT
No. I don't want that.
Comment 9 neil@parkwaycc.co.uk 2009-10-29 07:13:48 PDT
(In reply to comment #5)
> (In reply to comment #3)
> > build/automation-build.mk copies automation.py to the objdir (why?) and adds it
> > to the garbage. This is of course unhelpful for a srcdir build ;-)
> Because PYTHONPATH mangling can be a bit of a pain, and our preprocessing of
> Python files doesn't make it any easier. Ideally we wouldn't preprocess any of
> our Python, and we could stop copying these files around.
How does automation-utils.py relate to the preprocessing of automation.py?
Comment 10 Ted Mielczarek [:ted.mielczarek] 2009-11-13 09:10:19 PST
Comment on attachment 409058 [details] [diff] [review]
Possible patch

I'm pretty sure this will break Mochitest/reftest. When running them in-tree, they rely on having automationutils.py copied to the local directory, then copied again to $objdir/_test/whatever.
Comment 11 neil@parkwaycc.co.uk 2009-11-16 04:14:33 PST
(In reply to comment #10)
> (From update of attachment 409058 [details] [diff] [review])
> I'm pretty sure this will break Mochitest/reftest. When running them in-tree,
> they rely on having automationutils.py copied to the local directory, then
> copied again to $objdir/_test/whatever.
Oh I see, I need to change the references from $(CURDIR)/automationutils.py to $(topsrcdir)/build/automationutils.py, to avoid the unnecessary copy.
Comment 12 neil@parkwaycc.co.uk 2009-11-16 04:49:13 PST
Created attachment 412563 [details] [diff] [review]
Possible patch
Comment 13 Ted Mielczarek [:ted.mielczarek] 2009-11-23 09:35:16 PST
Comment on attachment 412563 [details] [diff] [review]
Possible patch

Ok, seems reasonable.
Comment 14 neil@parkwaycc.co.uk 2009-12-15 16:14:41 PST
Pushed changeset dbdbe3ad0234 to mozilla-central.
Comment 15 :Gavin Sharp [email: gavin@gavinsharp.com] 2009-12-15 20:38:06 PST
Backed out, since this caused clobbered leak test machines to fail with:

ImportError: No module named automationutils

e.g.: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1260932775.1260937785.2108.gz&fulltext=1

https://hg.mozilla.org/mozilla-central/rev/4bbe26190c7d
Comment 16 neil@parkwaycc.co.uk 2009-12-16 14:09:38 PST
(In reply to comment #15)
> Backed out, since this caused clobbered leak test machines to fail with:
> 
> ImportError: No module named automationutils
> 
> e.g.:
> http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1260932775.1260937785.2108.gz&fulltext=1

Weird. That log shows automationutils.py being installed into _leaktest:

/builds/moz2_slave/mozilla-central-linux-debug/build/obj-firefox/config/nsinstall -R automation.py /builds/moz2_slave/mozilla-central-linux-debug/build/build/automationutils.py leaktest.py /builds/moz2_slave/mozilla-central-linux-debug/build/build/bloatcycle.html /builds/moz2_slave/mozilla-central-linux-debug/build/build/pgo/server-locations.txt ../_leaktest

And python leaktest.py -- -register certainly succeeds locally...
Comment 17 neil@parkwaycc.co.uk 2009-12-16 14:37:00 PST
My bad, this works on Windows because it copies instead of symlinks...
Comment 18 neil@parkwaycc.co.uk 2009-12-16 14:39:46 PST
(In reply to comment #11)
> Oh I see, I need to change the references from $(CURDIR)/automationutils.py to
> $(topsrcdir)/build/automationutils.py, to avoid the unnecessary copy.
And I need to not forgot to prefix $(topsrcdir)/build/ !
Comment 19 neil@parkwaycc.co.uk 2009-12-17 13:30:02 PST
And I need to remember not to typo the checkin comment again too...

Pushed changeset 5a2692b1abee to mozilla-central.
Comment 20 Serge Gautherie (:sgautherie) 2009-12-17 18:55:05 PST
(In reply to comment #19)
> Pushed changeset 5a2692b1abee to mozilla-central.

http://hg.mozilla.org/mozilla-central/rev/d86a2cf35ca8
backout
Comment 21 neil@parkwaycc.co.uk 2009-12-18 16:13:28 PST
(In reply to comment #18)
> (In reply to comment #11)
> > Oh I see, I need to change the references from $(CURDIR)/automationutils.py to
> > $(topsrcdir)/build/automationutils.py, to avoid the unnecessary copy.
> And I need to not forgot to prefix $(topsrcdir)/build/ !
That's not it, since if you're building from $(CURDIR)/build/ then $(topsrcdir)/build/ is in your $(VPATH)...
Comment 22 neil@parkwaycc.co.uk 2009-12-22 06:17:03 PST
Created attachment 418843 [details] [diff] [review]
Stupid Python
Comment 23 neil@parkwaycc.co.uk 2009-12-22 14:12:23 PST
Created attachment 418907 [details] [diff] [review]
Stupid Python
Comment 24 Serge Gautherie (:sgautherie) 2009-12-22 18:12:57 PST
(In reply to comment #22)

Ftr,

> Created an attachment (id=418843) [details]

This one uses multiple |NSDISTMODE = copy|.

(In reply to comment #23)
> Created an attachment (id=418907) [details]

This one uses |sys.path.insert(0, "");|
Comment 25 Ted Mielczarek [:ted.mielczarek] 2010-01-11 05:56:38 PST
Comment on attachment 418843 [details] [diff] [review]
Stupid Python

Yeah, I'd rather keep the symlinks, they make development easier.
Comment 26 Ted Mielczarek [:ted.mielczarek] 2010-01-11 06:02:42 PST
Comment on attachment 418907 [details] [diff] [review]
Stupid Python

>diff --git a/build/automation.py.in b/build/automation.py.in
>--- a/build/automation.py.in
>+++ b/build/automation.py.in
>@@ -45,17 +45,17 @@ import re
> import re
> import select
> import shutil
> import signal
> import subprocess
> import sys
> import threading
> import tempfile
>-
>+sys.path.insert(0, "");

What does this wind up doing, adding the script's directory to sys.path? Could we make this clearer using os.path.dirname(__file__) or something? (I tested, and dirname(__file__) returns the containing dir even if __file__ is a symlink.)
Comment 27 neil@parkwaycc.co.uk 2010-01-11 06:18:21 PST
Created attachment 421035 [details] [diff] [review]
Even better
[Checkin: Comment 29 & 34 & 36]

We already have a variable with the appropriate path in question :-)
Comment 28 Ted Mielczarek [:ted.mielczarek] 2010-01-11 07:07:39 PST
Comment on attachment 421035 [details] [diff] [review]
Even better
[Checkin: Comment 29 & 34 & 36]

Yeah, that's the ticket.

For the record, when Neil says "stupid Python" he means "when Python adds the script directory to sys.path, if the script is a symlink, it actually adds the directory of the original file".
Comment 29 neil@parkwaycc.co.uk 2010-01-11 15:57:45 PST
Pushed changeset fa0a5a38b7f4 to mozilla-central.

(In reply to comment #28)
> For the record, when Neil says "stupid Python" he means "when Python adds the
> script directory to sys.path, if the script is a symlink, it actually adds the
> directory of the original file".
Which IMHO is still a stupid thing to do...
Comment 30 Ted Mielczarek [:ted.mielczarek] 2010-01-11 16:59:22 PST
Not disagreeing, just clarifying! (since it wasn't mentioned anywhere else on the bug)
Comment 31 Serge Gautherie (:sgautherie) 2010-02-05 19:01:24 PST
Comment on attachment 421035 [details] [diff] [review]
Even better
[Checkin: Comment 29 & 34 & 36]


"approval1.9.2.2=?":
Zero risk, build config only.
Comment 32 Daniel Veditz [:dveditz] 2010-02-26 13:51:50 PST
Comment on attachment 421035 [details] [diff] [review]
Even better
[Checkin: Comment 29 & 34 & 36]

Approved for 1.9.2.2, a=dveditz for release-drivers
Comment 33 Serge Gautherie (:sgautherie) 2010-02-27 04:45:17 PST
Comment on attachment 421035 [details] [diff] [review]
Even better
[Checkin: Comment 29 & 34 & 36]


"approval1.9.1.9=?":
Zero risk, build config only.
Comment 34 Serge Gautherie (:sgautherie) 2010-02-27 04:45:38 PST
Comment on attachment 421035 [details] [diff] [review]
Even better
[Checkin: Comment 29 & 34 & 36]


http://hg.mozilla.org/releases/mozilla-1.9.2/rev/08f2d33c06f6
Comment 35 Daniel Veditz [:dveditz] 2010-03-01 10:26:18 PST
Comment on attachment 421035 [details] [diff] [review]
Even better
[Checkin: Comment 29 & 34 & 36]

Approved for 1.9.1.9, a=dveditz for release-drivers
Comment 36 Serge Gautherie (:sgautherie) 2010-03-01 12:46:22 PST
Comment on attachment 421035 [details] [diff] [review]
Even better
[Checkin: Comment 29 & 34 & 36]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/4fdc73ee8729
Comment 37 Marco Zehe (:MarcoZ) 2010-03-22 17:18:32 PDT
Verified that patches pushed in comment#34 and comment#36 landed.

Note You need to log in before you can comment on or make changes to this bug.