Closed Bug 1457162 Opened 2 years ago Closed 2 years ago

EXPAND_LIBS_LIST_STYLE checks do not include CFLAGS

Categories

(Firefox Build System :: General, enhancement)

3 Branch
enhancement
Not set

Tracking

(firefox62 fixed)

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: tjr, Assigned: tjr)

References

Details

Attachments

(2 files, 1 obsolete file)

In the initial conftest.c compilation, we include $CFLAGS:
https://searchfox.org/mozilla-central/rev/78dbe34925f04975f16cb9a5d4938be714d41897/build/autoconf/expandlibs.m4#17

However in the subsequent list checks we do not include CFLAGS
(e.g. https://searchfox.org/mozilla-central/rev/78dbe34925f04975f16cb9a5d4938be714d41897/build/autoconf/expandlibs.m4#19)

If CFLAGS contains something that would check the object file output (like, say -fuse-ld=lld) - then all of the list checks will fail.
Assigning to tjr since he attached patches
Assignee: nobody → tom
Attachment #8971665 - Flags: review?(ted) → review?(nfroyd)
Comment on attachment 8971665 [details]
Bug 1457162 Include CFLAGS and CPPFLAGS during the EXPAND_LIBS_LIST_STYLE check

https://reviewboard.mozilla.org/r/240434/#review246698
Attachment #8971665 - Flags: review?(nfroyd) → review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b51c2e39a12a
Include CFLAGS and CPPFLAGS during the EXPAND_LIBS_LIST_STYLE check r=froydnj
Keywords: checkin-needed
Hm. So I'm not sure why; but this patch breaks the _Windows_ builds and causes them to select the list file type 'none' instead of their normal 'list'.  I'll have to investigate this.
Flags: needinfo?(tom)
So I traced this problem down to the fact that on Windows we add -TC to CFLAGS which forces the object file to be treated as a C file.
I have a successfull full try run just by removing -TC which doesn't seem to be required (at least based on TaskCluster output)...
Comment on attachment 8979089 [details]
Bug 1457162 Remove -TC from CFLAGS

https://reviewboard.mozilla.org/r/245342/#review251490

::: js/src/old-configure.in:147
(Diff revision 1)
>      if test "$GCC" != "yes"; then
>          # Check to see if we are really running in a msvc environemnt
>          _WIN32_MSVC=1
>  
>          # Make sure compilers are valid
> -        CFLAGS="$CFLAGS -TC -nologo"
> +        CFLAGS="$CFLAGS -nologo"

I doubt this will harm anything (as evidenced by your try push), but it does give us a weird asymmetry with `CXXFLAGS` still including `-TP` below.
Attachment #8979089 - Flags: review+
Attachment #8979089 - Flags: review?(core-build-config-reviews)
Comment on attachment 8971665 [details]
Bug 1457162 Include CFLAGS and CPPFLAGS during the EXPAND_LIBS_LIST_STYLE check

https://reviewboard.mozilla.org/r/240434/#review251488

I really think at this point we should just drop all these configure tests and hardcode the list style based on the linker we know is in use, but this is fine to unblock the work you're doing.
Attachment #8971665 - Flags: review?(ted) → review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/bf7ac536eb94
Remove -TC from CFLAGS r=ted
https://hg.mozilla.org/integration/autoland/rev/a77d15466672
Include CFLAGS and CPPFLAGS during the EXPAND_LIBS_LIST_STYLE check r=froydnj,ted
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/bf7ac536eb94
https://hg.mozilla.org/mozilla-central/rev/a77d15466672
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Version: Version 3 → 3 Branch
You need to log in before you can comment on or make changes to this bug.