841713 - support adding objdir paths to virtualenv

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[(not currently active) Ted Mielczarek]

The XPIDL parser generates .py files at build time in the objdir:
http://mxr.mozilla.org/mozilla-central/source/xpcom/idl-parser/Makefile.in#28

Right now there's no way to get that path into the virtualenv, so all the places we do IDL parsing wind up using pythonpath.py:
http://mxr.mozilla.org/mozilla-central/source/config/rules.mk#1276

I think we should make populate_virtualenv.py support this, maybe by adding support for something like:
xpidl.pth:$objdir/xpcom/idl-parser

Thoughts?

Comment 1

[Mike Hommey [:glandium]]

I think it's more complicated than that, because these files are actually generated wherever --cachedir points to, and i think it will also try to read from there.

Comment 2

[(not currently active) Ted Mielczarek]

We can almost certainly fix that so they always go to a known location.

Comment 3

[Mike Hommey [:glandium]]

The underlying point is that having the files in the virtualenv may not actually change anything (and pythonpath is possibly already useless)

Comment 4

[Gregory Szorc [:gps]]

Attachment: Add objdir paths to virtualenv, v1

I think this should do the trick.

Comment 5

[(not currently active) Ted Mielczarek]

Comment on attachment 715289 [details] [diff] [review]
Add objdir paths to virtualenv, v1

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

r=me, just a bikesheddy naming comment.

::: build/virtualenv/populate_virtualenv.py
@@ +157,5 @@
> +            will be read and processed as if its contents were concatenated
> +            into the manifest being read.
> +
> +        objdir -- Denotes a relative path in the object directory to add to the
> +            search path.

This seems oddly inconsistent, since we have "foo.pth:srcdir/path", and now we'll have "objdir:objdir/path". Any thoughts on how we can make that less confusing?

Comment 6

[Mike Hommey [:glandium]]

Comment on attachment 715289 [details] [diff] [review]
Add objdir paths to virtualenv, v1

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

::: build/autoconf/python-virtualenv.m4
@@ +52,5 @@
>    dnl This verifies our Python version is sane and ensures the Python
>    dnl virtualenv is present and up to date. It sanitizes the environment
>    dnl for us, so we don't need to clean anything out.
>    $PYTHON $_virtualenv_populate_path \
> +    $_virtualenv_topsrcdir $MOZ_BUILD_ROOT $MOZ_BUILD_ROOT/_virtualenv || exit 1

Do we have a reason not to just do $PYTHON $_virtualenv_populate_path $_virtualenv_topsrcdir $MOZ_BUILD_ROOT ? IOW, do we have a reason to specify a virtualenv path that would not be $MOZ_BUILD_ROOT/_virtualenv ?

Comment 7

[Gregory Szorc [:gps]]

