Closed Bug 1465800 Opened 2 years ago Closed Last year

Create a mingw-clang Browser Build Job

Categories

(Firefox Build System :: General: Unsupported Platforms, enhancement, P5)

3 Branch
enhancement

Tracking

(firefox-esr60 fixed, firefox64 fixed)

RESOLVED FIXED
mozilla64
Tracking Status
firefox-esr60 --- fixed
firefox64 --- fixed

People

(Reporter: tjr, Assigned: jacek)

References

(Blocks 1 open bug)

Details

(Whiteboard: [tor])

Attachments

(1 file, 2 obsolete files)

This bug tracks creating a browser build job for mingw-clang.  While Bug 1434316 is not yet completed, it will build off of that.

Because we initially will get this working with --enable-stylo, it will be based on esr60 and then when we have stylo working we'll rebase it for -central.
As of this writing, the following patches are required on top of esr60 as they are not landed yet.  This work should be based off of them however.

Bug 1434316 - all four patches from that
Bug 1389967 - a patchfile you can 'curl ... | hg import -' is at https://hg.mozilla.org/try/raw-rev/11f3c5a704a4db1d44705b453cce97cd4769d72f
Bug 1411401 - I haven't gotten around to cleaning up this patch for inclusion in MinGW so I'm just using a local version for now
Bug 1460620
Bug 1448748 - a patchfile you can curl is https://hg.mozilla.org/try/raw-rev/def054a2d36bbffaa55157f065a54e775c5bf21e
Timeout Prevention - a patchfile you can curl is https://hg.mozilla.org/try/raw-rev/7fc18bf32d89

Once you have those, the following command should produce a successful working build: ./mach try -b do -p win64-mingw32 -u none -t none

