This is breaking my WSL build, so might as well move it to moz.configure.

I diffed config.status on a local Windows build before and after this patch, and the changes are trivial: $ diff -u ../debug-mozilla-central/config.status /tmp/config.status --- ../debug-mozilla-central/config.status 2017-09-06 08:58:14 -0400 +++ /tmp/config.status 2017-09-06 08:56:28 -0400 @@ -100,7 +100,7 @@ 'ARM_ARCH': '', 'AR_EXTRACT': '', 'AR_FLAGS': '-NOLOGO -OUT:$@', - 'AS': 'ml64.exe', + 'AS': 'C:/PROGRA~2/MIB055~1/2017/COMMUN~1/VC/Tools/MSVC/1410~1.250/bin/HostX64/x64/ml64.exe', 'ASFLAGS': '', 'AS_DASH_C_FLAG': '-c', 'AUTOCONF': 'C:/mozilla-build/msys/local/bin/autoconf-2.13', @@ -224,7 +224,7 @@ 'HOST_CPU_ARCH': 'x86_64', 'HOST_CXX': 'cl.exe', 'HOST_CXXFLAGS': '', - 'HOST_LDFLAGS': ' -MACHINE:X64', + 'HOST_LDFLAGS': ' -MACHINE:X64', 'HOST_MAJOR_VERSION': '32', 'HOST_OPTIMIZE_FLAGS': '-O2', 'HOST_OS_ARCH': 'WINNT', @@ -274,7 +274,7 @@ 'MAKENSISU': 'c:/mozilla-build/nsis-3.0b1/makensis-3.0b1.exe', 'MAKENSISU_FLAGS': '', 'MAR_CHANNEL_ID': 'firefox-mozilla-central', - 'MIDL': 'midl', + 'MIDL': 'C:/PROGRA~2/WI3CF2~1/10/bin/100150~1.0/x64/midl.exe', 'MIDL_FLAGS': ' -env x64', 'MKCSHLIB': '$(LINK) -NOLOGO -DLL -OUT:$@ -PDB:$(LINK_PDBFILE) $(DSO_LDOPTS)', 'MKSHLIB': '$(LINK) -NOLOGO -DLL -OUT:$@ -PDB:$(LINK_PDBFILE) $(DSO_LDOPTS)', Full paths for AS/MIDL, whitespace differences in HOST_LDFLAGS.

Comment on attachment 8905020 [details] bug 1397263 - Move AS and MIDL checks to toolchain.configure. https://reviewboard.mozilla.org/r/176854/#review181776 ::: build/autoconf/compiler-opts.m4 (Diff revision 1) > -dnl ============================================================== > -if test -z "$CROSS_COMPILE"; then > -case "$target" in > -*-mingw*) > - if test -z "$CPP"; then CPP="$CC -E -nologo"; fi > - if test -z "$CXXCPP"; then CXXCPP="$CXX -TP -E -nologo"; ac_cv_prog_CXXCPP="$CXXCPP"; fi These are functionally ignored nowadays. (I note that we lost the `-nologo` along the way, but it doesn't seem to break anything.) ::: build/autoconf/compiler-opts.m4 (Diff revision 1) > - ;; > - esac > - fi > - if test -z "$MIDL"; then MIDL=midl; fi > - > - # need override this flag since we don't use $(LDFLAGS) for this. This doesn't seem to actually do anything useful, so I just removed it. ::: js/src/old-configure.in (Diff revision 1) > -if test "$JS_HAS_CTYPES"; then > - dnl JS_HAS_CTYPES is defined by Python configure. This check remains > - dnl as long as determining $AS remains in old-configure. > - dnl Error out if we're on MSVC and MASM is unavailable. > - if test -n "$_MSC_VER" -a \( "$AS" != "ml.exe" -a "$AS" != "ml64.exe" \); then > - AC_MSG_ERROR([\"$AS\" is not a suitable assembler to build js-ctypes. If you are building with MS Visual Studio 8 Express, you may download the MASM 8.0 package, upgrade to Visual Studio 9 Express, or install the Vista SDK. Or do not use --enable-ctypes.]) This didn't seem like a very useful test so I removed it. If you think it adds value, I'd suggest we instead simply add a test in toolchain.configure that the assembler is MASM when building with MSVC.

