Closed Bug 1255585 Opened 4 years ago Closed 4 years ago

Python virtualenv is new, and build backend is regenerated, with every |mach build mobile/android| Fennec artifact build

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox48 fixed)

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: nalexander, Assigned: chmanchester)

References

Details

Attachments

(2 files)

This is a Fennec artifact mozconfig (i.e., with --enable-artifact-builds).  I constantly see "New python executable in /Users/nalexander/Mozilla/gecko/objdir-droid/_virtualenv/bin/python2.7" when running the (automatic) artifact install; and the build backend is regenerated each build as well.
glandium: is there a way to figure out what is triggering the build-backend?
Flags: needinfo?(mh+mozilla)
With the diff at the end of this comment, I see that the offending file is /Users/nalexander/Mozilla/gecko/objdir-droid/_virtualenv/lib/python2.7/sitecustomize.py.

nalexander@chocho ~/M/gecko> ./mach build mobile/android -v
 0:00.20 /usr/bin/make -C /Users/nalexander/Mozilla/gecko/objdir-droid -j8 -s backend
 0:00.24 Build configuration changed. Regenerating backend.
 0:00.24 /Users/nalexander/Mozilla/gecko/objdir-droid/_virtualenv/lib/python2.7/sitecustomize.py
 0:00.24 Done.
 0:00.39 Reticulating splines...

nalexander@chocho ~/M/gecko> hg diff
diff --git a/Makefile.in b/Makefile.in
--- a/Makefile.in
+++ b/Makefile.in
@@ -109,16 +109,18 @@ backend: $(BUILD_BACKEND_FILES)
 #     backend%RecursiveMakeBackend backend%FasterMakeBackend:
 #         @echo do stuff
 #     backend: backend.RecursiveMakeBackend backend.FasterMakeBackend
 # would only execute the command once.
 #
 # Credit where due: http://stackoverflow.com/questions/2973445/gnu-makefile-rule-generating-a-few-targets-from-a-single-source-file/3077254#3077254
 $(subst .,%,$(BUILD_BACKEND_FILES)):
        @echo 'Build configuration changed. Regenerating backend.'
+       @echo $?
+       @echo 'Done.'
        $(PYTHON) config.status

 Makefile: $(BUILD_BACKEND_FILES)
        @$(TOUCH) $@

 define build_backend_rule
 $(1)_files := $$(shell cat $(1).in)
 $(1): $$($(1)_files)
I'm not seeing this on a desktop build.
Attached file out
nalexander@chocho ~/M/gecko> rm -rf ; and ./mach build > out

