This bug tracks creating a toolchain job for mingw-clang.

The attached patch will create a skeleton mingw toolchain job. It can be run in TC with ./mach try fuzzy --full -q linux64-clang-6-mingw I'll add instructions to this bug in a bit explaining how to run it locally in a Docker container that mimics TC.

Note also: this patch does not change nsis or fxc2 to build using the mingw-clang toolchain. They will be built using the mingw-gcc toolchain.

While I was working on the Docker walkthrough I discovered that our toolchains are built on a debian-7 image but the MinGW build job is built on a debian-9 image. I'm going to give instructions for debian 7 and hope that nothing gets screwy when running this in TC because of 7/9 difference. This will only affect local development in a Docker container - work run on TaskCluster will use debian-9.

Here is a try push containing submitted patch (on top of esr60 and together with other required changes): https://treeherder.mozilla.org/#/jobs?repo=try&revision=084bb5c2f0e6441a0649f428f5353f2ed0bd6e2b To build the toolchain, we need a regular clang build (clang is always a crosscompiler, so it's exactly the same as a usual Linux build) and mingw target-specific sysroot. My patch uses build-clang.py to build the compiler with working native libraries (libc++) and then does the rest in bash. I considered adding a special mode to build-clang.py so that it could be invoked separately for sysroot, but it doesn't seem clean. mingw setup is has quite a few tricks (like merging libraries, requiring libunwind and unusual cmake arguments) that would better be separated, I think. I also put the whole logic in one file, but made it straightforward to separate if we'd like to share it in other scripts (like for i686 target or different clang version). I will move it now if preferred. I used clang 7 branch as it contains some fixes that we need and are not present in earlier version. The toolchain is based on llvm-mingw: https://github.com/mstorsjo/llvm-mingw While I was at it, I gave ucrt-based builds a try and it worked rather well. With a few patches to mingw-w64 (that are upstreamed and used by the attached build script), it works now.

Comment on attachment 8987494 [details] Bug 1465798: Create a skeleton MinGW-Clang toolchain job https://reviewboard.mozilla.org/r/252746/#review259214 ::: taskcluster/scripts/misc/build-clang-7-pre-mingw.sh:43 (Diff revision 1) > + git clone https://github.com/llvm-mirror/libunwind.git > + pushd libunwind > + git checkout $libunwind_version > + popd > + > + wget -c --progress=dot:mega ftp://ftp.gnu.org/gnu/binutils/binutils-$binutils_version.tar.bz2 In the past we've needed to make the extension a variable as well; but if binutils is consistent for .tar.bz2 then we could omit it. ::: taskcluster/scripts/misc/build-clang-7-pre-mingw.sh:43 (Diff revision 1) > + git clone https://github.com/llvm-mirror/libunwind.git > + pushd libunwind > + git checkout $libunwind_version > + popd > + > + wget -c --progress=dot:mega ftp://ftp.gnu.org/gnu/binutils/binutils-$binutils_version.tar.bz2 In the past we've needed to make the extension a variable as well; but if binutils is consistent for .tar.bz2 then we could omit it. ::: taskcluster/scripts/misc/build-clang-7-pre-mingw.sh:109 (Diff revision 1) > + mkdir -p $INSTALL_DIR/lib/clang/$CLANG_VERSION/lib/windows > + cp lib/windows/libclang_rt.builtins-x86_64.a $INSTALL_DIR/lib/clang/$CLANG_VERSION/lib/windows/ > + popd > +} > + > +merge_libs() { Could you add a comment explaining what this is doing and why it's needed? ::: taskcluster/scripts/misc/build-clang-7-pre-mingw.sh:224 (Diff revision 1) > + make $make_flags > + cp binutils/windres $INSTALL_DIR/bin/x86_64-w64-mingw32-windres > + popd > +} > + > +install_wrappers() { Could we put the functions in the order they are called below in bash? ::: taskcluster/scripts/misc/build-clang-7-pre-mingw.sh:246 (Diff revision 1) > + popd > +} > + > +export PATH=$INSTALL_DIR/bin:$PATH > + > +CC="x86_64-w64-mingw32-clang" These don't exist until install_wrappers is run, right? Could we define them later (after install_wrappers) to be more explicit about that? ::: taskcluster/scripts/misc/build-clang-7-pre-mingw.sh:275 (Diff revision 1) > +# Put a tarball in the artifacts dir > +mkdir -p $UPLOAD_DIR > + > +pushd $(dirname $INSTALL_DIR) > +rm -f clang/lib/libstdc++* > +tar caf clangmingw.tar.xz clang I believe that build-clang also tars up clang, so I believe we should pass --skip-tar to build-clang to have it skip that step.

