Closed Bug 1451409 Opened Last year Closed 9 months ago

Spidermonkey builds don't respect CC/CXX/LINKER from mozconfig.vs-latest

Categories

(Firefox Build System :: General, enhancement)

3 Branch
enhancement
Not set

Tracking

(firefox65 fixed)

RESOLVED FIXED
mozilla65
Tracking Status
firefox65 --- fixed

People

(Reporter: dmajor, Assigned: dmajor)

References

Details

Attachments

(1 file)

checking for the target C++ compiler... z:/task_1522688410/vs2017_15.6.0/VC/bin/Hostx64/x64/cl.exe
checking for linker... z:/task_1522688410/vs2017_15.6.0/VC/bin/Hostx64/x64/link.exe
Maybe this is just intended behavior. I'd be fine with that resolution. But if so, then I need to know where I can override these to use clang-cl :-)
Hrm, so in the push in comment 0 I actually edited mozconfig.vs-latest (not mozconfig.common.override), which _should_ be picked up by autospider.py/winbuildenv.sh

++++ test -d 'Z:\task_1522688410\src/clang/bin'
++++ export CC=clang-cl
++++ CC=clang-cl
++++ export CXX=clang-cl
++++ CXX=clang-cl
+++ . 'Z:\task_1522688410\src/build/mozconfig.lld-link'
++++ test -d 'Z:\task_1522688410\src/clang/bin'
++++ export LINKER=lld-link
++++ LINKER=lld-link
Setting WINDOWSSDKDIR = z:/task_1522688410/vs2017_15.6.0/SDK
Setting PATH = ...
Setting INCLUDE = ...
Setting LIB = ...

It makes sense that LINKER was not picked up since it wasn't passed to set_vars_from_script, but I'm surprised CC and CXX didn't work.
Summary: Spidermonkey builds don't respect CC/CXX/LINKER from mozconfig.common.override → Spidermonkey builds don't respect CC/CXX/LINKER from mozconfig.vs-latest
Just as a test, I added to winbuildenv.sh the following:

+mk_export_correct_style CC
+mk_export_correct_style CXX

on top of a previous patch that sets this in build/win64/mozconfig.vs-latest:

+. "$topsrcdir/build/mozconfig.clang-cl"

and it got me a little bit farther, in that at least we're looking for clang-cl now:

checking for the target C compiler... not found
DEBUG: _cc: Trying clang-cl
ERROR: Cannot find the target C compiler

I noticed that $topsrcdir/clang/bin was not set in PATH:

Setting PATH = z:\task_1522865660\vs2017_15.6.0\VC\bin\Hostx64\x64;z:\task_1522865660\vs2017_15.6.0\SDK\bin\10.0.15063.0\x64;z:\task_1522865660\vs2017_15.6.0\VC\redist\x64\Microsoft.VC141.CRT;z:\task_1522865660\vs2017_15.6.0\SDK\Redist\ucrt\DLLs\x64;z:\task_1522865660\vs2017_15.6.0\DIA SDK\bin\amd64;.\VC\bin\Hostx64\x86;.\rustc\bin;.\clang\bin;c:\Program Files\Mercurial;c:\mozilla-build\7zip;c:\mozilla-build\info-zip;c:\mozilla-build\kdiff3;c:\mozilla-build\moztools-x64\bin;c:\mozilla-build\mozmake;c:\mozilla-build\msys\bin;c:\mozilla-build\msys\local\bin;c:\mozilla-build\nsis-3.01;c:\mozilla-build\python;c:\mozilla-build\python\Scripts;c:\mozilla-build\upx394w;c:\mozilla-build\wget;c:\mozilla-build\yasm;c:\Windows\system32;c:\Windows;c:\Windows\System32\Wbem;c:\Windows\System32\WindowsPowerShell\v1.0\;c:\Program Files\Amazon\cfn-bootstrap\;c:\Program Files (x86)\GNU\GnuPG\pub;c:\Program Files (x86)\Windows Kits\10\Windows Performance Toolkit\;c:\mozilla-build\python\lib\site-packages\pywin32_system32

