implement a way to DRY mozbase packages for m-c

RESOLVED FIXED in mozilla21

Status

Testing
Mozbase
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Jeff Hammel, Assigned: Jeff Hammel)

Tracking

unspecified
mozilla21
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

6 years ago
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

6 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

6 years ago
let's go with that then
(Assignee)

Comment 4

6 years ago
Created attachment 704102 [details] [diff] [review]
Part 1: fix generate_diff.py

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

6 years ago
Created attachment 704147 [details] [diff] [review]
Part 2: Enhancement for populate_virtualenv.py (WIP)

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

6 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 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

6 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

6 years ago
Created attachment 705527 [details] [diff] [review]
Part 2: Enhancement for populate_virtualenv.py

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

6 years ago
Created attachment 705952 [details] [diff] [review]
Part 2: Enhancement for populate_virtualenv.py

pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=d3b856c5a3e9
Attachment #705527 - Attachment is obsolete: true
Attachment #705952 - Flags: review?(gps)

Comment 12

6 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 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

6 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

6 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]
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
I can confirm that updating to the cset right before comment 15 fixes the problem.
(Assignee)

Comment 18

6 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

6 years ago
Created attachment 706145 [details] [diff] [review]
follow up

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)
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'
https://hg.mozilla.org/mozilla-central/rev/c779b2ab7695
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
(Assignee)

Comment 22

6 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 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 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 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+
Backed out the changes to populate_virtualenv.py.

https://hg.mozilla.org/mozilla-central/rev/a207f33adc1a
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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

6 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

6 years ago
Created attachment 706665 [details] [diff] [review]
Part 2: trying again
Attachment #705952 - Attachment is obsolete: true
Attachment #706145 - Attachment is obsolete: true
Attachment #706665 - Flags: review?(gps)
(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 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+

Comment 33

6 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

6 years ago
pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/818bfecbe8e5

Hopefully it will stick this time
https://hg.mozilla.org/mozilla-central/rev/818bfecbe8e5
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.