Comment on attachment 8987494 [details] Bug 1465798: Create a skeleton MinGW-Clang toolchain job Exciting!!!

Thanks for the review. Here is try push with a new version: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cb6d875b7ae7c8b87bbc2b478db160c4aa487e52

Comment on attachment 8987494 [details] Bug 1465798: Create a skeleton MinGW-Clang toolchain job https://reviewboard.mozilla.org/r/252746/#review259698 This all seems pretty reasonable. ::: taskcluster/scripts/misc/build-clang-7-pre-mingw.sh:20 (Diff revision 2) > +CLANG_VERSION=7.0.0 > +make_flags="-j$(nproc)" > + > +mingw_version=16151c441e89081fd398270bb888511ebef6fb35 > +libunwind_version=86ab23972978242b6f9e27cebc239f3e8428b1af > +llvm_mingw_version=951796aeb704dc1984db330f3df549e52641b205 This variable doesn't seem to be used? ::: taskcluster/scripts/misc/build-clang-7-pre-mingw.sh:34 (Diff revision 2) > + git clone -n git://git.code.sf.net/p/mingw-w64/mingw-w64 > + pushd mingw-w64 > + git checkout $mingw_version > + popd > + > + git clone https://github.com/llvm-mirror/libunwind.git > + pushd libunwind > + git checkout $libunwind_version > + popd > + > + wget -c --progress=dot:mega ftp://ftp.gnu.org/gnu/binutils/binutils-$binutils_version.tar.$binutils_ext > + if [ "$(sha256sum binutils-$binutils_version.tar.$binutils_ext)" != "$binutils_sha binutils-$binutils_version.tar.$binutils_ext" ]; > + then > + echo Corrupted binutils archive > + exit 1 > + fi > + tar -jxf binutils-$binutils_version.tar.$binutils_ext gps recently added functionality to fetch source packages as taskcluster artifacts. I don't know that we necessarily have to make this build use such artifacts now, but it would sure be nice as a followup bug to transition the build to use those. Can you file a followup bug for doing that? Bonus points for going ahead and converting the bits in here to use the binutils artifact that the GCC builds already use. ::: taskcluster/scripts/misc/build-clang-7-pre-mingw.sh:86 (Diff revision 2) > + $SRC_DIR/mingw-w64/mingw-w64-headers/configure --host=x86_64-w64-mingw32 \ > + --enable-sdk=all \ > + --enable-secure-api \ > + --enable-idl \ > + --with-default-msvcrt=ucrtbase \ > + --with-default-win32-winnt=0x600 \ Should we pull this out into a separate variable and document what version of Windows this corresponds to? ::: taskcluster/scripts/misc/build-clang-7-pre-mingw.sh:124 (Diff revision 2) > + -DCMAKE_C_COMPILER=$CC \ > + -DCMAKE_SYSTEM_NAME=Windows \ > + -DCMAKE_AR=$INSTALL_DIR/bin/llvm-ar \ > + -DCMAKE_RANLIB=$INSTALL_DIR/bin/llvm-ranlib \ > + -DCMAKE_C_COMPILER_WORKS=1 \ > + -DCMAKE_C_COMPILER_TARGET=x86_64-windows-gnu \ Why is this different from `x86_64-w64-mingw32`? Just different naming conventions in compiler-rt land? ::: taskcluster/scripts/misc/build-clang-7-pre-mingw.sh:128 (Diff revision 2) > + mkdir -p $INSTALL_DIR/lib/clang/$CLANG_VERSION/lib/windows > + cp lib/windows/libclang_rt.builtins-x86_64.a $INSTALL_DIR/lib/clang/$CLANG_VERSION/lib/windows/ Why are we not using compiler-rt's `make install`? ::: taskcluster/scripts/misc/build-clang-7-pre-mingw.sh:244 (Diff revision 2) > + $SRC_DIR/binutils-$binutils_version/configure --prefix=$INSTALL_DIR \ > + --disable-multilib \ > + --target=x86_64-w64-mingw32 Our other GCC binutils scripts pass `--disable-nls --enable-plugins`; can we do that here too (perhaps just the nls part) preemptively? ::: taskcluster/scripts/misc/build-clang-7-pre-mingw.sh:279 (Diff revision 2) > + > +# Put a tarball in the artifacts dir > +mkdir -p $UPLOAD_DIR > + > +pushd $(dirname $INSTALL_DIR) > +rm -f clang/lib/libstdc++* Um. Maybe we should fix this in build-clang.py itself, rather than in the build script here? Then presumably we would skip the `--skip-tar` step above?

