Remove mozharness' copy of virtualenv and use the one under /third_party/python instead

NEW
Unassigned

Status

enhancement
2 years ago
9 months ago

People

(Reporter: ahal, Unassigned, Mentored)

Tracking

(Blocks 2 bugs)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Reporter

Description

2 years ago
In bug 1297515 we copied the in-tree virtualenv to testing/mozharness/external_tools. We needed to do this to upgrade pip, since buildbot was downloading the 'testing/mozharness' directory directly from hg.m.o and there was no easy way to also package other directories.

This means there are now two copies of virtualenv in the tree. Once everything is off of buildbot, we can remove the copy under testing/mozharness/external_tools and instead package virtualenv into the mozharness.zip artifact that taskcluster builds produce.
Reporter

Comment 1

Last year
Catlee indicated this should be actionable now that everything is off buildbot. If anyone would like to try and take a stab at this I'm happy to mentor. Otherwise I may get around to this eventually :).
Mentor: ahalberstadt
We have various bits of releng automation (merge day which gets it from the archiver [1], l10n bumper which downloads it from hg.m.o as a tarball [2]) which would need to be updated to get virtualenv from the new location.

[1] https://github.com/mozilla-releng/releasewarrior-2.0/blob/master/docs/mergeduty/howto.md#connect-to-remote-instance-and-get-a-copy-of-mozharness
[2] https://hg.mozilla.org/build/puppet/file/tip/modules/l10n_bumper/templates/download_mozharness.sh.erb#l7
Assignee: nobody → catlee
Attachment #8976582 - Flags: review?(mozilla)

Comment 4

Last year
mozreview-review
Comment on attachment 8976582 [details]
Bug 1408051: Remove mozharness' copy of virtualenv

https://reviewboard.mozilla.org/r/244686/#review250744

::: testing/mozharness/mozharness/base/python.py:344
(Diff revision 1)
>          c = self.config
>          dirs = self.query_abs_dirs()
>          venv_path = self.query_virtualenv_path()
>          self.info("Creating virtualenv %s" % venv_path)
>  
> -        # Always use the virtualenv that is vendored since that is deterministic.
> +        for p in ('base_work_dir', 'abs_src_dir'):

This comment is probably worth keeping.

::: testing/mozharness/mozharness/base/python.py:510
(Diff revision 1)
>          if sys.version_info[:2] < (2, 7):
>              self.warning('Resource monitoring will not be enabled! Python 2.7+ required.')
>              return
>  
>          try:
> +            self.activate_virtualenv()

Does this want to be in the `try:` block? I think a failure here is distinct enough that it shouldn't be conveted to the generic error about the resource monitor.
Attachment #8976582 - Flags: review?(mozilla) → review+
Comment hidden (mozreview-request)

Comment 6

Last year
Pushed by catlee@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a00ab75aea2b
Remove mozharness' copy of virtualenv r=tomprince
Comment hidden (mozreview-request)

Comment 10

Last year
mozreview-review
Comment on attachment 8976582 [details]
Bug 1408051: Remove mozharness' copy of virtualenv

https://reviewboard.mozilla.org/r/244686/#review250914

::: python/mozbuild/mozbuild/action/test_archive.py:376
(Diff revision 3)
>              'pattern': 'mozharness/**',
>          },
> +        {
> +            'source': buildconfig.topsrcdir,
> +            'base': 'third_party/python/virtualenv',
> +            'dest': 'third_party/python/virtualenv',

Would it be better to prefix this with `mozharness/`? Or, maybe even better, change the prefix to match the old location: `mozharness/external_tools/virtualenv`? It is unusual to have a a zip or tarball extract to multiple top-level directories.

This means we don't need the changes to the taskcluster scripts (which isn't the reason), and some changes to the logic for finding the virtualenv location.
Attachment #8976582 - Flags: review+ → review-

Comment 11

Last year
mozreview-review-reply
Comment on attachment 8976582 [details]
Bug 1408051: Remove mozharness' copy of virtualenv

https://reviewboard.mozilla.org/r/244686/#review250914

> Would it be better to prefix this with `mozharness/`? Or, maybe even better, change the prefix to match the old location: `mozharness/external_tools/virtualenv`? It is unusual to have a a zip or tarball extract to multiple top-level directories.
> 
> This means we don't need the changes to the taskcluster scripts (which isn't the reason), and some changes to the logic for finding the virtualenv location.

To be clear:

