Closed Bug 525047 Opened 11 years ago Closed 11 years ago

'make clean' deletes automationutils.py

Categories

(Firefox Build System :: General, defect)

Product:
Firefox Build System
Component:
General
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(status1.9.2 .2-fixed, status1.9.1 .9-fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla1.9.3a1
Tracking Flags:
Tracking Status
status1.9.2 --- .2-fixed
status1.9.1 --- .9-fixed

People

(Reporter: ennofennema, Assigned: neil)

References

Details

(Keywords: verified1.9.1, verified1.9.2)

Attachments

(1 file, 4 obsolete files)

Even better [Checkin: Comment 29 & 34 & 36]
2.71 KB, patch
ted
: review+
dveditz
: approval1.9.2.2+
dveditz
: approval1.9.1.9+
Details | Diff | Splinter Review 
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
I've run into this problem also with SeaMonkey 2.0 release on Mac OS X
I assume you're doing a srcdir build?
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 ;-)
Attached patch Possible patch (obsolete) — — Splinter Review
Assignee: nobody → neil
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #409058 - Flags: review?(ted.mielczarek)
(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.
(In reply to comment #2)
> I assume you're doing a srcdir build?

Yes, if that means not using objdir
I know little about the python preprocessing but could a solution be introducing a automationutils.py.in with nill reprocessing?
No. I don't want that.
(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?
Version: unspecified → 1.9.1 Branch
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.
(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.
Attached patch Possible patch (obsolete) — — Splinter Review
Attachment #409058 - Attachment is obsolete: true
Attachment #412563 - Flags: review?(ted.mielczarek)
Attachment #409058 - Flags: review?(ted.mielczarek)
Comment on attachment 412563 [details] [diff] [review]
Possible patch

Ok, seems reasonable.
Attachment #412563 - Flags: review?(ted.mielczarek) → review+
Pushed changeset dbdbe3ad0234 to mozilla-central.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [wanted on m-1.9.2/m-1.9.1 too...]
Target Milestone: --- → mozilla1.9.3a1
Version: 1.9.1 Branch → Trunk
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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...
My bad, this works on Windows because it copies instead of symlinks...
(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/ !
And I need to remember not to typo the checkin comment again too...

Pushed changeset 5a2692b1abee to mozilla-central.
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
(In reply to comment #19)
> Pushed changeset 5a2692b1abee to mozilla-central.

http://hg.mozilla.org/mozilla-central/rev/d86a2cf35ca8
backout
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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)...
Attached patch Stupid Python (obsolete) — — Splinter Review
Attachment #412563 - Attachment is obsolete: true
Attachment #418843 - Flags: review?(ted.mielczarek)
Attached patch Stupid Python (obsolete) — — Splinter Review
Attachment #418907 - Flags: review?(ted.mielczarek)
(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 on attachment 418843 [details] [diff] [review]
Stupid Python

Yeah, I'd rather keep the symlinks, they make development easier.
Attachment #418843 - Flags: review?(ted.mielczarek) → review-
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.)
Attachment #418907 - Flags: review?(ted.mielczarek) → review-
We already have a variable with the appropriate path in question :-)
Attachment #418843 - Attachment is obsolete: true
Attachment #418907 - Attachment is obsolete: true
Attachment #421035 - Flags: review?(ted.mielczarek)
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".
Attachment #421035 - Flags: review?(ted.mielczarek) → review+
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...
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Not disagreeing, just clarifying! (since it wasn't mentioned anywhere else on the bug)
Comment on attachment 421035 [details] [diff] [review]
Even better
[Checkin: Comment 29 & 34 & 36]


"approval1.9.2.2=?":
Zero risk, build config only.
Attachment #421035 - Flags: approval1.9.2.2?
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
Attachment #421035 - Flags: approval1.9.2.2? → approval1.9.2.2+
Comment on attachment 421035 [details] [diff] [review]
Even better
[Checkin: Comment 29 & 34 & 36]


"approval1.9.1.9=?":
Zero risk, build config only.
Attachment #421035 - Attachment description: Even better → Even better [Checkin: Comment 29 & 33]
Attachment #421035 - Flags: approval1.9.1.9?
Attachment #421035 - Attachment description: Even better [Checkin: Comment 29 & 33] → Even better [Checkin: Comment 29 & 34]
status1.9.2: wanted → .2-fixed
Flags: in-testsuite-
Whiteboard: [wanted on m-1.9.2/m-1.9.1 too...]
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
Attachment #421035 - Flags: approval1.9.1.9? → approval1.9.1.9+
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
Attachment #421035 - Attachment description: Even better [Checkin: Comment 29 & 34] → Even better [Checkin: Comment 29 & 34 & 36]
Verified that patches pushed in comment#34 and comment#36 landed.
Keywords: verified1.9.1, verified1.9.2
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.