(With some trivial patches to print when we're writing sitecustomize.py.)
OK, the issue under all this is that |mach artifact install| (and probably other mach commands that activate the virtualenv) don't specify if they want sys.executable_path or the virtualenv Python path at https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/base.py#666.  After Bug 1253502, this matters.  This manifests as back-to-back |mach artifact install| invocations writing two virtual environments.  I'm not sure if the correct solution is to always specify the virtual Python path, but that solves the issue locally:

	Modified   python/mozbuild/mozbuild/base.py
diff -r 0837015740ff python/mozbuild/mozbuild/base.py
--- a/python/mozbuild/mozbuild/base.py	Thu Mar 10 21:05:05 2016 +0100
+++ b/python/mozbuild/mozbuild/base.py	Thu Mar 10 14:30:45 2016 -0800
@@ -662,9 +662,9 @@ class MozbuildObject(ProcessExecutionMix
         return cls(self.topsrcdir, self.settings, self.log_manager,
             topobjdir=self.topobjdir)
 
     def _activate_virtualenv(self):
-        self.virtualenv_manager.ensure()
+        self.virtualenv_manager.ensure(python=self.virtualenv_manager.python_path)
         self.virtualenv_manager.activate()
 
 
 class MachCommandBase(MozbuildObject):
That most definitely looks wrong. The python path to pass there is not sys.executable vs. virtualenv python. It's sys.executable vs. some entirely different python.
Flags: needinfo?(mh+mozilla)
Assignee: nobody → cmanchester
(In reply to Mike Hommey [:glandium] from comment #6)
> That most definitely looks wrong. The python path to pass there is not
> sys.executable vs. virtualenv python. It's sys.executable vs. some entirely
> different python.

So is the real issue that up_to_date is supposed to be true when we pass sys.executable and the virtualenv Python is installed?  'cuz on my device, it's not -- the system Python and the virtualenv Python have different sizes (after following lots of symlinks).  I can verify that's how up_to_date fails if needed.
Flags: needinfo?(mh+mozilla)
wtf? why are your system python and virtualenv python different?
Flags: needinfo?(mh+mozilla)
(In reply to Mike Hommey [:glandium] from comment #8)
> wtf? why are your system python and virtualenv python different?

No idea.

nalexander@chocho ~/M/gecko> which python
/usr/local/bin/python
nalexander@chocho ~/M/gecko> ls -al /usr/local/bin/python
lrwxr-xr-x  1 nalexander  admin  34 21 Feb 11:39 /usr/local/bin/python -> ../Cellar/python/2.7.11/bin/python
nalexander@chocho ~/M/gecko> ls -al /usr/local/Cellar/python/2.7.11/bin/python
lrwxr-xr-x  1 nalexander  admin  54 22 Jan 08:29 /usr/local/Cellar/python/2.7.11/bin/python -> ../Frameworks/Python.framework/Versions/2.7/bin/python
nalexander@chocho ~/M/gecko> ls -al /usr/local/Cellar/python/2.7.11/Frameworks/Python.framework/Versions/2.7/bin/python2.7
-rwxr-xr-x  1 nalexander  admin  13316 21 Feb 11:38 /usr/local/Cellar/python/2.7.11/Frameworks/Python.framework/Versions/2.7/bin/python2.7

Also:

nalexander@chocho ~/M/gecko> ls -al /usr/local/bin/python2.7
lrwxr-xr-x  1 nalexander  admin  37 21 Feb 11:39 /usr/local/bin/python2.7 -> ../Cellar/python/2.7.11/bin/python2.7

However:

nalexander@chocho ~/M/gecko> ls -al objdir-droid/_virtualenv/bin/python
lrwxr-xr-x  1 nalexander  staff  9 10 Mar 14:34 objdir-droid/_virtualenv/bin/python -> python2.7
nalexander@chocho ~/M/gecko> ls -al objdir-droid/_virtualenv/bin/python2.7
-rwxr-xr-x  1 nalexander  staff  12568 10 Mar 14:34 objdir-droid/_virtualenv/bin/python2.7
nalexander@chocho ~/M/gecko> python --version
Python 2.7.11
nalexander@chocho ~/M/gecko> ./mach python --version
Python 2.7.11

This is after a fresh clobber.  Is it possible that some other Python is being found, instead of the one I installed via homebrew?
that is what this suggests...
I was able to reproduce after upgrading Python on my macbook. Virtualenv is finding itself a different Python, here: https://dxr.mozilla.org/mozilla-central/rev/dd1abe874252e507b825a0a4e1063b0e13578288/python/virtualenv/virtualenv.py#1257
Virtualenv will sometimes find a different executable from its sys.executable on OS X,
causing a check in the build system comparing filesizes between sys.executable and virtualenv
python to fail, resulting in clobbering and re-building the virtualenv every time the virtualenv
is activated, causing the build backend and more to be re-built.

Instead of checking file sizes, this commit causes us to compare the output of sys.version
to estimate compatibility between interpreters.

Review commit: https://reviewboard.mozilla.org/r/39367/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39367/
Attachment #8729329 - Flags: review?(gps)
Nick, can you verify the fix in comment 12?
Flags: needinfo?(nalexander)
(In reply to Chris Manchester (:chmanchester) from comment #13)
> Nick, can you verify the fix in comment 12?

Yes, this works for me locally.  Thanks!
Flags: needinfo?(nalexander)
There is code in python/virtualenv/virtualenv.py that mucks about with the python binary in the virtualenv. See "mach_o_change" for one example. So yeah, sniffing file size feels fragile.
Comment on attachment 8729329 [details]
MozReview Request: Bug 1255585 - Prevent Python executable mis-match from constantly clobbering the virtualenv on OS X. r=gps

https://reviewboard.mozilla.org/r/39367/#review36177

I'm not a fan of having to spawn processes to perform this check. This will add dozens of milliseconds of overhead on Windows.

I think what we should do instead is write a file into the virtualenv when it is created giving info about the source and virtualenv python executables, notably path and file size. We can read this state file and compare against filesystem state to see if things have changed and act accordingly.

Will that work?
Attachment #8729329 - Flags: review?(gps)
Comment on attachment 8729329 [details]
MozReview Request: Bug 1255585 - Prevent Python executable mis-match from constantly clobbering the virtualenv on OS X. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39367/diff/1-2/
Comment 17 is updated with gps' suggestion. I'll re-request review once I'm able to test this on OS X.
Attachment #8729329 - Flags: review?(gps)
Comment on attachment 8729329 [details]
MozReview Request: Bug 1255585 - Prevent Python executable mis-match from constantly clobbering the virtualenv on OS X. r=gps

https://reviewboard.mozilla.org/r/39367/#review36263

Assuming you've verified this actually works, land it!
Attachment #8729329 - Flags: review?(gps) → review+
Comment on attachment 8729329 [details]
MozReview Request: Bug 1255585 - Prevent Python executable mis-match from constantly clobbering the virtualenv on OS X. r=gps

https://reviewboard.mozilla.org/r/39367/#review36265

::: python/mozbuild/mozbuild/virtualenv.py:107
(Diff revision 2)
> +            print(python, file=fh)
> +            print(os.path.getsize(python), file=fh)

print() here is kinda wonky. Typically fh.write() is used. I /think/ print() will add CRLF on Windows where fh.write() must be told the line ending. But it shouldn't matter for this use case.
Attachment #8729329 - Flags: review+
(In reply to Gregory Szorc [:gps] from comment #20)
> Comment on attachment 8729329 [details]
> MozReview Request: Bug 1255585 - Prevent Python executable mis-match from
> constantly clobbering the virtualenv on OS X. r=gps
> 
> https://reviewboard.mozilla.org/r/39367/#review36265
> 
> ::: python/mozbuild/mozbuild/virtualenv.py:107
> (Diff revision 2)
> > +            print(python, file=fh)
> > +            print(os.path.getsize(python), file=fh)
> 
> print() here is kinda wonky. Typically fh.write() is used. I /think/ print()
> will add CRLF on Windows where fh.write() must be told the line ending. But
> it shouldn't matter for this use case.

I don't think this r+ was meant to be cancelled, because an issue wasn't opened with it... I'll just address this comment and land.
Comment on attachment 8729329 [details]
MozReview Request: Bug 1255585 - Prevent Python executable mis-match from constantly clobbering the virtualenv on OS X. r=gps

https://reviewboard.mozilla.org/r/39367/#review36269

No it wasn't meant to be cancelled. WTF MozReview.
Attachment #8729329 - Flags: review+
That's a fun failure.

On Windows, you can't overwrite or delete a file/executable that is currently running. So if we're trying to reinstall the virtualenv from a Python process running from that virtualenv, you'll get a permissions error. That might be what happened here. We could work around it with a clobber in automation. But this may be an issue for local developers.
Also appears to have left bld-lion-r5-080 in a permanently broken state, so that's nice.
Windows bustage was just a matter of normalizing paths. On OS X though, somewhere between mozharness' own virtualenv and multiple objdirs a consistent Python is too much to ask for, so we attempt to re-create the virtualenv when we shouldn't, and somehow create a circular symlink. In particular, /tools/python27/bin/python2.7 is used to create the virtualenv, but we see /tools/buildbot/bin/python when we try to run printconfigsetting.py after the build.

In light of this I think we should dump hexversion in the state file instead of using a path, which still mostly checks what we want, and works fine on automation: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3b6e3762e2a
Comment on attachment 8729329 [details]
MozReview Request: Bug 1255585 - Prevent Python executable mis-match from constantly clobbering the virtualenv on OS X. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39367/diff/2-3/
Attachment #8729329 - Flags: review+ → review?(gps)
Comment on attachment 8729329 [details]
MozReview Request: Bug 1255585 - Prevent Python executable mis-match from constantly clobbering the virtualenv on OS X. r=gps

https://reviewboard.mozilla.org/r/39367/#review36411

I still think we could have got the path based approach to work. But let's try this.
Attachment #8729329 - Flags: review?(gps) → review+
https://hg.mozilla.org/mozilla-central/rev/a1a8876174c3
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.