After that take Bug 1465798 (this is the mingw skeleton toolchain job) and the skeleton patch attached this this bug and run ./mach try -b do -p win64-mingwclang -u none -t none to build the mingw-clang based browser. (Currently this will fail, you'll need to look at the patch here and edit the mozconfig as needed.)

If for some reason you need the build machine to have additional tools, the Dockerfile is here: https://searchfox.org/mozilla-central/source/taskcluster/docker/mingw32-build/Dockerfile  In general, only system tools (like patch) should be installed there, if it's a library you need that could be created in the mingw-clang toolchain job (or a separate toolchain job).  But you can edit the Dockerfile and stick something there for testing purposes.
Comment on attachment 8987512 [details]
Bug 1465800: Create a skeleton MinGW-Clang Browser Build job

https://reviewboard.mozilla.org/r/252770/#review259228

::: browser/config/mozconfigs/win64/mingwclang:40
(Diff revision 1)
> +ac_add_options --disable-accessibility # https://sourceforge.net/p/mingw-w64/bugs/648/
> +
> +ac_add_options --disable-stylo # Bug 1390583
> +
> +# For now, we'll disable the sandbox, until we get get Bug 1461421 figured out
> +ac_add_options --disable-sandbox

Now that we know (although haven't landed) the problem for this; maybe it works?

::: browser/config/mozconfigs/win64/mingwclang:61
(Diff revision 1)
> +# explicitly specify clang as a preprocessor.
> +WINDRES="x86_64-w64-mingw32-windres --preprocessor=\"$CPP -xc\" -DRC_INVOKED"
> +
> +# Disable stripping binaries and let lld strip them instead.
> +# Binutils corrupt clang-built binaries when stripping.
> +# This works around the problem until we have a better solution.

Could we file a bug (probably a child of 1463261) about what this issue is and link to any upstream bugs about it?

::: browser/config/mozconfigs/win64/mingwclang:67
(Diff revision 1)
> +OBJCOPY=/bin/true
> +STRIP=/bin/true
> +LDFLAGS="-Wl,-s"
> +
> +# Silence verbose and mostly inaccurate warnings.
> +CXXFLAGS="-Wno-incompatible-ms-struct"

This should go in warnings.configure. There's a MinGW-only section you could apply it to for clang-only.
Comment on attachment 8987512 [details]
Bug 1465800: Create a skeleton MinGW-Clang Browser Build job

https://reviewboard.mozilla.org/r/252770/#review259702

tjr has good comments as well.

::: browser/config/mozconfigs/win64/mingwclang:37
(Diff revision 1)
> +ac_add_options --disable-warnings-as-errors
> +
> +# Temporary config settings until we get these working on mingw
> +ac_add_options --disable-accessibility # https://sourceforge.net/p/mingw-w64/bugs/648/
> +
> +ac_add_options --disable-stylo # Bug 1390583

Is this option still supported?!

::: browser/config/mozconfigs/win64/mingwclang:69
(Diff revision 1)
> +# We want to make sure we use binutils and other binaries in the tooltool
> +# package.
> +mk_add_options "export PATH=$TOOLTOOL_DIR/clang/bin:$TOOLTOOL_DIR/mingw32/bin:$TOOLTOOL_DIR/wine/bin:$TOOLTOOL_DIR/upx/bin:$TOOLTOOL_DIR/fxc2/bin:$PATH"

Can we achieve this with options passed to clang (`-B` and friends) instead?  I guess the wine and fxc2 programs can't be accessed that way.

::: taskcluster/ci/build/windows.yml:786
(Diff revision 1)
> +    worker-type: aws-provisioner-v1/gecko-{level}-b-linux
> +    worker:
> +        docker-image: {in-tree: mingw32-build}
> +        max-run-time: 7200
> +        env:
> +            PERFHERDER_EXTRA_OPTIONS: "opt 64 clang"

Do we even want to record anything on perfherder for these builds until they're tier 1?

::: taskcluster/ci/build/windows.yml:819
(Diff revision 1)
> +    worker-type: aws-provisioner-v1/gecko-{level}-b-linux
> +    worker:
> +        docker-image: {in-tree: mingw32-build}
> +        max-run-time: 72000
> +        env:
> +            PERFHERDER_EXTRA_OPTIONS: "debug 64 clang"

Same question here.
Attachment #8987512 - Flags: review?(nfroyd) → review+
Comment on attachment 8987512 [details]
Bug 1465800: Create a skeleton MinGW-Clang Browser Build job

https://reviewboard.mozilla.org/r/252770/#review259704

::: commit-message-e1e16:1
(Diff revision 1)
> +Bug 1465800: Create a sekeleton MinGW-Clang Browser Build job r?froydnj

Nit: "skeleton"
(In reply to Nathan Froyd [:froydnj] from comment #5)
> ::: browser/config/mozconfigs/win64/mingwclang:37
> (Diff revision 1)
> > +ac_add_options --disable-warnings-as-errors
> > +
> > +# Temporary config settings until we get these working on mingw
> > +ac_add_options --disable-accessibility # https://sourceforge.net/p/mingw-w64/bugs/648/
> > +
> > +ac_add_options --disable-stylo # Bug 1390583
> 
> Is this option still supported?!

No. Until we get the build working with stylo, this build will be esr60 only.

> ::: taskcluster/ci/build/windows.yml:786
> (Diff revision 1)
> > +    worker-type: aws-provisioner-v1/gecko-{level}-b-linux
> > +    worker:
> > +        docker-image: {in-tree: mingw32-build}
> > +        max-run-time: 7200
> > +        env:
> > +            PERFHERDER_EXTRA_OPTIONS: "opt 64 clang"
> 
> Do we even want to record anything on perfherder for these builds until
> they're tier 1?

Well, until we enable tests for (any) MinGW build - nothings going to go to perfherder anyway. This variable is needed to make sure that when stuff does go to perfherder, that the results don't get mixed with anything else.

As for whether or not we should allow one to run Talos tests for (any) MinGW build - I suppose that's a question for the patch that enables tests :)
(In reply to Tom Ritter [:tjr] from comment #7)
> (In reply to Nathan Froyd [:froydnj] from comment #5)
> > ::: taskcluster/ci/build/windows.yml:786
> > (Diff revision 1)
> > > +    worker-type: aws-provisioner-v1/gecko-{level}-b-linux
> > > +    worker:
> > > +        docker-image: {in-tree: mingw32-build}
> > > +        max-run-time: 7200
> > > +        env:
> > > +            PERFHERDER_EXTRA_OPTIONS: "opt 64 clang"
> > 
> > Do we even want to record anything on perfherder for these builds until
> > they're tier 1?
> 
> Well, until we enable tests for (any) MinGW build - nothings going to go to
> perfherder anyway. This variable is needed to make sure that when stuff does
> go to perfherder, that the results don't get mixed with anything else.

Perfherder gets numbers about build times and things like that, no?  Otherwise I'm unsure of how specifying PERFHERDER_EXTRA_OPTIONS on a build job would magically carry over to the test jobs from said build job.

Regardless, if things work, things work!  I'm not super-picky about this.
(In reply to Tom Ritter [:tjr] from comment #4)
> Comment on attachment 8987512 [details]
> Bug 1465800: Create a sekeleton MinGW-Clang Browser Build job
> 
> https://reviewboard.mozilla.org/r/252770/#review259228
> 
> ::: browser/config/mozconfigs/win64/mingwclang:40
> (Diff revision 1)
> > +ac_add_options --disable-accessibility # https://sourceforge.net/p/mingw-w64/bugs/648/
> > +
> > +ac_add_options --disable-stylo # Bug 1390583
> > +
> > +# For now, we'll disable the sandbox, until we get get Bug 1461421 figured out
> > +ac_add_options --disable-sandbox
> 
> Now that we know (although haven't landed) the problem for this; maybe it
> works?

I tried and sadly it didn't work for me.

> ::: browser/config/mozconfigs/win64/mingwclang:61
> (Diff revision 1)
> > +# explicitly specify clang as a preprocessor.
> > +WINDRES="x86_64-w64-mingw32-windres --preprocessor=\"$CPP -xc\" -DRC_INVOKED"
> > +
> > +# Disable stripping binaries and let lld strip them instead.
> > +# Binutils corrupt clang-built binaries when stripping.
> > +# This works around the problem until we have a better solution.
> 
> Could we file a bug (probably a child of 1463261) about what this issue is
> and link to any upstream bugs about it?

I still need to investigate that further. This config also needs a gecko change:
https://hg.mozilla.org/try/rev/75dd9075c56e2d565efa2acac922b2a2fe6cd75f
I will create a bug about that and share more thoughts there.

> ::: browser/config/mozconfigs/win64/mingwclang:67
> (Diff revision 1)
> > +OBJCOPY=/bin/true
> > +STRIP=/bin/true
> > +LDFLAGS="-Wl,-s"
> > +
> > +# Silence verbose and mostly inaccurate warnings.
> > +CXXFLAGS="-Wno-incompatible-ms-struct"
> 
> This should go in warnings.configure. There's a MinGW-only section you could
> apply it to for clang-only.

Good point, I will it there.
Comment on attachment 8987512 [details]
Bug 1465800: Create a skeleton MinGW-Clang Browser Build 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.
Attachment #8987512 - Flags: approval-mozilla-esr60?
The updated version uses -S (aka. --strip-debug) for stripping.
Comment on attachment 8987512 [details]
Bug 1465800: Create a skeleton MinGW-Clang Browser Build job

I'm obsoleting this as I need to submit a new version that's been rebased for -central.  Basically the same thing though.
Attachment #8987512 - Attachment is obsolete: true
Attachment #8987512 - Flags: approval-mozilla-esr60?
Comment on attachment 8982341 [details]
Bug 1465800 Create an x64 MinGW-Clang Browser Build job

https://reviewboard.mozilla.org/r/248294/#review268862
Attachment #8982341 - Flags: review?(nfroyd) → review+
Comment on attachment 9004911 [details]
Bug 1465800 Create an x64 MinGW-Clang Browser Build job r=froydnj

Nathan Froyd [:froydnj] has approved the revision.
Attachment #9004911 - Flags: review+
Pushed by shindli@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2cc2e2ee5bb6
Create an x64 MinGW-Clang Browser Build job r=froydnj
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2cc2e2ee5bb6
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Version: Version 3 → 3 Branch
Priority: -- → P5
You need to log in before you can comment on or make changes to this bug.