Closed Bug 1043144 Opened 5 years ago Closed 5 years ago

machc generated in working dir after build, not ignored by .hgignore/.gitignore

Categories

(Firefox Build System :: Mach Core, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla34

People

(Reporter: Unfocused, Assigned: gps)

Details

Attachments

(1 file, 4 obsolete files)

machc is generated in the root of the working directory during a build. I don't like it! Should make HG/Git ignore it.
Attached patch Patch v1 (obsolete) — Splinter Review
Attachment #8461308 - Flags: review?(mh+mozilla)
Are we attempting an import of "mach" from Python anywhere? That would result in Python byte compiling mach and creating a machc file.

Blair: I'd really appreciate you nailing down what command is creating the machc file. Do you get it after `mach help`? What about `mach xpcshell-test`? `mach configure`? `mach build`?
Attachment #8461308 - Flags: review?(mh+mozilla) → review+
(In reply to Gregory Szorc [:gps] from comment #2)
> Blair: I'd really appreciate you nailing down what command is creating the
> machc file. Do you get it after `mach help`? What about `mach
> xpcshell-test`? `mach configure`? `mach build`?

After |mach build|, after I've clobbered.

First noticed it a couple of days ago. I'm building on Windows, FWIW.
(In reply to Gregory Szorc [:gps] from comment #2)
> Are we attempting an import of "mach" from Python anywhere? That would
> result in Python byte compiling mach and creating a machc file.

Using multiprocessing in mach is what makes us import mach. See the win32 branch in mach itself.
Wouldn't a better fix to be to surround the "import" at https://hg.mozilla.org/mozilla-central/file/a91ec42d6a9c/mach#l104 with `sys.dont_write_bytecode = True`?
I guess that'd work.
Comment on attachment 8461308 [details] [diff] [review]
Patch v1

Let's do the proper approach outlined in comments.
Attachment #8461308 - Flags: review-
Attached patch Patch v2 (obsolete) — Splinter Review
Aaaaaand this doesn't work.

Any other ideas?
Attachment #8461308 - Attachment is obsolete: true
Attachment #8462443 - Flags: feedback?(mh+mozilla)
Attachment #8462443 - Flags: feedback?(gps)
Attached patch Don't write machc bytecode file (obsolete) — Splinter Review
When writing bytecode, Python will append "c" to the loaded filename to
produce a bytecode file. Since "mach" was being imported, this resulted
in the creation of a "machc" file.

The implementation of imp.load_module() in CPython's import.c checks
sys.dont_write_bytecode. So, we wrap imp.load_module to set this flag
when importing "mach."

Reviewer notes: I was unable to reproduce the creation of machc. I'd
like Blair to confirm this patch fixes things before committing. My
earlier comments about a one-line fix were wrong because I misread the
code. This hack on top of a hack is sadly necessary (unless we want to
globally disable bytecode, which isn't recommended for performance
reasons).
Attachment #8467900 - Flags: review?(mshal)
Attachment #8467900 - Flags: feedback?(bmcbride)
Assignee: bmcbride → gps
Attachment #8462443 - Attachment is obsolete: true
Attachment #8462443 - Flags: feedback?(mh+mozilla)
Attachment #8462443 - Flags: feedback?(gps)
Comment on attachment 8467900 [details] [diff] [review]
Don't write machc bytecode file

Looks reasonable, as far as hacks go :)
Attachment #8467900 - Flags: review?(mshal) → review+
Comment on attachment 8467900 [details] [diff] [review]
Don't write machc bytecode file

khuey was complaining about this too. f? to confirm the fix.
Attachment #8467900 - Flags: feedback?(khuey)
I still get a machc with the patch.
Comment on attachment 8467900 [details] [diff] [review]
Don't write machc bytecode file

(In reply to David Major [:dmajor] from comment #12)
> I still get a machc with the patch.

Ditto :(
Attachment #8467900 - Flags: feedback?(bmcbride) → feedback-
What is the minimum STR? |mach build|?
Yes. (In reply to Gregory Szorc [:gps] from comment #14)
> What is the minimum STR? |mach build|?

Yes. That's the only successful STR I've found. None of |mach help|, |mach configure|, or |mach build ThisDoesNotExist| will work. But when you |mach build| the file will appear right away, so you don't have to wait for the rest of the build.
Attached patch Don't write machc bytecode file (obsolete) — Splinter Review
multiprocess.forking hard-codes the name of the main module that is
imported. So our new special code path behind |if name == 'mach'| was
never running.

I was able to reproduce the creation of machc and this patch seems to
fix it.

FWIW, I believe that background resource monitoring from mach build is
what's triggering this code path. That was recently re-enabled, which is
why people are suddenly seeing this.

Feel free to f+ if this fixes matters for you.
Attachment #8469682 - Flags: review?(mshal)
Attached patch pSplinter Review
multiprocess.forking hard-codes the name of the main module that is
imported. So our new special code path behind |if name == 'mach'| was
never running.

I was able to reproduce the creation of machc and this patch seems to
fix it.

FWIW, I believe that background resource monitoring from mach build is
what's triggering this code path. That was recently re-enabled, which is
why people are suddenly seeing this.

Feel free to f+ if this fixes matters for you.
Attachment #8467900 - Attachment is obsolete: true
Attachment #8469682 - Attachment is obsolete: true
Attachment #8467900 - Flags: feedback?(khuey)
Attachment #8469682 - Flags: review?(mshal)
Attachment #8469683 - Flags: review?(mshal)
Comment on attachment 8469683 [details] [diff] [review]
p

Dunno what happened to this attachment, but it works for me.
Attachment #8469683 - Attachment is patch: true
Attachment #8469683 - Flags: feedback+
Attachment #8469683 - Flags: review?(mshal) → review+
https://hg.mozilla.org/mozilla-central/rev/65bcc7b2a8a6
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Flags: qe-verify-
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.