1668718 - sitecustomize.py in mach virtualenv import mach_bootstrap which is not available

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Comment 1

[Mike Hommey [:glandium]]

I was writing a commit message for a simple fix, but it turns out there's more to it.

• sitecustomize.py contains an import of the mach_bootstrap module, but that can only work when populate_local_paths is set, which is not the case for the mach virtualenv. So in theory, the import shouldn't be there if populate_local_paths is set (that's what I was fixing in my local commit).
• the code the mach_bootstrap module import is meant to trigger is there to avoid loading .pyc files when there isn't a corresponding .py file in the same directory. That is actually not necessary with python 3, which puts the .pyc files in a separate __pycache__ directory, and if the corresponding .py file disappears, the .pyc files in there are ignored. But it's still necessary in python 2. It would actually be good to have the mach python2 virtualenv have this workaround so the simple workaround is not enough... (see bug 1188224)
  Ricky, WDYT?

Comment 2

[Ricky Stewart]

Mike, did you see this at HEAD, or only atop https://phabricator.services.mozilla.com/D92173?

Weird how nobody's caught this until now. That virtualenv works perfectly for me, so I can only assume that the sitecustomize isn't actually getting run on my machine... That's something to follow up on, first of all. I wonder if my patch introduced the issue or if this problem has been around for longer.

Setting populate_local_paths in the .mozbuild virtualenvs is a non-starter. That would make it unnecessarily difficult to do work on multiple trees. i.e., there's a reason we don't populate_local_paths.

So I'm in favor of doing the following:

1. Don't import mach_bootstrap in the .mozbuild virtualenvs. It'll get imported anyway when mach starts up.

2. Only do the import mach_bootstrap logic for the in-objdir Python 2 virtualenv.

3. As a follow-up, figure out why my (and everyone's?) .mozbuild virtualenv's sitecustomize package isn't working.

Comment 3

[Ricky Stewart]

Attachment: Bug 1668718 - Don't `import mach_bootstrap` for `virtualenv`s that don't have `populate_local_paths` set, or in Python 3

Before, this would be written to sitecustomize.py irrespective of the value of populate_local_paths. This doesn't make sense -- since the local paths aren't included in the virtualenv's PYTHONPATH when Python starts up, it doesn't know how to import mach_bootstrap. Since on mach startup the import hook will be loaded anyway, and the virtualenvs in ~/.mozbuild (i.e. the only virtualenvs for which we don't populate_local_paths) are just used to run mach, this is fine and won't regress anything.

Also, since the import hook is only necessary for Python 2, add a couple conditional checks to get rid of the added overhead when we're running with Python 3.

Comment 4

[Ricky Stewart]

Evidently sitecustomize ignores some errors under certain mysterious circumstances. :(

1. If I import mach_bootstrap in my ~/.mozbuild sitecustomize, the Python works fine (but the failure to import mach_bootstrap gets silently ignored).

2. If I import a package that DEFINITELY does not exist ANYWHERE, like mach_bootstrap_asdlfkjsdfkjdf, then again, the Python works fine and the failure is silently ignored.

3. If I throw some other exception, e.g. by dividing by zero, then finally the Python fails to start up and prints a helpful message.

I can't find this behavior documented -- at least, not exactly. There's this:

After these path manipulations, an attempt is made to import a module named sitecustomize, which can perform arbitrary site-specific customizations. It is typically created by a system administrator in the site-packages directory. If this import fails with an ImportError or its subclass exception, and the exception’s name attribute equals to 'sitecustomize', it is silently ignored.

... but the .name attribute of the exception thrown would be mach_bootstrap, not sitecustomize, so this doesn't really make sense. I assume that the old virtualenv we're using doesn't emulate this behavior exactly, which would explain why you're encountering the problem now. So I assume no other work is required on our end and any other possible issues like this would be addressed by the virtualenv upgrade.

Comment 5

[Ricky Stewart]

ImportErrors when importing sitecustomize used to be unilaterally ignored until Python 3.7.

Comment 6

[Ricky Stewart]

Yup.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fb38825fa2ff
Don't `import mach_bootstrap` for `virtualenv`s that don't have `populate_local_paths` set, or in Python 3 r=mhentges,firefox-build-system-reviewers

Comment 8

[Narcis Beleuzu [:NarcisB]]

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

Comment 10

[Mike Hommey [:glandium]]

Considering bug 1669639, we may want to uplift this to all branches, including esr.

Comment 11

[Ricky Stewart]

The change to python/mozbuild/mozbuild/virtualenv.py probably won't uplift cleanly in general (it involves the populate_local_paths machinery that was introduced in the last couple months and has gone through several iterations), but the change to build/mach_bootstrap.py will, and that's what matters at least with respect to bug 1669639. So I can get another patch going just for the uplift.

Comment 12

[Ricky Stewart]

Attachment: Bug 1668718 - Don't set `import` hook for Python 3 in `mach bootstrap`

Comment 13

[Ricky Stewart]

Comment on attachment 9180270 [details]
Bug 1668718 - Don't set import hook for Python 3 in mach bootstrap

Beta/Release Uplift Approval Request

• User impact if declined: Builds can fail a la bug 1669639.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This gets rid of code that was relevant in Python 2 but not Python 3 (Python 3 being what we use to build).
• String changes made/needed:

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Without this patch, builds can fail a la bug 1669639.
• User impact if declined: Builds can fail a la bug 1669639.
• Fix Landed on Version: 83
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This gets rid of code that was relevant in Python 2 but not Python 3 (Python 3 being what we use to build).
• String or UUID changes made by this patch:

Comment 14

[Julien Cristau [:jcristau] (back August 18)]

Comment on attachment 9180270 [details]
Bug 1668718 - Don't set import hook for Python 3 in mach bootstrap

a=npotb

Comment 15

[Julien Cristau [:jcristau] (back August 18)]

https://hg.mozilla.org/releases/mozilla-beta/rev/5e652949c152

Comment 16

[Julien Cristau [:jcristau] (back August 18)]

https://hg.mozilla.org/releases/mozilla-esr78/rev/e2fade7a3527

Comment 17

[Pascal Chevrel:pascalc]

Comment on attachment 9180270 [details]
Bug 1668718 - Don't set import hook for Python 3 in mach bootstrap

Approved for 81.0.2, thanks.

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-release/rev/c1986de311b0

Comment 19

[Mitchell Hentges [:mhentges] 🦀]

Comment on attachment 9179358 [details]
Bug 1668718 - Don't import mach_bootstrap for virtualenvs that don't have populate_local_paths set, or in Python 3

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Resolves local-build sharp edge
• User impact if declined: Without this patch, local builds will fail if using system, read-only python.
  If using write-able python, causes import warnings to appear whenever python is run (even outside of Firefox)
• Fix Landed on Version: 83
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Build patch
• String or UUID changes made by this patch: