1670167 - ImportError: No module named hgdemandimport when building

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Timothy Nikkel (:tnikkel)]

0:02.81 checking for Mercurial version...
0:02.81 DEBUG: Executing: /usr/local/bin/hg --version
0:02.81 DEBUG: The command returned non-zero exit status 1.
0:02.81 DEBUG: Its error output was:
0:02.81 DEBUG: | Traceback (most recent call last):
0:02.81 DEBUG: | File "/usr/local/bin/hg", line 30, in <module>
0:02.81 DEBUG: | from hgdemandimport import tracing
0:02.81 DEBUG: | ImportError: No module named hgdemandimport
0:02.81 ERROR: Command /usr/local/bin/hg --version failed with exit status 1.
0:02.86 *** Fix above errors and then restart with "./mach build"
0:02.86 make: *** [configure] Error 1

This is on macos.

Bisected to https://hg.mozilla.org/mozilla-central/rev/91f4934e4063 bug 1654457

I took the steps at https://bugzilla.mozilla.org/show_bug.cgi?id=1669934#c2 but that didn't fix it. I've run mach bootstrap as well.

Comment 1

[Mike Hommey [:glandium]]

I can reproduce on a changeset before bug 1654457, and the reason is this hunk in bug 1656993: https://phabricator.services.mozilla.com/D85916#change-AuXbO8dwkvCc
It seems this hunk should be removed, but I also don't know why Ricky added it.

Comment 2

[Timothy Nikkel (:tnikkel)]

I removed those lines from python/mozbuild/mozbuild/controller/building.py and I still get the same error building. (I repeated the steps https://bugzilla.mozilla.org/show_bug.cgi?id=1669934#c2 just to be sure) Perhaps you are reproducing a different issue (since I bisected it to the specific changeset mentioned)?

Comment 3

[Mike Hommey [:glandium]]

(In reply to Timothy Nikkel (:tnikkel) from comment #2)

I removed those lines from python/mozbuild/mozbuild/controller/building.py and I still get the same error building. (I repeated the steps https://bugzilla.mozilla.org/show_bug.cgi?id=1669934#c2 just to be sure) Perhaps you are reproducing a different issue (since I bisected it to the specific changeset mentioned)?

I bet running /usr/local/bin/hg --version fails with the same error, then.

Comment 4

[Timothy Nikkel (:tnikkel)]

(In reply to Mike Hommey [:glandium] from comment #3)

(In reply to Timothy Nikkel (:tnikkel) from comment #2)

I removed those lines from python/mozbuild/mozbuild/controller/building.py and I still get the same error building. (I repeated the steps https://bugzilla.mozilla.org/show_bug.cgi?id=1669934#c2 just to be sure) Perhaps you are reproducing a different issue (since I bisected it to the specific changeset mentioned)?

I bet running /usr/local/bin/hg --version fails with the same error, then.

It runs fine actually. I checked that before filing and double checked just now.

Comment 5

[Mike Hommey [:glandium]]

So, what I reproduced in comment 1 is a different variant when the hashbang for hg is e.g. /usr/bin/env python, and does point to the building.py hunk. Timothy's variant, though, was reproducible on my MBP when installing mercurial as a python2 app instead of python3, and the problem happens because of PYTHONEXECUTABLE being set in https://searchfox.org/mozilla-central/source/build/moz.configure/init.configure#365

Comment 6

[Mike Hommey [:glandium]]

Attachment: Bug 1670167 - Unset PYTHONEXECUTABLE once python configure has been reexecuted in the virtualenv.

Comment 7

[Mike Hommey [:glandium]]

Ricky, I'm still interested in the answer to comment 1.

Comment 8

[BugBot [:suhaib / :marco]]

Set release status flags based on info from the regressing bug 1656993

Comment 9

[Ricky Stewart]

The idea was to enable us to do this. Basically, in that patch, it became a little more error-prone to do the "are we in the virtualenv right now?" check, and the version of the code that exists in that function right now (where we try to import mozfile and switch on whether the import succeeded) is the version of that check that works the best that I was able to find.

Two thoughts:

1. If the use of ImportError in this way is problematic we can just add an @imports(_from='six.moves.builtins', _import='ImportError'), which is probably what I "should" have done in the first place tbh.

2. Uh, why does this "work"? Looking at it with fresh eyes, this is obviously wrong, we're not supposed to import stuff directly in configure. Little surprised that this isn't unconditionally broken and that the configure lint still passes.

Comment 10

[Mike Hommey [:glandium]]

(In reply to Ricky Stewart from comment #9)

The idea was to enable us to do this.

How is it related to https://phabricator.services.mozilla.com/D85916#change-AuXbO8dwkvCc ?

Comment 11

[Mike Hommey [:glandium]]

Oh I see what's going on, the link is not showing the right thing... sigh... I meant to link the building.py change.

Comment 12

[Mike Hommey [:glandium]]

Although unrelated to this bug...

(In reply to Ricky Stewart from comment #9)

2. Uh, why does this "work"? Looking at it with fresh eyes, this is obviously wrong, we're not supposed to import stuff directly in configure.

It doesn't work. If you look what code path is taken, it's the except ImportError, but if you inspect the error, it's this one: https://searchfox.org/mozilla-central/rev/165e8d8f80c02605c2f3e89c5de462abfce91c32/python/mozbuild/mozbuild/configure/__init__.py#249

Comment 13

[Ricky Stewart]

Oh I see what's going on, the link is not showing the right thing... sigh... I meant to link the building.py change.

Okay, I've been playing with this a bit and I have no idea what this was meant to do. I assume it's an unnecessary artifact given how long I was working on this patch. We can delete it (and if something breaks we can deal with the fallout, TBH it's my bad for not commenting this code).

It doesn't work. If you look what code path is taken, it's the except ImportError, but if you inspect the error, it's this one: https://searchfox.org/mozilla-central/rev/165e8d8f80c02605c2f3e89c5de462abfce91c32/python/mozbuild/mozbuild/configure/__init__.py#249

Yes, this is what I was expecting would be the case. That should be fixed.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/06ae775cc1e4
Unset PYTHONEXECUTABLE once python configure has been reexecuted in the virtualenv. r=firefox-build-system-reviewers,andi,rstewart

Comment 15

[Ricky Stewart]

Attachment: Bug 1670167 - Fix janky Python 3 logic in `configure` from bug 1656993

The try: import mozfile; except ImportError: ... thing was completely broken since it unconditionally triggered this exception. The MOZBUILD_VIRTUALENV thing should work now, so just use that as a signal for whether we're already in the virtualenv instead.

Also delete some PATH munging in building.py, which I think should be unnecessary after we make the above change.

Comment 16

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/06ae775cc1e4

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8ac3e9149928
Fix janky Python 3 logic in `configure` from bug 1656993 r=firefox-build-system-reviewers,glandium

Comment 18

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/8ac3e9149928

Comment 19

[Mike Hommey [:glandium]]

Ricky's landing should have fixed the remaining things I left the bug open for.

Comment 20

[Mitchell Hentges [:mhentges] 🦀]

Comment on attachment 9180637 [details]
Bug 1670167 - Unset PYTHONEXECUTABLE once python configure has been reexecuted in the virtualenv.

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Resolves build-breaking regression from 1654457.
• User impact if declined: Builds will fail unless 1654457 is also declined
• 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:

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9180796 [details]
Bug 1670167 - Fix janky Python 3 logic in configure from bug 1656993

I don't think this is needed since bug 1656993 wasn't uplifted to ESR78. It has conflicts anyway.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9180637 [details]
Bug 1670167 - Unset PYTHONEXECUTABLE once python configure has been reexecuted in the virtualenv.

Approved for 78.8esr.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

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

Comment 24

[Mitchell Hentges [:mhentges] 🦀]

That makes sense, thanks!