Set Windows compiler options appropriately when using sccache

REOPENED
Assigned to

Status

()

Core
Build Config
REOPENED
9 months ago
18 days ago

People

(Reporter: ted, Assigned: ted)

Tracking

(Depends on: 2 bugs)

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

9 months ago
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)

Updated

8 months ago
Duplicate of this bug: 1266717
(Assignee)

Updated

8 months ago
Depends on: 1322703
(Assignee)

Comment 2

8 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=70bb527aba35
(Assignee)

Updated

8 months ago
Assignee: nobody → ted
(Assignee)

Comment 3

8 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0e6c780f174
(Assignee)

Comment 4

7 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e991e33e35f9
(Assignee)

Comment 5

7 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b434f0d46118
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

7 months ago
Attachment #8830676 - Attachment is obsolete: true
Attachment #8830676 - Flags: review?(mh+mozilla)
Comment hidden (mozreview-request)

Comment 9

7 months 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

7 months 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

6 months 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

6 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7516f1c84565
(Assignee)

Comment 13

6 months 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

6 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/72f6eace33a1897d9d3d00189894e6e85c7b97ae
bug 1318370 - fix clang-cl builds. r=bustage

Comment 15

6 months 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
Last Resolved: 6 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54

Comment 16

6 months 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
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.
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 → ---
Confirmed on try:
- before the patch, CL_INCLUDES_PREFIX is "Note: including file: " in autoconf.mk.
- after the patch, it is "" in autoconf.mk.
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

6 months ago
Ah, good catch!

Having build system behavior that varies based on whether sccache enabled makes it tricky to get right.
Oh, also, in light of bug 1341515, turns out host targets don't use COMPILE_PDB_FLAG.
(Assignee)

Updated

25 days ago
Depends on: 1384557
(Assignee)

Comment 23

18 days ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e6aa35db1a23dac567a861fe3754e0102229179
You need to log in before you can comment on or make changes to this bug.