Comment on attachment 8905439 [details] bug 1397263 - move MIDL_FLAGS to toolchain.configure. https://reviewboard.mozilla.org/r/177254/#review182252 I wound up needing to fiddle with `MIDL_FLAGS` as well, so I moved it also. ::: old-configure.in (Diff revision 1) > INCREMENTAL_LINKER=1 > > - # Set midl environment > - case "$target" in > - i*86-*) > - MIDL_FLAGS="${MIDL_FLAGS} -env win32" I dropped the feature of accepting `$MIDL_FLAGS` from the environment, but I would be shocked to find that anyone was actually using that.

Comment on attachment 8905020 [details] bug 1397263 - Move AS and MIDL checks to toolchain.configure. https://reviewboard.mozilla.org/r/176854/#review183616 ::: build/moz.configure/toolchain.configure:1256 (Diff revision 1) > +def with_toolchain_prefix(name, toolchain_prefix): > + return tuple('%s%s' % (p, name) for p in toolchain_prefix) why not use this in default_c_compilers too? ::: build/moz.configure/toolchain.configure:1265 (Diff revision 1) > +def as_names(target, c_compiler, cross_compiling, toolchain_prefix): > + if c_compiler.type in ('msvc', 'clang-cl'): > + ml = { > + 'x86': 'ml', > + 'x86_64': 'ml64', > + }.get(target.cpu) Considering check_prog is probably not going to handle this function returning (None,) quite well, I'd rather have a KeyError here in the hypothetical case where target.cpu doesn't match anything in the ml dict. So [target.cpu] instead of get(). ::: build/moz.configure/toolchain.configure:1273 (Diff revision 1) > + return with_toolchain_prefix('as', toolchain_prefix) > + return ('as', c_compiler.compiler) > + > +assembler = check_prog('AS', as_names) > + > +# This only exists so directories using yasm can unset it. Makes me wonder... how many directories are *not* using yasm at this point? ::: build/moz.configure/toolchain.configure:1278 (Diff revision 1) > +# This only exists so directories using yasm can unset it. > +set_config('AS_DASH_C_FLAG', '-c') > + > +@depends(c_compiler, toolchain_prefix) > +def midl_names(c_compiler, toolchain_prefix): > + if c_compiler.type in ['gcc', 'clang']: ('gcc', 'clang') ::: build/moz.configure/toolchain.configure:1287 (Diff revision 1) > + widl = with_toolchain_prefix('widl', toolchain_prefix) + widl > + return widl > + else: > + return ('midl',) > + > +midl = check_prog('MIDL', midl_names, when=depends(target)(lambda t: t.kernel == 'WINNT')) when=target_is_windows

Comment on attachment 8905439 [details] bug 1397263 - move MIDL_FLAGS to toolchain.configure. https://reviewboard.mozilla.org/r/177254/#review183618 ::: build/moz.configure/toolchain.configure:1292 (Diff revision 1) > midl = check_prog('MIDL', midl_names, when=depends(target)(lambda t: t.kernel == 'WINNT')) > > -# Needed until we move MIDL_FLAGS and --disable-accessibility from old-configure > +# Needed until we move --disable-accessibility from old-configure > add_old_configure_assignment('MIDL', midl) > + > +@depends(c_compiler, target, when=depends(midl, target)(lambda m, t: m and t.kernel == 'WINNT')) when=midl should be enough, since midl is never going to resolving to a value on non-WINNT builds.

Will this fix bug 1329587? If so, please set up dependency or dupe as appropriate.

Unfortunately not.

I found myself wanting to touch the midl configure code as part of bug 1401627. Are these patches going to land soon?