(In reply to Mike Hommey [:glandium] from comment #6)
> ::: build/autoconf/python-virtualenv.m4
> @@ +52,5 @@
> >    dnl This verifies our Python version is sane and ensures the Python
> >    dnl virtualenv is present and up to date. It sanitizes the environment
> >    dnl for us, so we don't need to clean anything out.
> >    $PYTHON $_virtualenv_populate_path \
> > +    $_virtualenv_topsrcdir $MOZ_BUILD_ROOT $MOZ_BUILD_ROOT/_virtualenv || exit 1
> 
> Do we have a reason not to just do $PYTHON $_virtualenv_populate_path
> $_virtualenv_topsrcdir $MOZ_BUILD_ROOT ? IOW, do we have a reason to specify
> a virtualenv path that would not be $MOZ_BUILD_ROOT/_virtualenv ?

Not really. The only reason is populate_virtualenv.py as currently implemented is generic and could be used by any project. I could foresee a future where we may take advantage of this. Until then, YAGNI applies. But, I don't think it's worth any of our time to go back and change a reviewed patch.

Comment 8

[Gregory Szorc [:gps]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4682ed369545

Comment 9

[Jeff Hammel]

(In reply to Gregory Szorc [:gps] from comment #7)
> (In reply to Mike Hommey [:glandium] from comment #6)
> > ::: build/autoconf/python-virtualenv.m4
> > @@ +52,5 @@
> > >    dnl This verifies our Python version is sane and ensures the Python
> > >    dnl virtualenv is present and up to date. It sanitizes the environment
> > >    dnl for us, so we don't need to clean anything out.
> > >    $PYTHON $_virtualenv_populate_path \
> > > +    $_virtualenv_topsrcdir $MOZ_BUILD_ROOT $MOZ_BUILD_ROOT/_virtualenv || exit 1
> > 
> > Do we have a reason not to just do $PYTHON $_virtualenv_populate_path
> > $_virtualenv_topsrcdir $MOZ_BUILD_ROOT ? IOW, do we have a reason to specify
> > a virtualenv path that would not be $MOZ_BUILD_ROOT/_virtualenv ?
> 
> Not really. The only reason is populate_virtualenv.py as currently
> implemented is generic and could be used by any project. I could foresee a
> future where we may take advantage of this. Until then, YAGNI applies. But,
> I don't think it's worth any of our time to go back and change a reviewed
> patch.

Let's be careful here too, as long as we're talking about vantage points.  (This is all OT, btw, so feel free to skip this now.) populate_virtualenv.py has a huge intention overlap with pip (as well as other software not really worth mentioning) and while a populate_virtualenv.py is the right size now for its task I would really want to sit-on-hands a bit before going ahead with it as a general best-practice for our software.  I would love to reach out to pip, get anything we would need generic there, and figure out what to do to front end pip and take care of non-generic use cases (as this would no doubt be).  While there is more overhead in reaching out and collaborating here, what is gained is both the developer wisdom and ongoing involvement and investment of pip as well as both the prestige in the python community and influence that comes with it.  I'll truncate here.  Apologies for being off-topic, but I would like to put Mozilla's python craft, where applicable and not hung up in project politics, into generic tools where there is greater exposure(=testing) and interest, and mostly for Mozilla's sake. FWIW, I may be doing some work in the future on pip to provide specific functionality needed.

Comment 10

[Gregory Szorc [:gps]]

I couldn't agree with you more. populate_virtualenv.py was originally written (bug 774106) so we could code logic in Python and not in some Makefile.in. It has slowly expanded in scope. If there is an existing Python tool out there that does something similar or if we could convince an existing Python tool to gain the features we desire, we should pursue that.

Comment 11

[Jeff Hammel]

(In reply to Gregory Szorc [:gps] from comment #10)
> I couldn't agree with you more. populate_virtualenv.py was originally
> written (bug 774106) so we could code logic in Python and not in some
> Makefile.in. It has slowly expanded in scope. If there is an existing Python
> tool out there that does something similar or if we could convince an
> existing Python tool to gain the features we desire, we should pursue that.

To replace OOTB we'd need to be able to do everything in http://mxr.mozilla.org/mozilla-central/source/build/virtualenv/packages.txt with pip and presumably have to interpolate the file since things are relative to topsrcdir.  I'll ticket separately so not to further go off-topic here

Comment 12

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] from comment #8)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/4682ed369545

I don't know how this doesn't break the tree, but it does break incremental builds for me locally, with the following error:

Creating Python environment
Traceback (most recent call last):
  File "/home/mh/mozilla-central/build/virtualenv/populate_virtualenv.py", line 379, in <module>
    manager.ensure()
  File "/home/mh/mozilla-central/build/virtualenv/populate_virtualenv.py", line 100, in ensure
    if self.up_to_date():
  File "/home/mh/mozilla-central/build/virtualenv/populate_virtualenv.py", line 85, in up_to_date
    submanifest)
TypeError: __init__() takes exactly 6 arguments (5 given)

and indeed, the call in there hasn't been updated.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4682ed369545

Comment 14

[Jonathan Watt [:jwatt]]

Seems likely that this caused bug 843059.

Comment 15

[Gregory Szorc [:gps]]

Fixed bug reported in comment #12. IRC review.

https://hg.mozilla.org/mozilla-central/rev/9dbb23d8ab8a

Comment 16

[Jonathan Watt [:jwatt]]

Is that also likely to fix bug 843059? Or are you looking into that, or should we back out your patch?

Comment 17

[Gregory Szorc [:gps]]

Backed out for reftest bustage:

https://hg.mozilla.org/mozilla-central/rev/1e5d26d9e154
https://hg.mozilla.org/mozilla-central/rev/461e427960b2

Comment 19

[Jeff Hammel]

(In reply to Jeff Hammel [:jhammel] from comment #11)
> (In reply to Gregory Szorc [:gps] from comment #10)
> > I couldn't agree with you more. populate_virtualenv.py was originally
> > written (bug 774106) so we could code logic in Python and not in some
> > Makefile.in. It has slowly expanded in scope. If there is an existing Python
> > tool out there that does something similar or if we could convince an
> > existing Python tool to gain the features we desire, we should pursue that.
> 
> To replace OOTB we'd need to be able to do everything in
> http://mxr.mozilla.org/mozilla-central/source/build/virtualenv/packages.txt
> with pip and presumably have to interpolate the file since things are
> relative to topsrcdir.  I'll ticket separately so not to further go
> off-topic here

bug 843426

Comment 20

[Gregory Szorc [:gps]]

I was reminded of this today. I can't remember what the error was. So, I pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=3058f4f107be

Comment 21

[(not currently active) Ted Mielczarek]

I think this was backed out for bug 843059, because it broke running reftests (and presumably other tests) locally, because automation.py has hardcoded relative paths in it.

Comment 22

[Gregory Szorc [:gps]]

Hmmm. I think the try push looks green, does it not? Perhaps I should try re-landing this...

Comment 23

[(not currently active) Ted Mielczarek]

You didn't break the tree, you broke people running tests locally from their objdir.

Comment 24

[Gregory Szorc [:gps]]

Indeed. It seems when we add <objdir>/build to sys.path that automation.py from that directory is picked up instead of the automation.py in _tests. I, uh, am not sure how to fix this one short of hacking up everywhere automation.py is needed and ensuring sys.path order is sane.

Comment 25

[Gregory Szorc [:gps]]

Attachment: Add objdir paths to virtualenv, v2

In the objdir, we have automation.py in the following directories:

./_leaktest/automation.py
./_profile/pgo/automation.py
./_tests/reftest/automation.py
./_tests/testing/mochitest/automation.py
./build/automation.py
./build/pgo/automation.py
./layout/tools/reftest/automation.py
./testing/mochitest/automation.py

I looked at the mochitest and reftest Python test runners. There were all installing the script/local directory in sys.path[0] except for reftest's runner. It was appending to sys.path. I changed it to sys.path.insert(0, path) and reftests started working locally again.

I /think/ this is sufficient to fix the bustage with the original patch. If we find additional bustage, it should hopefully be a matter of replacing sys.path.append with sys.path.insert...

Comment 26

[Gregory Szorc [:gps]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ab7902d65bab

Comment 27

[Ryan VanderMeulen [:RyanVM]]

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