(In reply to Nathan Froyd [:froydnj] from comment #11) > > + tar -jxf binutils-$binutils_version.tar.$binutils_ext > > gps recently added functionality to fetch source packages as taskcluster > artifacts. I don't know that we necessarily have to make this build use > such artifacts now, but it would sure be nice as a followup bug to > transition the build to use those. Until we fix stylo (maybe this week; maybe in a while) we're going to be landing this in esr60 only. gps' patches aren't there; but yes, eventually it'd be good to use them.

(In reply to Tom Ritter [:tjr] from comment #12) > (In reply to Nathan Froyd [:froydnj] from comment #11) > > > + tar -jxf binutils-$binutils_version.tar.$binutils_ext > > > > gps recently added functionality to fetch source packages as taskcluster > > artifacts. I don't know that we necessarily have to make this build use > > such artifacts now, but it would sure be nice as a followup bug to > > transition the build to use those. > > Until we fix stylo (maybe this week; maybe in a while) we're going to be > landing this in esr60 only. > > gps' patches aren't there; but yes, eventually it'd be good to use them. I somehow missed this part, sorry! Carry on, then!

(In reply to Nathan Froyd [:froydnj] from comment #11) > > +mingw_version=16151c441e89081fd398270bb888511ebef6fb35 > > +libunwind_version=86ab23972978242b6f9e27cebc239f3e8428b1af > > +llvm_mingw_version=951796aeb704dc1984db330f3df549e52641b205 > > This variable doesn't seem to be used? Yes. I considered using it at some point, but llvm-mingw repo itself is not really needed. > ::: taskcluster/scripts/misc/build-clang-7-pre-mingw.sh:86 > (Diff revision 2) > > + $SRC_DIR/mingw-w64/mingw-w64-headers/configure --host=x86_64-w64-mingw32 \ > > + --enable-sdk=all \ > > + --enable-secure-api \ > > + --enable-idl \ > > + --with-default-msvcrt=ucrtbase \ > > + --with-default-win32-winnt=0x600 \ > > Should we pull this out into a separate variable and document what version > of Windows this corresponds to? We could, but it's not really important. It's a default value of _WIN32_WINNT. We explicitly define it for Gecko builds in old-configure.in anyway. I just used same value as llvm-mingw scripts do since it's well tested for building toolchain itself. I will move it to a separate variable. > > ::: taskcluster/scripts/misc/build-clang-7-pre-mingw.sh:124 > (Diff revision 2) > > + -DCMAKE_C_COMPILER=$CC \ > > + -DCMAKE_SYSTEM_NAME=Windows \ > > + -DCMAKE_AR=$INSTALL_DIR/bin/llvm-ar \ > > + -DCMAKE_RANLIB=$INSTALL_DIR/bin/llvm-ranlib \ > > + -DCMAKE_C_COMPILER_WORKS=1 \ > > + -DCMAKE_C_COMPILER_TARGET=x86_64-windows-gnu \ > > Why is this different from `x86_64-w64-mingw32`? Just different naming > conventions in compiler-rt land? x86_64-w64-mingw32 is a tripple that frontend understands. clang internally represents that as x86_64-w64-windows-gnu. You can see that by running clang with verbose option: $ x86_64-w64-mingw32-clang++ test.cpp -v clang version 7.0.0 (trunk) (llvm/trunk 324058) Target: x86_64-w64-windows-gnu Thread model: posix InstalledDir: /tmp/clang/bin "/tmp/clang/bin/clang-7.0" -cc1 -triple x86_64-w64-windows-gnu ... > ::: taskcluster/scripts/misc/build-clang-7-pre-mingw.sh:128 > (Diff revision 2) > > + mkdir -p $INSTALL_DIR/lib/clang/$CLANG_VERSION/lib/windows > > + cp lib/windows/libclang_rt.builtins-x86_64.a $INSTALL_DIR/lib/clang/$CLANG_VERSION/lib/windows/ > > Why are we not using compiler-rt's `make install`? Note that we don't compile whole compiler-rt from its top level directory but just lib/builtins subdirectory. Such config does not have install make target. > > ::: taskcluster/scripts/misc/build-clang-7-pre-mingw.sh:244 > (Diff revision 2) > > + $SRC_DIR/binutils-$binutils_version/configure --prefix=$INSTALL_DIR \ > > + --disable-multilib \ > > + --target=x86_64-w64-mingw32 > > Our other GCC binutils scripts pass `--disable-nls --enable-plugins`; can we > do that here too (perhaps just the nls part) preemptively? Sure, I will add --disable-nls. We use only windres and I think that --enable-plugins has no effect on that, so I'd skip that. > ::: taskcluster/scripts/misc/build-clang-7-pre-mingw.sh:279 > (Diff revision 2) > > + > > +# Put a tarball in the artifacts dir > > +mkdir -p $UPLOAD_DIR > > + > > +pushd $(dirname $INSTALL_DIR) > > +rm -f clang/lib/libstdc++* > > Um. Maybe we should fix this in build-clang.py itself, rather than in the > build script here? Then presumably we would skip the `--skip-tar` step > above? I'm not sure about moving it to build-clang.py, it may be desired to leave those on other builds. It was problematic in this case because it got added to LD_LIBRARY_PATH before system libstdc++, which caused problems with some build tools. If other clang toolchains don't have such problems, it seems better to live it there. We can't use build-clang.py to create the tar, because we need to put mingw sysroot into its directory first.

Comment on attachment 8987494 [details] Bug 1465798: Create a skeleton MinGW-Clang toolchain job [Approval Request Comment] This is one of several MinGW Build patches I'd like to land in esr60 for Tor. It will prevent them from carrying their own patches for the lifetime of esr60 and will enable us to keep the MinGW build functioning and know if/when/how it was broken by new commits into esr60. This commit affects only MinGW builds, so it is low risk.

I updated a hopefully last iteration. Changes to previous version: - Update LLVM revision to include -S lld option (bug 1471698) - Update mingw-w64 revision to include a fix for stylo build (bug 1390583) - Install binutils nm so that checks for module order works (bug 1471698) - Use, recently introduced, ucrt instead of ucrtbase mingw-w64 mode, which uses API sets instead of linking directly to ucrtbase.dll (that's how MS recommends it) - Addressed previous comment

I bumped LLVM version to pick fixes for bug 1471592.

Hey Nathan, I'm going to land this on -central instead of Jacek; the patch is 99% the same; just rebased and I had to add 'touch .build-clang' to avoid a build-clang.py error. Could you re-approve?

Comment on attachment 8982275 [details] Bug 1465798: Create a MinGW-Clang toolchain job https://reviewboard.mozilla.org/r/248216/#review266328

Done!

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s c8389abc7dc530d6ee7973fdab70316b2bd2f600 -d c235d6f86c22: rebasing 474562:c8389abc7dc5 "Bug 1465798: Create a skeleton MinGW-Clang toolchain job r=froydnj" (tip) merging taskcluster/ci/toolchain/linux.yml warning: conflicts while merging taskcluster/ci/toolchain/linux.yml! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)

I have no problems merging this onto -central; so I think it's conflict is in inbound; I'll try again tomorrow.

(In reply to Tom Ritter [:tjr] from comment #25) > I have no problems merging this onto -central; so I think it's conflict is > in inbound; I'll try again tomorrow. Thank you!

I can apply this cleanly to -inbound and -central so.... I think it'll work?

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s c8389abc7dc530d6ee7973fdab70316b2bd2f600 -d 427b229b7ea4: rebasing 474880:c8389abc7dc5 "Bug 1465798: Create a skeleton MinGW-Clang toolchain job r=froydnj" (tip) merging taskcluster/ci/toolchain/linux.yml warning: conflicts while merging taskcluster/ci/toolchain/linux.yml! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)

Pushed by apavel@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/5b8d59e9b59c Create a MinGW-Clang toolchain job r=froydnj