These patches ought to be landable, but I haven't run them by try yet. I think the copies in mozreview might be on top of my other WSL patches, so they would need to be rebased to central or something like that first.

This broke everything-but-Windows on try, because I missed a critical bit: -AS='$(CC)' We override the value of AS to be the C compiler on every other platform.

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #13) > This broke everything-but-Windows on try, because I missed a critical bit: > -AS='$(CC)' > > We override the value of AS to be the C compiler on every other platform. I tried fixing this locally but I ran into a related issue. MOZ_TOOL_VARIABLES gets expanded very early in old-configure: https://dxr.mozilla.org/mozilla-central/rev/574f4f58fe09dd590ea892406e237318c31705b4/old-configure.in#146 and that runs $AS to check for and set GNU_AS: https://dxr.mozilla.org/mozilla-central/rev/574f4f58fe09dd590ea892406e237318c31705b4/build/autoconf/toolchain.m4#26 but then *later* we reset AS='$(CC)': https://dxr.mozilla.org/mozilla-central/rev/574f4f58fe09dd590ea892406e237318c31705b4/old-configure.in#458 ...so the test for GNU_AS relies on using a value of AS that we don't actually use in the build anywhere.

I fiddled enough things that I think this needs another review. I added an additional patch to move the GNU_AS check to toolchain.configure, since it was broken by moving the AS check. The diff of config.status on my Linux build before and after these changes looks like: https://gist.github.com/c367d776479e05bdc36a6daf23c215c1

Comment on attachment 8934178 [details] bug 1397263 - move GNU_AS checks to toolchain.configure. https://reviewboard.mozilla.org/r/205122/#review211758 ::: build/moz.configure/toolchain.configure:1540 (Diff revision 1) > +def gnu_as(assembler, c_compiler, toolchain_flags): > + if c_compiler.type in ('gcc', 'clang'): > + cmd = [assembler] + c_compiler.flags > + if toolchain_flags: > + cmd += toolchain_flags > + cmd += ['-Wa,--version', '-c', '-o', devnull, '-x', 'assembler', '-'] Thank you for actually fixing this test at the same time :)

Tried updating these patches today: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e858246eb11f86941d6c11d19d9f9b7e144c2c23 Problems: - OS X cross builds fall over, because we're not using $(CC) (and associated CFLAGS), we're using the actual compiler, and therefore we don't pick up things like --target. I think this is easily solvable by setting ASFLAGS appropriately in the mozconfig, but it's kind of annoying. Maybe we should fix this in the emitter? - artifact packaging builds fall over because they can't find AS in the configure environment. I'm not really sure how this ever worked, because these builds look for CC/CXX, and those shouldn't be defined for artifact builds either...? toolchain.configure has notes about check_prog'ing _CC/_CXX because old-configure defines CC/CXX (?), so maybe we should apply the same treatment to AS. - Linux32 builds fall over for the same reason as OS X cross builds. - Android builds fall over...for the same reason as OS X cross builds. Not optimistic about the Win32 builds at this point.

...and Windows breaks because of the ASOUTOPTION changes, as USE_YASM flips AS/AS_DASH_C_FLAG, so we really do need some sort of dynamic changes for ASOUTOPTION there.

This patch has seen enough changes (in particular, getting flags for the assembler correct) that it seems worth doing another review.

Carrying over r+ from glandium, since the change here were trivial.

The only significant change here was to unconditionally say that clang is GNU_AS, as the previous test for clang being GNU_AS was bogus. I think that's small enough to carry over r+. It's debatable whether GNU_AS should just be `c_compiler.type in ('clang', gcc')`.

While we're here...

Argh, still haven't fixed the artifact build packaging issues, and the Tup build breaks, presumably because AS is a list now instead of a string.

