Closed
Bug 830996
Opened 11 years ago
Closed 11 years ago
implement a way to DRY mozbase packages for m-c
Categories
(Testing :: Mozbase, defect)
Testing
Mozbase
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla21
People
(Reporter: k0scist, Assigned: k0scist)
Details
Attachments
(2 files, 4 obsolete files)
3.89 KB,
patch
|
wlach
:
review+
|
Details | Diff | Splinter Review |
10.67 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
Currently, the mozbase modules are installed into the virtualenv via https://mxr.mozilla.org/mozilla-central/source/build/virtualenv/populate_virtualenv.py from https://mxr.mozilla.org/mozilla-central/source/build/virtualenv/packages.txt . However, this requires listing the path of each package to install. For mozbase packages, I assert that if we mirror -> m-c (for context, see https://wiki.mozilla.org/Auto-tools/Projects/MozBase#Mirroring ), we want these packages available in the virtualenv. There's nothing in particular to prevent the case where a package is mirrored to the tree successfully but by failure to add to the virtualenv. While, IIRC, this has never caused a failure in production, there have been failures on try and people not knowing how to fix them and (ateam) developer time wasted on figuring this out. I have a few ideas on how to fix this, listed in order of my preference: - add (e.g.) `mozbase.subdir` capability to https://mxr.mozilla.org/mozilla-central/source/build/virtualenv/populate_virtualenv.py . This introduces a new type of 'thing' whereby, given a directory, populate_virtualenv.py will descend into each subdirectory and if it has a setup.py will add that directory to a .pth file - dynamically generate a packages.txt via generate_diff.py . In this approach, we extend populate_virtualenv.py to allow build/virtualenv/packages.txt to specify additional packages.txt files and also add the ability for generate_diff.py to generate a new testing/mozbase/packages.txt upon invocation. - invoke setup_development.py; i don't really think that setup_development.py should be used in m-c. its not what it was designed for, for one, though of course we can modify its intent. It also incurs overhead which i know :gps was keen to avoid. - having updating build/virtualenv/packages.py be part of the process. In other words, WONTFIX. I'm not keen on this option as in my experience if you require developers to do extra things when e.g. mirroring to m-c, then they will forget and won't necessarily check the wiki. However, if we do punt, we should at least document what should be done as mirroring is already complicated enough Aside, I'm not sure if populate_virtualenv.py is "a good stop gap" or "a line of technology we want to pursue". I tend to think in the longer term we should rethink this but I don't have any brilliant ideas now. pip requirements files jump to mind, but our use case is a little different and we don't have a dedicated pip developer (though damn i wish we did).
Comment 1•11 years ago
|
||
(In reply to Jeff Hammel [:jhammel] from comment #0) > - dynamically generate a packages.txt via generate_diff.py . In this > approach, we extend populate_virtualenv.py to allow > build/virtualenv/packages.txt to specify additional packages.txt > files and also add the ability for generate_diff.py to generate a > new testing/mozbase/packages.txt upon invocation. I like this one. It's generic and reusable for other needs.
Assignee | ||
Comment 2•11 years ago
|
||
let's go with that then
Comment 3•11 years ago
|
||
+1
Assignee | ||
Comment 4•11 years ago
|
||
I'm going to break this into two parts because they are conceptually separate: Part 1: this patch; fix generate_diff.py Part 2: fix populate_virtualenv.py to read multiple packages.txt files While I want them to be reviewed separately, they should be landed together
Assignee: nobody → jhammel
Attachment #704102 -
Flags: review?(wlachance)
Assignee | ||
Comment 5•11 years ago
|
||
This is most of what I intended to do for populate_virtualenv.py. It is missing one important thing, which is why I didn't put it up for review: .ensure() now needs to be able to call the other packages.txt things recursively. TBD. But I'd be curious to know if I'm going in a good direction here.
Attachment #704147 -
Flags: feedback?(gps)
Comment 6•11 years ago
|
||
Comment on attachment 704147 [details] [diff] [review] Part 2: Enhancement for populate_virtualenv.py (WIP) Review of attachment 704147 [details] [diff] [review]: ----------------------------------------------------------------- Looks like a good start!
Attachment #704147 -
Flags: feedback?(gps) → feedback+
Comment 7•11 years ago
|
||
Comment on attachment 704102 [details] [diff] [review] Part 1: fix generate_diff.py This looks reasonable to me, though I have to confess I'm not overly familiar with this m-c virtualenv stuff.
Attachment #704102 -
Flags: review?(wlachance) → review+
Assignee | ||
Comment 8•11 years ago
|
||
(In reply to William Lachance (:wlach) from comment #7) > Comment on attachment 704102 [details] [diff] [review] > Part 1: fix generate_diff.py > > This looks reasonable to me, though I have to confess I'm not overly > familiar with this m-c virtualenv stuff. pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/52d942866e4e
Whiteboard: [leave open]
Assignee | ||
Comment 9•11 years ago
|
||
I haven't tested this yet, as it requires https://bugzilla.mozilla.org/attachment.cgi?id=704102&action=edit landed on m-c. Will put up for review then if all is good
Attachment #704147 -
Attachment is obsolete: true
Assignee | ||
Comment 11•11 years ago
|
||
pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=d3b856c5a3e9
Attachment #705527 -
Attachment is obsolete: true
Attachment #705952 -
Flags: review?(gps)
Comment 12•11 years ago
|
||
Try run for d3b856c5a3e9 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=d3b856c5a3e9 Results (out of 19 total builds): success: 18 failure: 1 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jhammel@mozilla.com-d3b856c5a3e9
Comment 13•11 years ago
|
||
Comment on attachment 705952 [details] [diff] [review] Part 2: Enhancement for populate_virtualenv.py Review of attachment 705952 [details] [diff] [review]: ----------------------------------------------------------------- Great feature addition! ::: build/virtualenv/populate_virtualenv.py @@ +94,5 @@ > highest-level. > """ > + if self.up_to_date(): > + return self.virtualenv_root > + else: No need for an else after return.
Attachment #705952 -
Flags: review?(gps) → review+
Assignee | ||
Comment 14•11 years ago
|
||
(In reply to Gregory Szorc [:gps] from comment #13) > Comment on attachment 705952 [details] [diff] [review] > Part 2: Enhancement for populate_virtualenv.py > > Review of attachment 705952 [details] [diff] [review]: > ----------------------------------------------------------------- > > Great feature addition! > > ::: build/virtualenv/populate_virtualenv.py > @@ +94,5 @@ > > highest-level. > > """ > > + if self.up_to_date(): > > + return self.virtualenv_root > > + else: > > No need for an else after return. Funny, I was debating with myself which was more idiomatic. Will change.
Assignee | ||
Comment 15•11 years ago
|
||
pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/c779b2ab7695 It'd be nice to make the code in handle_package more plugin-like: that is, have a class for each possibility. I thought about going down that route for this patch, but figured it was YAGNI for now.
Whiteboard: [leave open]
Comment 16•11 years ago
|
||
I just started getting a configure error in the spidermonkey shell in populate_virtualenv.py that I suspect is caused by this patch. STR: cd js/src autoconf2.13 mkdir objdir cd objdir ../configure ../configure // fails with: Creating Python environment Traceback (most recent call last): File "/moz/mi/js/src/../../build/virtualenv/populate_virtualenv.py", line 354, in <module> manager.ensure() File "/moz/mi/js/src/../../build/virtualenv/populate_virtualenv.py", line 96, in ensure if self.up_to_date(): File "/moz/mi/js/src/../../build/virtualenv/populate_virtualenv.py", line 81, in up_to_date src) NameError: global name 'src' is not defined
Comment 17•11 years ago
|
||
I can confirm that updating to the cset right before comment 15 fixes the problem.
Assignee | ||
Comment 18•11 years ago
|
||
Thanks, Luke, and sorry about this. The code should of course be s/src/submanifest/ . I'm not seeing the failure locally and have no idea why. I'll continue to investigate this and in the mean time put up with a patch.
Assignee | ||
Comment 19•11 years ago
|
||
Luke, can you check if this patch fixes your problem? Still trying to repro locally (and have no idea why it is not breaking for me currently...yes, i'm clobbering and everything though I'm probably doing something stupid).
Attachment #706145 -
Flags: review?(luke)
Comment 20•11 years ago
|
||
I'm afraid I get a new error: Creating Python environment Traceback (most recent call last): File "/moz/mi/js/src/../../build/virtualenv/populate_virtualenv.py", line 354, in <module> manager.ensure() File "/moz/mi/js/src/../../build/virtualenv/populate_virtualenv.py", line 96, in ensure if self.up_to_date(): File "/moz/mi/js/src/../../build/virtualenv/populate_virtualenv.py", line 82, in up_to_date if not submanager.up_to_date(): File "/moz/mi/js/src/../../build/virtualenv/populate_virtualenv.py", line 70, in up_to_date dep_mtime = max(os.path.getmtime(p) for p in deps) File "/moz/mi/js/src/../../build/virtualenv/populate_virtualenv.py", line 70, in <genexpr> dep_mtime = max(os.path.getmtime(p) for p in deps) File "/usr/lib/python2.7/genericpath.py", line 54, in getmtime return os.stat(filename).st_mtime OSError: [Errno 2] No such file or directory: 'testing/mozbase/packages.txt'
Updated•11 years ago
|
Attachment #706145 -
Flags: review?(luke)
Comment 21•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c779b2ab7695
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Assignee | ||
Comment 22•11 years ago
|
||
(In reply to Ryan VanderMeulen from comment #21) > https://hg.mozilla.org/mozilla-central/rev/c779b2ab7695 This may need to be backed out wrt Luke's problems :(
Comment 23•11 years ago
|
||
Comment on attachment 706145 [details] [diff] [review] follow up Review of attachment 706145 [details] [diff] [review]: ----------------------------------------------------------------- While I wasn't able to repro locally, this is the fix I made as well. Land directly in m-c. You should know within 10 minutes if this broke the tree since configure should fail.
Attachment #706145 -
Flags: review+
Comment 24•11 years ago
|
||
Comment on attachment 706145 [details] [diff] [review] follow up Review of attachment 706145 [details] [diff] [review]: ----------------------------------------------------------------- Grrr. ::: build/virtualenv/populate_virtualenv.py @@ +77,5 @@ > for submanifest in submanifests: > submanager = VirtualenvManager(self.topsrcdir, > self.virtualenv_root, > self.log_handle, > + submanifest) r+ with os.path.join(self.topsrcdir, submanifest)
Comment 25•11 years ago
|
||
Comment on attachment 706145 [details] [diff] [review] follow up Review of attachment 706145 [details] [diff] [review]: ----------------------------------------------------------------- Actually no. I'm going to back this out until we can figure out the SpiderMonkey bit. I think it's slightly more complicated than one line.
Attachment #706145 -
Flags: review+
Comment 26•11 years ago
|
||
Backed out the changes to populate_virtualenv.py. https://hg.mozilla.org/mozilla-central/rev/a207f33adc1a
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 27•11 years ago
|
||
Comment on attachment 705952 [details] [diff] [review] Part 2: Enhancement for populate_virtualenv.py Review of attachment 705952 [details] [diff] [review]: ----------------------------------------------------------------- ::: build/virtualenv/populate_virtualenv.py @@ +56,5 @@ > > + def up_to_date(self): > + """Returns whether the virtualenv is present and up to date.""" > + > + deps = [self.manifest_path, __file__] The problem is self.manifest_path is a relative path, which is later stat'd. Feed an absolute path into deps for the os.path.getmtime later and you should be golden.
Assignee | ||
Comment 28•11 years ago
|
||
Looking closely, testing/mozbase/packages.txt also has to be added as a config.status dep: http://mxr.mozilla.org/mozilla-central/source/client.mk#287 I'm wondering if we're checking for packages.txt(s) here what the utility is for rebuilding based on them in populate_virtualenv.py . I in general hate these sort of hidden dependencies and it *seems* like having these deps declared in client.mk is redundant with the checks in populate_virtualenv.py . I am likely to not take this for this bug, but wondering what the thoughts on eliminating this redundancy before I file that bug.
Assignee | ||
Comment 29•11 years ago
|
||
Attachment #705952 -
Attachment is obsolete: true
Attachment #706145 -
Attachment is obsolete: true
Attachment #706665 -
Flags: review?(gps)
Comment 30•11 years ago
|
||
(In reply to Jeff Hammel [:jhammel] from comment #28) > Looking closely, testing/mozbase/packages.txt also has to be added as a > config.status dep: > http://mxr.mozilla.org/mozilla-central/source/client.mk#287 > > I'm wondering if we're checking for packages.txt(s) here what the utility is > for rebuilding based on them in populate_virtualenv.py . I in general hate > these sort of hidden dependencies and it *seems* like having these deps > declared in client.mk is redundant with the checks in populate_virtualenv.py. It's all hacky. We should also list the setup.py, etc files as well in case changes to them would require a virtualenv rebuild to reflect. Ideally, we should probably have populate_virtualenv.txt write out a list of dependency files and should have a make rule checking that all these are older than the last virtualenv build time. I'd add the new packages.txt to config.status deps. Everything else can be a follow-up bug.
Comment 31•11 years ago
|
||
Comment on attachment 706665 [details] [diff] [review] Part 2: trying again Review of attachment 706665 [details] [diff] [review]: ----------------------------------------------------------------- Ship it.
Attachment #706665 -
Flags: review?(gps) → review+
Assignee | ||
Comment 32•11 years ago
|
||
pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=fc43bf84b21a
Comment 33•11 years ago
|
||
Try run for fc43bf84b21a is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=fc43bf84b21a Results (out of 152 total builds): exception: 2 success: 147 warnings: 3 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jhammel@mozilla.com-fc43bf84b21a
Assignee | ||
Comment 34•11 years ago
|
||
pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/818bfecbe8e5 Hopefully it will stick this time
Comment 35•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/818bfecbe8e5
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•