Closed
Bug 1318370
Opened 8 years ago
Closed 6 years ago
Set Windows compiler options appropriately when using sccache
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: ted, Assigned: ted)
References
Details
Attachments
(3 obsolete files)
We currently do a bunch of fiddling in mozconfig.cache to set compiler flags and unset the compiler wrapper that we use to generate depfiles on Windows:
https://dxr.mozilla.org/mozilla-central/rev/13f49da109ea460665ad27c8497cb1489548450c/build/mozconfig.cache#121
My patch in bug 1286934 will add some minimal bits to make `--with-ccache=sccache` work. We should move the rest of this into moz.configure as well so that when we start telling developers to use sccache they can just do `--with-ccache=sccache`.
Assignee | ||
Comment 2•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → ted
Assignee | ||
Comment 3•8 years ago
|
||
Assignee | ||
Comment 4•8 years ago
|
||
Assignee | ||
Comment 5•8 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8830676 -
Attachment is obsolete: true
Attachment #8830676 -
Flags: review?(mh+mozilla)
Comment hidden (mozreview-request) |
Comment 9•8 years ago
|
||
mozreview-review |
Comment on attachment 8830677 [details]
bug 1318370 - move _DEPEND_CFLAGS+CL_INCLUDES_PREFIX to toolchain.configure, ignore {CC,CXX}_WRAPPER when using sccache
https://reviewboard.mozilla.org/r/107402/#review109794
::: build/moz.configure/toolchain.configure:974
(Diff revision 1)
> +@depends(c_compiler, using_sccache)
> +def depend_cflags(c_compiler, using_sccache):
> + if c_compiler.type != 'msvc':
> + return '-MD -MP -MF $(MDDEPDIR)/$(@F).pp'
> + elif using_sccache:
> + # sccache supports parsing -showIncludes
The comment in old-configure was actually better.
::: build/moz.configure/toolchain.configure:979
(Diff revision 1)
> + # sccache supports parsing -showIncludes
> + return '-deps$(MDDEPDIR)/$(@F).pp'
> +
> +set_config('_DEPEND_CFLAGS', depend_cflags)
> +
> +@depends_when(c_compiler, when=building_with_msvc)
You can actually do depends(foo, when=...) since bug 1296530. I got distracted and never filed the bug to remove depends_when. Will do so after this review...
::: build/moz.configure/toolchain.configure:998
(Diff revision 1)
> +# Make sure that the build system can handle non-ASCII characters
> +# in environment variables to prevent it from breaking silently on
> +# non-English systems.
> +set_config('NONASCII',
> + depends_if(c_compiler)(lambda info: u'\241\241' if info.type == 'msvc' else None))
I think we can remove this thing. IIRC, it actually didn't catch anything the last time CL_INCLUDES_PREFIX was broken on non-English locales.
Plus, you're returning a unicode string here while msvc_showincludes_prefix is likely to return a str in the windows native encoding (mbcs), so this test is, in fact, not relevant in this form.
It would be better to have some Japanese windows developer check that this doesn't break for them, and in the long term, to have a taskcluster job that installs MSVC in japanese and tries to run mach configure.
::: build/mozconfig.cache
(Diff revision 1)
> - case "$master" in
> - *us[ew][12].mozilla.com*|*euc1.mozilla.com*)
> - mk_add_options "export SCCACHE_NAMESERVER=169.254.169.253"
> - ;;
> - esac
Please move this in a separate patch that states why it is removed, which has not much to do with the commit message associated with this patch.
Attachment #8830677 -
Flags: review?(mh+mozilla) → review+
Comment 10•8 years ago
|
||
mozreview-review |
Comment on attachment 8830680 [details]
bug 1318370 - stop using -Z7 for MSVC compilation with sccache.
https://reviewboard.mozilla.org/r/107412/#review109798
Attachment #8830680 -
Flags: review?(mh+mozilla) → review+
Assignee | ||
Comment 11•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8830677 [details]
bug 1318370 - move _DEPEND_CFLAGS+CL_INCLUDES_PREFIX to toolchain.configure, ignore {CC,CXX}_WRAPPER when using sccache
https://reviewboard.mozilla.org/r/107402/#review109794
> I think we can remove this thing. IIRC, it actually didn't catch anything the last time CL_INCLUDES_PREFIX was broken on non-English locales.
>
> Plus, you're returning a unicode string here while msvc_showincludes_prefix is likely to return a str in the windows native encoding (mbcs), so this test is, in fact, not relevant in this form.
>
> It would be better to have some Japanese windows developer check that this doesn't break for them, and in the long term, to have a taskcluster job that installs MSVC in japanese and tries to run mach configure.
I filed bug 1340632 about having CI with a Japanese MSVC install.
Assignee | ||
Comment 12•8 years ago
|
||
Assignee | ||
Comment 13•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0848b3201c40dd75711aeb24d3ab7ca72a351eef
bug 1318370 - stop using -Z7 for MSVC compilation with sccache. r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8032ae9cb2ad1c4574c6ac6f5c2778863cd71e0
bug 1318370 - move _DEPEND_CFLAGS+CL_INCLUDES_PREFIX to toolchain.configure, ignore {CC,CXX}_WRAPPER when using sccache r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0c0e10ed6131dda7d98315574b3b739081055b0
bug 1318370 - remove SCCACHE_NAMESERVER from mozconfig.cache, unused since we switched to sccache2. r=glandium
Assignee | ||
Comment 14•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/72f6eace33a1897d9d3d00189894e6e85c7b97ae
bug 1318370 - fix clang-cl builds. r=bustage
Comment 15•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0848b3201c40
https://hg.mozilla.org/mozilla-central/rev/a8032ae9cb2a
https://hg.mozilla.org/mozilla-central/rev/c0c0e10ed613
https://hg.mozilla.org/mozilla-central/rev/72f6eace33a1
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment 16•8 years ago
|
||
Pushed by philringnalda@gmail.com:
https://hg.mozilla.org/mozilla-central/rev/196f9ddbf7aa
followup, touch CLOBBER to try to unbreak mozilla-central Windows static-analysis builds, a=hope
Comment 17•8 years ago
|
||
The error in those builds:
03:48:04 INFO - .deps/log.obj.pp:1: warning: overriding recipe for target 'log.obj'
03:48:04 INFO - z:/task_1487730637/build/src/config/rules.mk:877: warning: ignoring old recipe for target 'log.obj'
03:48:04 INFO - .deps/log.obj.pp:2: *** missing separator (did you mean TAB instead of 8 spaces?). Stop.
This happens during the processing of the misc rule.
Now, some interesting facts, when comparing the log of a failed build with the log of a successful build on mozilla-central before the merge:
- the command line to compile log.obj is the exact same
- nothing in the log indicates outputting a .deps/log.obj.pp file
- both builds recurse in that directory during the misc tier, so if the file was created during the successful build, it should have failed the same way
- sccache is not used on either build, because we don't set a bucket for mozilla-central builds. That is however a difference with inbound, which does use sccache, which changes how things are compiled, and things work in that case.
- whatever is creating the .deps/log.obj.pp file, it's doing it wrong, cf. the error. What does it is presumably the mozbuild.action.cl, which is not used during sccache builds, and considering it's based on CL_INCLUDES_PREFIX, and CL_INCLUDES_PREFIX is being touched in this bug, my guess is that something is going wrong with that.
Comment 18•8 years ago
|
||
Backed out in https://hg.mozilla.org/mozilla-central/rev/a180b976c165 for https://queue.taskcluster.net/v1/task/M1jKEBsrQryF8YUGmT7iGQ/runs/0/artifacts/public/logs/live_backing.log
Status: RESOLVED → REOPENED
status-firefox54:
fixed → ---
Resolution: FIXED → ---
Target Milestone: mozilla54 → ---
Comment 19•8 years ago
|
||
Confirmed on try:
- before the patch, CL_INCLUDES_PREFIX is "Note: including file: " in autoconf.mk.
- after the patch, it is "" in autoconf.mk.
Comment 20•8 years ago
|
||
Oh, that's because of:
@depends_when(c_compiler, when=building_with_msvc)
After the patch, CL_INCLUDES_PREFIX is not set when building with clang-cl, while it used to.
Assignee | ||
Comment 21•8 years ago
|
||
Ah, good catch!
Having build system behavior that varies based on whether sccache enabled makes it tricky to get right.
Comment 22•8 years ago
|
||
Oh, also, in light of bug 1341515, turns out host targets don't use COMPILE_PDB_FLAG.
Assignee | ||
Comment 23•7 years ago
|
||
Updated•7 years ago
|
Product: Core → Firefox Build System
Assignee | ||
Updated•6 years ago
|
Attachment #8830677 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #8830680 -
Attachment is obsolete: true
Assignee | ||
Comment 24•6 years ago
|
||
Between bug 1506848 and bug 1384557 let's call this fixed. Followup work in bug 1509961.
Status: REOPENED → RESOLVED
Closed: 8 years ago → 6 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•