Does this mean that clang binaries don't go into $topsrcdir/clang/bin for Spidermonkey builds? What can I do here? (Not sure who is right to ask this, picking glandium as a wild guess)
Flags: needinfo?(mh+mozilla)
Spidermonkey builds are *not* using mozconfigs *at all*, and they don't build the same way as Firefox either. If you ask me, those discrepancies are a major PITA. Anyways, sfink will be able to help you here.
Flags: needinfo?(mh+mozilla) → needinfo?(sphink)
(In reply to Mike Hommey [:glandium] from comment #6)
> Spidermonkey builds are *not* using mozconfigs *at all*

Then why does winbuildenv.sh source build/winNN/mozconfig.vs-latest?
Okay, correction, windows setup is so much of a burden that spidermonkey builds cherry picks one mozconfig. I just wish those builds were just using mozconfigs and mach.
(In reply to Mike Hommey [:glandium] from comment #8)
> Okay, correction, windows setup is so much of a burden that spidermonkey
> builds cherry picks one mozconfig.

... for paths setup, but probably not more.
That PATH has ".\clang\bin", probably from https://searchfox.org/mozilla-central/source/taskcluster/scripts/builder/sm-tooltool-config.sh#67 which seems to indicate that TOOLTOOL_CHECKOUT is being set to '.' rather than an absolute path. By default, it is set to $WORK, which defaults to $HOME/workspace. The debug output says

++ : .
++ : ./src
++ : .
++ export TOOLTOOL_CHECKOUT

which correspond to


: ${WORK:=$HOME/workspace}
: ${SRCDIR:=$WORK/build/src}

: ${TOOLTOOL_CHECKOUT:=$WORK}
export TOOLTOOL_CHECKOUT

which means something is setting WORK to '.'. What would do that?

Oh. https://searchfox.org/mozilla-central/source/taskcluster/taskgraph/transforms/job/spidermonkey.py#99 would do that. And it looks like that only applies to Windows, too. Added in bug 1387199, which I remember as Callek doing whatever he could just to get Windows spidermonkey builds migrated to taskcluster (I can't blame him!). So gaping holes are likely.
Hm, looks like the scripts are running in Z:\task_1522688410 which suggests an unpredictable location. So the minimal fix here would probably be to convert WORK and/or TOOLTOOL_CHECKOUT to absolute directories. But I don't know what a safe incantation would be without trying it out on a Windows box. Would WORK="$(cd "$WORK"; pwd)" do it? Or would that create some unholy string from evaluating backslashed path components?

Longer term, it would be wonderful to eliminate all of the winbuildenv.sh garbage and use something much closer to what firefox uses. autospider.py tries to fulfill a couple of goals:

1. use the build procedure expected by the JS team and external contributors & embedders, to keep it tested and working
2. work for both developers' builds as well as automation builds
3. work when invoked from a standalone spidermonkey source release

#3 isn't much of a constraint, as we package up the build system already. #1 can always be argued over. I don't know what objections people still have to using mach directly, nor how much weirdness in external contributors' eyes is acceptable before it becomes a significant barrier to contribution. For the record, #1 currently means roughly

  mkdir objdir
  cd objdir
  .../path/to/js/src/configure --enable-debug
  make

I think the JS team has gotten over the surprise of mozconfigs being consulted? (I think most of us now make sure to not have a file named plain 'mozconfig', but instead to always set $MOZCONFIG for our firefox builds if nondefault settings are needed.)
Flags: needinfo?(sphink)
Duplicate of this bug: 1502896
It looks like absolutizing things with cd and pwd works, or at least doesn't break anything. I tried two different ways to do that, and they both seemed to work. I'm not sure how to tell if they would give you what you need, though.

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=eefe9a78e4d7e27053b7566438f55a01f3c645be
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=87652af034c47098b3498df825a7ec044aff66ac

I also tried doing it at the source of the problem, but it looks like it produces a path with the toothpicks leaning the wrong way or something:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=704117c77e0f03260b3fba099f7df10233590e6c

[taskcluster:error] Uploading error artifact public/build from file public/build with message "Could not read directory 'Z:\\task_1540853063\\public\\build'", reason "file-missing-on-worker" and expiry 2018-11-26T22:48:00.614Z
[taskcluster:error] TASK FAILURE during artifact upload: file-missing-on-worker: Could not read directory 'Z:\task_1540853063\public\build'

dmajor: does something like https://hg.mozilla.org/try/rev/d542a2869b68409016f8cf3d7ddfd73d1cf066cc give you what you need?
Flags: needinfo?(dmajor)
Oh, I should also mention that there's some sort of evil VC hardcoding that you can see at https://hg.mozilla.org/try/rev/65cd4791affdffbcf17ed5a70e4fe319c8f8ce96
Well I don't know what happened, but apparently it's been long enough and the surrounding codebase has changed enough that this is not a problem anymore.
Flags: needinfo?(dmajor)
I really don't remember how I arrived at this patch in April, but all I can say is it seems to work now: https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=c1f53a9bac49e39afe2e3c7f2005cd264df3b9b5
(In the try push, I even threw in an #error for MSVC just to be sure configure wasn't lying to me.)
Assignee: nobody → dmajor
Attachment #9026461 - Flags: review?(sphink)
Attachment #9026461 - Flags: review?(mh+mozilla)
Attachment #9026461 - Flags: review?(sphink) → review+
Comment on attachment 9026461 [details] [diff] [review]
Bug 1451409: Convince Windows Spidermonkey builds to use clang-cl.

Review of attachment 9026461 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/devtools/automation/winbuildenv.sh
@@ +25,5 @@
>    . $topsrcdir/build/win32/mozconfig.vs-latest
>  fi
>  
> +mk_export_correct_style CC
> +mk_export_correct_style CXX

O_o how does this work, when the value set by mozconfig.vs-latest is "clang-cl", with no path in it? I don't expect mk_export_correct_style to change those values.
Comment on attachment 9026461 [details] [diff] [review]
Bug 1451409: Convince Windows Spidermonkey builds to use clang-cl.

Review of attachment 9026461 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/devtools/automation/winbuildenv.sh
@@ +25,5 @@
>    . $topsrcdir/build/win32/mozconfig.vs-latest
>  fi
>  
> +mk_export_correct_style CC
> +mk_export_correct_style CXX

So the reason this works is that autospider.py does crazy things, and _parses_ the stdout from the mozconfig, where mk_add_options prints out what it's passed per the function added in winbuildenv.sh. And mk_export_correct_style does call mk_add_options. That's the only reason this works. You could just call mk_add_options directly. Or "echo CC=$CC" would work too.

What it also means is that you should add LINKER here too. This currently only works because configure defaults to lld-link when using clang-cl already, but if you try to change LINKER in mozconfig.lld-link, you should see that your new value is not picked currently.

Overall, what this means is that autospider should just stop trying to be its own mozconfig handling, should just use mozconfigs. autospider was written back when js configure didn't support mozconfigs, but it does now.
Attachment #9026461 - Flags: review?(mh+mozilla) → review+
Pushed by dmajor@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f1e92f9db11
Convince Windows Spidermonkey builds to use clang-cl. r=sfink,glandium
https://hg.mozilla.org/mozilla-central/rev/4f1e92f9db11
Status: NEW → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
(In reply to Mike Hommey [:glandium] from comment #18)
> Overall, what this means is that autospider should just stop trying to be
> its own mozconfig handling, should just use mozconfigs. autospider was
> written back when js configure didn't support mozconfigs, but it does now.

Filed as bug 1510034.
Version: Version 3 → 3 Branch
You need to log in before you can comment on or make changes to this bug.