1549186 - mach python state is stored in objdir but refers to srcdir, which causes unexplained breakage if you switch srcdirs but reuse the objdir

Status: NEW

(Firefox Build System :: General, defect, P3)

Description

[:Gijs (he/him)]

I have linting setup as a commit hook using:

[hooks]
pretxncommit.lint = python:path/to/repo/tools/lint/hooks.py:hg

Now when I commit, I get:

Traceback (most recent call last):
  File "/path/to/repo/python/mozlint/mozlint/roller.py", line 137, in setup
    res = findobject(linter['setup'])(**self.lintargs)
  File "/path/to/repo/python/mozlint/mozlint/pathutils.py", line 216, in findobject
    obj = __import__(modulepath)
  File "/path/to/repo/build/mach_bootstrap.py", line 392, in __call__
    module = self._original_import(name, globals, locals, fromlist, level)
  File "/path/to/repo/tools/lint/python/l10n_lint.py", line 14, in <module>
    from compare_locales.lint.linter import L10nLinter
  File "/path/to/repo/build/mach_bootstrap.py", line 392, in __call__
    module = self._original_import(name, globals, locals, fromlist, level)
ImportError: No module named lint.linter
error: problem with lint setup, skipping l10n

It shouldn't be dumping python exceptions in my stdout/stderr.

Comment 1

[Axel Hecht [:Pike]]

I can't reproduce this locally. I added this to my .hg/hgrc, and the linter runs and reports, w/out problems.

Gijs, do you have more steps to reproduce? Which version of m-c are you based on, and did the patch you wrote affect l10n? My hunch is that it didn't have to, but IDK.

Maybe the OBJDIR/_virtualenvs/init environment was out of date with your source tree?

Comment 2

[:Gijs (he/him)]

OK, per chat with Axel on IRC, we figured some of this out. Detailed STR:

prerequisites: install mozconfigwrapper, create an artifact build config with objdir, let's call it $OBJDIR .

0. cd ~/tmp
1. hg clone https://hg.mozilla.org/mozilla-unified/
2. hg share mozilla-unified inbound
3. buildwith myartifactconfig
4. cd mozilla-unified
5. hg up -r e8aebe488b2f # note: this is from April 29 (currently a week ago) and doesn't have the python linter; m-c tip does.
6. ./mach build
7. cd ../inbound
8. hg up -r central
9. ./mach lint

Note that somehow, ./mach python is using modules from the other source dir (thanks Axel for these steps):

