Closed Bug 1332689 (st-an-3.9) Opened 3 years ago Closed 5 months ago

Remove clang <3.9 specific code from the clang-plugin

Categories

(Firefox Build System :: Source Code Analysis, defect)

defect
Not set

Tracking

(firefox72 fixed)

RESOLVED FIXED
mozilla72
Tracking Status
firefox72 --- fixed

People

(Reporter: Nika, Assigned: emilio)

References

(Depends on 1 open bug)

Details

Attachments

(2 files)

According to :froydnj, Stylo is going to require clang >= 3.9 for running bindgen. If we have this requirement to build our tree anyway, we can increase the minimum supported clang version of our clang-plugin. This would allow us to remove code like: http://searchfox.org/mozilla-central/rev/30fcf167af036aeddf322de44a2fadd370acfd2f/build/clang-plugin/plugin.h#29-52 and simplify the build setup to not check for things like HAVE_NEW_ASTMATCHER_NAMES.
Depends on: 1332687
Does this mean people won't be able to build the tree using Apple clang any more?  That seems bad.  Are we planning to run bindgen as part of the build for everyone?
Flags: needinfo?(nfroyd)
The way things currently work is that bindgen can use a completely separate compiler/clang installation than the main CC/CXX.  Once stylo lands and is enabled by default, bindgen will be required, so developers will need a clang-$VERSION installation separate from XCode's clang.  (The idea here is that people shouldn't have to check in generated Rust to the tree; bindgen should be able to generate all the files we need during the build process.)  The plan is to have that clang-$VERSION package installed via |mach bootstrap|, and ideally that package will be installed from the same binaries we use in automation.

Sound reasonable?
I'm assuming that we couldn't use Apple's clang with bindgen even if we wanted to, since I suspect the necessary libraries/headers aren't really provided?
Flags: needinfo?(nfroyd)
(In reply to Nathan Froyd [:froydnj] from comment #3)
> The way things currently work is that bindgen can use a completely separate
> compiler/clang installation than the main CC/CXX.  Once stylo lands and is
> enabled by default, bindgen will be required, so developers will need a
> clang-$VERSION installation separate from XCode's clang.  (The idea here is
> that people shouldn't have to check in generated Rust to the tree; bindgen
> should be able to generate all the files we need during the build process.) 
> The plan is to have that clang-$VERSION package installed via |mach
> bootstrap|, and ideally that package will be installed from the same
> binaries we use in automation.
> 
> Sound reasonable?

Yes it sounds awesome.  FWIW I'm essentially writing the code that does the last part in bug 1328454.  :-)

(In reply to Nathan Froyd [:froydnj] from comment #4)
> I'm assuming that we couldn't use Apple's clang with bindgen even if we
> wanted to, since I suspect the necessary libraries/headers aren't really
> provided?

Yes that's correct.  (Similarly you can't use it if you want to enable static analysis.)
(In reply to Michael Layzell [:mystor] from comment #2)
> (In reply to :Ehsan Akhgari from comment #1)
> > Does this mean people won't be able to build the tree using Apple clang any
> > more?  That seems bad.  Are we planning to run bindgen as part of the build
> > for everyone?
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/210b8d21be06#l1.21 is
> the new code for stylo IIRC (currently only on inbound).

Ugh, it sucks that this wasn't caught at review time.  This code isn't even using the same environment variable name.  Sigh. :(
FWIW I'm bringing up the static analysis builders slowly to clang 3.9.
Alias: st-an-3.9
Assignee: nobody → ehsan
Depends on: 1331957, 1331012, 1337233
Product: Core → Firefox Build System
andi, you might be interested in working on this :-) - Could be some nice cleanup!
Flags: needinfo?(bpostelnicu)
Assignee: ehsan → nobody

I accidentally wrote patches for this when diagnosing some sccache-dist issue.

Assignee: nobody → emilio

This is just drive-by, but it confused me. We require at least clang 4 to build
nowadays.

Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/88ad4b97fe78
Remove clang 3.8 compat code. r=andi
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/98470c1a958c
Remove clang 3.9 compat code for the clang-plugin. r=andi
Status: NEW → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla72
Flags: needinfo?(bpostelnicu)
You need to log in before you can comment on or make changes to this bug.