(In reply to Nathan Froyd [:froydnj] from comment #27) > Argh, still haven't fixed the artifact build packaging issues, and the Tup > build breaks, presumably because AS is a list now instead of a string. Oh, hah, tup had code to handle AS = $(CC). It doesn't need that now, since we perform the substitution in moz.configure. Should we just remove that code in backend/tup.py: https://searchfox.org/mozilla-central/source/python/mozbuild/mozbuild/backend/tup.py#187-190 or are there other interesting compiler-related variables that can have embedded variables?

Oh, and Windows L10n builds also break, because apparently now they don't check for MIDL, and such builds --enable-accessibility.

(In reply to Nathan Froyd [:froydnj] from comment #28) > (In reply to Nathan Froyd [:froydnj] from comment #27) > > Argh, still haven't fixed the artifact build packaging issues, and the Tup > > build breaks, presumably because AS is a list now instead of a string. > > Oh, hah, tup had code to handle AS = $(CC). It doesn't need that now, since > we perform the substitution in moz.configure. Should we just remove that > code in backend/tup.py: > > https://searchfox.org/mozilla-central/source/python/mozbuild/mozbuild/ > backend/tup.py#187-190 > > or are there other interesting compiler-related variables that can have > embedded variables? It looks like this is just for AS=$(CC), so we can remove the expand_variables() call. When I remove that and add your patches, I see a bunch of flags duplicated on the command-line though - I'm still looking into that.

(In reply to Michael Shal [:mshal] from comment #30) > It looks like this is just for AS=$(CC), so we can remove the > expand_variables() call. When I remove that and add your patches, I see a > bunch of flags duplicated on the command-line though - I'm still looking > into that. Oh nevermind, just a bug in my patch where I was accidentally overwriting self.environment.substs['AS'] on every rule :). We should be good to remove expand_variables here.

So after moving --enable-accessibility to moz.configure: https://hg.mozilla.org/try/rev/6bd9820b8f95168b8e3b7b9088fcd4d557a7edf0 fixing the tup build: https://hg.mozilla.org/try/rev/37df246c6103ab51b18e5693081b025121c5e242 and fixing artifact build packaging (that wants CC/CXX/AS, whyyyyy): https://hg.mozilla.org/try/rev/da07eb51965b8c6eb54d29fa2cd64e5da70dbc4d Builds at least look kind of green. Need to squash a few things, but should have fresh patches up tomorrow.

Comment on attachment 9012975 [details] [diff] [review] move AS and MIDL checks to toolchain.configure Review of attachment 9012975 [details] [diff] [review]: ----------------------------------------------------------------- ::: build/autoconf/compiler-opts.m4 @@ -29,5 @@ > - if test -z "$MIDL"; then MIDL=midl; fi > - > - # need override this flag since we don't use $(LDFLAGS) for this. > - if test -z "$HOST_LDFLAGS" ; then > - HOST_LDFLAGS=" " Hopefully the lack of this doesn't cause problems? ::: build/autoconf/ios.m4 @@ -96,5 @@ > MOZ_IOS_PATH_PROG(CC, clang, $ARGS) > MOZ_IOS_PATH_PROG(CXX, clang++, $ARGS) > export CPP="$CC -E" > MOZ_IOS_PATH_PROG(AR) > - MOZ_IOS_PATH_PROG(AS, as, $ARGS) It feels like this should stay here. Which means the AC_SUBST would need to stay in old-configure.in.

(In reply to Mike Hommey [:glandium] from comment #33) > ::: build/autoconf/ios.m4 > > MOZ_IOS_PATH_PROG(AR) > > - MOZ_IOS_PATH_PROG(AS, as, $ARGS) > > It feels like this should stay here. Which means the AC_SUBST would need to > stay in old-configure.in. We don't have an actual working iOS build to speak of so I think just removing this in service of this cleanup is reasonable.

Separating these out so they're easier to follow. The allow_missing=True bit will make sense at a later point.

This version differs in not removing {AC,MOZ}_PATH_PROG(AS, ...) and related code, for the same reason that toolchain.configure only adds an old-configure assignment for CC/CXX, and doesn't set_config those variables: we have code and builds that depend on AS/CC/CXX showing up even if we're not compiling anything. This is arguably a bug, but that is the situation right now.

Still carrying over r+ here.

Still carrying over r+ here.

And finally, we might as well move this to make the MIDL issues easier to understand, especially since we removed the default setting of MIDL=midl way back in part 1...

Comment on attachment 9013769 [details] [diff] [review] move AS checks to toolchain.configure >Bug 1397263 - move AS checks to toolchain.configure; r=glandium FYI this patch might need to touch CLOBBER. If I run configure before this patch, AS='$(CC)', and then if I apply this patch and re-run configure without clobbering, AS='/usr/bin/as', which doesn't work. But if I re-configure after clobbering or removing config.cache, AS='/home/mshal/.mozbuild/clang/bin/clang -std=gnu99', which is the correct value.

Comment on attachment 9013769 [details] [diff] [review] move AS checks to toolchain.configure Review of attachment 9013769 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/old-configure.in @@ -1688,5 @@ > > AC_SUBST(AR) > AC_SUBST(AR_FLAGS) > AC_SUBST(AR_EXTRACT) > -AC_SUBST(AS) Does this really work with neither this nor a set_config('AS'...)?

Comment on attachment 9013756 [details] [diff] [review] move MIDL checks to moz.configure Review of attachment 9013756 [details] [diff] [review]: ----------------------------------------------------------------- It's a little weird that there's a midl part in the as patch.

Comment on attachment 9013769 [details] [diff] [review] move AS checks to toolchain.configure (In reply to Mike Hommey [:glandium] from comment #42) > ::: js/src/old-configure.in > @@ -1688,5 @@ > > > > AC_SUBST(AR) > > AC_SUBST(AR_FLAGS) > > AC_SUBST(AR_EXTRACT) > > -AC_SUBST(AS) > > Does this really work with neither this nor a set_config('AS'...)? This really does work, yes. AC_PATH_PROG and related will AC_SUBST(AS). (In reply to Mike Hommey [:glandium] from comment #43) > Comment on attachment 9013756 [details] [diff] [review] > move MIDL checks to moz.configure > > Review of attachment 9013756 [details] [diff] [review]: > ----------------------------------------------------------------- > > It's a little weird that there's a midl part in the as patch. Good point, will move that.

Comment on attachment 9013769 [details] [diff] [review] move AS checks to toolchain.configure Review of attachment 9013769 [details] [diff] [review]: ----------------------------------------------------------------- Oh right, you're not removing the AC_PATH_PROG.

Pushed by nfroyd@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/73a4e7ed19f3 move MIDL checks to moz.configure; r=glandium https://hg.mozilla.org/integration/mozilla-inbound/rev/b4dec9b774a8 move AS checks to toolchain.configure; r=glandium https://hg.mozilla.org/integration/mozilla-inbound/rev/b3ad04383928 move MIDL_FLAGS to toolkit/moz.configure; r=glandium https://hg.mozilla.org/integration/mozilla-inbound/rev/23f384237f73 move GNU_AS checks to toolchain.configure; r=glandium https://hg.mozilla.org/integration/mozilla-inbound/rev/237dbec378d8 move ASOUTOPTION to moz.configure; r=mshal https://hg.mozilla.org/integration/mozilla-inbound/rev/d678adeaddcd move --enable-accessibility to moz.configure; r=mshal

Pushed by nfroyd@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/4b136b18becf touch CLOBBER as suggested by mshal; r=me

https://hg.mozilla.org/mozilla-central/rev/73a4e7ed19f3 https://hg.mozilla.org/mozilla-central/rev/b4dec9b774a8 https://hg.mozilla.org/mozilla-central/rev/b3ad04383928 https://hg.mozilla.org/mozilla-central/rev/23f384237f73 https://hg.mozilla.org/mozilla-central/rev/237dbec378d8 https://hg.mozilla.org/mozilla-central/rev/d678adeaddcd

Pushed by archaeopteryx@coole-files.de: https://hg.mozilla.org/mozilla-central/rev/be74e0c5c164 touch CLOBBER as suggested by mshal; r=me a=CLOBBER