We should definitely have mozharness.zip only have one top-level directory. I don't have strong feelings on where virtualenv goes inside of it.
(In reply to Tom Prince [:tomprince] from comment #11)
> Comment on attachment 8976582 [details]
> Bug 1408051: Remove mozharness' copy of virtualenv
> 
> https://reviewboard.mozilla.org/r/244686/#review250914
> 
> > Would it be better to prefix this with `mozharness/`? Or, maybe even better, change the prefix to match the old location: `mozharness/external_tools/virtualenv`? It is unusual to have a a zip or tarball extract to multiple top-level directories.
> > 
> > This means we don't need the changes to the taskcluster scripts (which isn't the reason), and some changes to the logic for finding the virtualenv location.
> 
> To be clear:
> 
> We should definitely have mozharness.zip only have one top-level directory.
> I don't have strong feelings on where virtualenv goes inside of it.

I wanted to try and keep the structure of mozharness.zip as close as possible to the original source directory. I'll look into moving virtualenv into mozharness/third_party/... inside the zip file.
Flags: needinfo?(catlee)
(In reply to Tom Prince [:tomprince] from comment #4)
> ::: testing/mozharness/mozharness/base/python.py:510
> (Diff revision 1)
> >          if sys.version_info[:2] < (2, 7):
> >              self.warning('Resource monitoring will not be enabled! Python 2.7+ required.')
> >              return
> >  
> >          try:
> > +            self.activate_virtualenv()
> 
> Does this want to be in the `try:` block? I think a failure here is distinct
> enough that it shouldn't be conveted to the generic error about the resource
> monitor.

Now I remember why I put this in. If the main create_virtualenv action fails, then activate_virtualenv() will also raise an exception in the post-action handler. The original exception from create_virtualenv is then lost. I think this is actually a more fundamental bug in how BaseScript is not logging exceptions from the main action methods.
Comment hidden (mozreview-request)

Comment 15

Last year
mozreview-review
Comment on attachment 8976582 [details]
Bug 1408051: Remove mozharness' copy of virtualenv

https://reviewboard.mozilla.org/r/244686/#review251104

::: taskcluster/scripts/tester/test-macosx.sh:41
(Diff revision 4)
>  
>      echo "Downloading mozharness"
>  
>      while [[ $attempt < $max_attempts ]]; do
>          if curl --fail -o mozharness.zip --retry 10 -L $MOZHARNESS_URL; then
> -            rm -rf mozharness
> +            rm -rf mozharness third_party

This change is not needed.
Attachment #8976582 - Flags: review?(mozilla) → review+
Comment hidden (mozreview-request)

Comment 17

Last year
Pushed by catlee@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5afad9b7fd21
Remove mozharness' copy of virtualenv r=tomprince
The jobs are taking like 20 minutes to set up their virtualenv on OSX. Not sure why yet....
Flags: needinfo?(catlee)
With some more debug logging, it looks like the new pip is spending a long time fetching indexes from https endpoints. e.g.

old:
18:13:47     INFO -    Getting page https://pypi.pub.build.mozilla.org/pub
18:13:47     INFO -    Starting new HTTPS connection (1): pypi.pub.build.mozilla.org
18:13:47     INFO -    "GET /pub HTTP/1.1" 301 246
18:13:47     INFO -    "GET /pub/ HTTP/1.1" 200 None
18:13:48     INFO -    Analyzing links from page https://pypi.pub.build.mozilla.org/pub/

new:
15:38:39     INFO -    Getting page https://pypi.pub.build.mozilla.org/pub
15:38:39     INFO -    Starting new HTTPS connection (1): pypi.pub.build.mozilla.org
15:38:39     INFO -    https://pypi.pub.build.mozilla.org:443 "GET /pub HTTP/1.1" 301 246
15:38:39     INFO -    https://pypi.pub.build.mozilla.org:443 "GET /pub/ HTTP/1.1" 200 None
15:40:40     INFO -    Analyzing links from page https://pypi.pub.build.mozilla.org/pub/

Aki found this, which may be relevant: https://github.com/pyca/pyopenssl/issues/625
Assignee: catlee → nobody
Blocks: 1201085
Reporter

Comment 21

9 months ago
It might be worth upgrading our in-tree pip and trying again (current is 9.0.3, released is 18.0). Also this reply has a workaround that seems worth investigating:
https://github.com/pyca/pyopenssl/issues/625#issuecomment-300322038
You need to log in before you can comment on or make changes to this bug.