$ ./mach python                                                                                                                                                                                                                                    
Python 2.7.15 (default, Feb 12 2019, 12:27:02) 
[GCC 4.2.1 Compatible Apple LLVM 10.0.0 (clang-1000.11.45.5)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import compare_locales
>>> compare_locales.__path__
['path/to/mozilla-unified/third_party/python/compare-locales/compare_locales']

. Removing $OBJDIR/_virtualenv/init does not fix this. ./mach clobber does, but (a) ./mach should cope, and (b) it's not clear where this state lives and/or what is broken when it does break.

Comment 3

[Andrew Halberstadt [:ahal]]

I've never really liked the fact that the virtualenvs were stored in the objdir, but I don't have a build peer's perspective and my understanding is that there was some sort of good reason we put it there. Or maybe not.. now that we have a readily-available srcdir specific state dir, maybe it's time to revisit.

This will ultimately be a decision made by someone on :kmoir's team though.

Comment 4

[Chris Manchester (limited bugmail, email directly)]

This is an unfortunate situation, but sharing an objdir between srcdirs is breaking a lot of assumptions in the build system. I was able to reproduce this with the steps in comment 2, but even |./mach build| in the new srcdir fails with an error that seems to suggest a clobber. There may be more we can do to guard against this in mach. I'll also note that mozconfigwrapper provides a per-srcdir objdir by default.

Comment 5

[:Gijs (he/him)]

(In reply to Chris Manchester (:chmanchester) from comment #4)

I'll also note that mozconfigwrapper provides a per-srcdir objdir by default.

Can you elaborate on what this means?

Comment 6

[Chris Manchester (limited bugmail, email directly)]

(In reply to :Gijs (he/him) from comment #5)

(In reply to Chris Manchester (:chmanchester) from comment #4)

I'll also note that mozconfigwrapper provides a per-srcdir objdir by default.

Can you elaborate on what this means?

When I tested this mozconfigwrapper generated an objdir that was under the srcdir, presumably from this: https://github.com/ahal/mozconfigwrapper/blob/0df8b20eb240846866603d184166345ebe1cce40/mozconfigwrapper/template#L1 I wasn't able to reproduce until I replaced that objdir with an absolute path.

Comment 7

[:Gijs (he/him)]

(In reply to Chris Manchester (:chmanchester) from comment #6)

(In reply to :Gijs (he/him) from comment #5)

(In reply to Chris Manchester (:chmanchester) from comment #4)

I'll also note that mozconfigwrapper provides a per-srcdir objdir by default.

Can you elaborate on what this means?

When I tested this mozconfigwrapper generated an objdir that was under the srcdir, presumably from this: https://github.com/ahal/mozconfigwrapper/blob/0df8b20eb240846866603d184166345ebe1cce40/mozconfigwrapper/template#L1 I wasn't able to reproduce until I replaced that objdir with an absolute path.

Oh, I see. I didn't think we really were in favour of objdir-in-srcdir, but OK, that makes sense.

Comment 8

[Andrew Halberstadt [:ahal]]

Just noting that I wouldn't use mozconfigwrapper's default as any indication of best practice :). I'm happy to change that default if they should be placed elsewhere.

Comment 9

[Andrew Halberstadt [:ahal]]

Chris, out of curiosity.. can you think of any reason not to move the virtualenv from $OBJDIR to ~/.mozbuild/srcdirs/<src> ?

Comment 10

[Chris Manchester (limited bugmail, email directly)]

The issue here is that the build system isn't detecting moving between srcdirs, it's not clear to me moving the virtualenvs would be the most straightforward way to fix that, as I mention above the ensuing build in this case will fail when clearly we should be running configure and re-building lots of things. When you mention srcdir specific state dir that sounds an awful lot like an objdir: the objdir's python packages refer to the code in a srcdir, the sources compiled come from a specific srcdir, etc. It's probably fine if there are compelling reasons to do it, but I'm not sure this bug is one, and this might have counter-intuitive effects, for instance if |./mach clobber| didn't clear the state dir out as well.

Comment 11

[:Gijs (he/him)]

If we stored this in the source dir, and I switched mozconfigs (which have different objdirs) on the same source dir, would that break anything? Because I do that all the time and I'd prefer it if that didn't require clobbers...

Comment 12

[Andrew Halberstadt [:ahal]]

I don't think it would break anything, but I won't guarantee it.

Comment 13

[James Graham [:jgraham]]

FWIW for the virtualenv part specifically https://virtualenv.pypa.io/en/legacy/userguide.html#making-environments-relocatable exists, although it does come with a bunch of warnings about possibly breaking things.

Comment 14

[Nicolas B. Pierron [:nbp]]

(I am not a build system peer)

My understanding of the problem is that the object directory is reused across source directories, and does not dependent on the same source directory used to create it.

Maybe a when mach is started from the source directory, it could double check and raise an error if the object directory's source does not match, and simply suggest to clobber first.

(In reply to Andrew Halberstadt [:ahal] from comment #9)

Chris, out of curiosity.. can you think of any reason not to move the virtualenv from $OBJDIR to ~/.mozbuild/srcdirs/<src> ?

Yes, I think the best reason is that allowing sharing objdir is an unended thread of errors. For example, what would happen if you compile both in parallel?

-- edit: I miss-understood the question, which actually suggested the same thing.

I think the best option here are either to raise an error and clobber or have the $OBJDIR path dependent on the $SRCDIR.