Closed
Bug 1043144
Opened 10 years ago
Closed 10 years ago
machc generated in working dir after build, not ignored by .hgignore/.gitignore
Categories
(Firefox Build System :: Mach Core, enhancement)
Firefox Build System
Mach Core
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla34
People
(Reporter: Unfocused, Assigned: gps)
Details
Attachments
(1 file, 4 obsolete files)
2.54 KB,
patch
|
mshal
:
review+
away
:
feedback+
|
Details | Diff | Splinter Review |
machc is generated in the root of the working directory during a build. I don't like it! Should make HG/Git ignore it.
Reporter | ||
Comment 1•10 years ago
|
||
Attachment #8461308 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 2•10 years ago
|
||
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`?
Updated•10 years ago
|
Attachment #8461308 -
Flags: review?(mh+mozilla) → review+
Reporter | ||
Comment 3•10 years ago
|
||
(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.
Comment 4•10 years ago
|
||
(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.
Assignee | ||
Comment 5•10 years ago
|
||
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`?
Comment 6•10 years ago
|
||
I guess that'd work.
Assignee | ||
Comment 7•10 years ago
|
||
Comment on attachment 8461308 [details] [diff] [review] Patch v1 Let's do the proper approach outlined in comments.
Attachment #8461308 -
Flags: review-
Reporter | ||
Comment 8•10 years ago
|
||
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)
Assignee | ||
Comment 9•10 years ago
|
||
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 | ||
Updated•10 years ago
|
Assignee: bmcbride → gps
Assignee | ||
Updated•10 years ago
|
Attachment #8462443 -
Attachment is obsolete: true
Attachment #8462443 -
Flags: feedback?(mh+mozilla)
Attachment #8462443 -
Flags: feedback?(gps)
Comment 10•10 years ago
|
||
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+
Assignee | ||
Comment 11•10 years ago
|
||
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)
Comment 12•10 years ago
|
||
I still get a machc with the patch.
Reporter | ||
Comment 13•10 years ago
|
||
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-
Assignee | ||
Comment 14•10 years ago
|
||
What is the minimum STR? |mach build|?
Comment 15•10 years ago
|
||
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.
Assignee | ||
Comment 16•10 years ago
|
||
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)
Assignee | ||
Comment 17•10 years ago
|
||
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 18•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8469683 -
Flags: review?(mshal) → review+
Assignee | ||
Comment 19•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/65bcc7b2a8a6
Comment 20•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/65bcc7b2a8a6
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Updated•10 years ago
|
Flags: qe-verify-
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•