Testing to see if stylo can be built assumes llvm is the same as clang

RESOLVED FIXED in Firefox 57

Status

()

Core
Build Config
RESOLVED FIXED
6 months ago
6 months ago

People

(Reporter: Ken Moffat, Assigned: froydnj)

Tracking

(Blocks: 1 bug)

57 Branch
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [stylo])

Attachments

(1 attachment)

(Reporter)

Description

6 months ago
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20170820043346

Steps to reproduce:

Try to build 56.0b9 on a linux x86_64 system where LLVM-4.0.1 is installed (but NOT clang).

For people who build on linux, LLVM is required by a few things, such as Mesa, but clang is usually optional (some people like it, either because they hate the GPL, or because they think it can find extra bugs, other people think it is an unnecessary encumbrance).

Making it an extra requirement is unpleasant for those of us who compile from source on less than state-of-the-art machines. Adding rust in the belief it will reduce vulnerabilities is one thing, requiring an extra C/C++ compiler is quite another.

The error message gives no indication why it has been triggered in 56, but looking at the code it seems to be part of a "decide whether or not we *can* build stylo" test, so I would expect it to either decide the answer is "no", perhaps with a message, or else to suggest that using "--disable-stylo" can be used if people really want to build without clang.
 



Actual results:

It **** out unnecessarily:

ERROR: Could not find the clang shared library in the path /usr/lib
returned by `llvm-config --libdir` (searched for files [u'libclang.so.1', u'libclang.so']).
Please check your system configuration.




Expected results:

It should have decided that stylo could not be built, since the error seems to come from a test which tries to decide if stylo *can* be built.

For linux users, LLVM is required by a few packages such as Mesa, but clang is optional (and only currently useful either for developers who think it will help them in some way, or by people who are scared of the GPL). Assuming that LLVM implies clang is currently wide of the mark, and adding a second C/C++ compiler as a build requirement is unpleasant.

In some ways, forcing clang is more unpleasant than forcing rust because although the build time for clang is less, it does not offer the same hope of reducing vulnerabilities, it merely offers different corner-cases.

If there is some reason why clang is essential for stylo, it would alternatively be helpful to mention the benefits, whilst noting that for the moment this can be disabled by using --disable-stylo.
You can install Stylo's expected clang version using `mach bootstrap`.
Blocks: 1345321
Component: Untriaged → Build Config
Product: Firefox → Core
Whiteboard: [stylo]
Version: 52 Branch → 57 Branch
(Assignee)

Comment 2

6 months ago
Thanks for filing this bug!

(In reply to Ken Moffat from comment #0)
> It should have decided that stylo could not be built, since the error seems
> to come from a test which tries to decide if stylo *can* be built.

What is probably not clear is that stylo is enabled by default for the platform you're trying to build (in fact, for all of our major desktop platforms: Windows/Mac/Linux).  Given that fact, configure is checking whether the necessary pieces to actually build stylo exist on your system; since they do not, configure fails.  This is not significantly different from failing to configure if audio library headers or similar don't exist.  Silently deciding to not build stylo would be possible, but would greatly increase confusion for people who really did want stylo to be built, but were lacking the necessary pieces.  It's also better to fail as early in the build process as possible if something doesn't look like it's going to work.

> If there is some reason why clang is essential for stylo, it would
> alternatively be helpful to mention the benefits, whilst noting that for the
> moment this can be disabled by using --disable-stylo.

Enumerating all the benefits of stylo requiring clang is probably not appropriate in a configure message, but we can definitely mention the existence of --enable-stylo.
(Assignee)

Comment 3

6 months ago
Created attachment 8904621 [details] [diff] [review]
clarify bindgen error messages around clang

Stylo's recent enabled-by-default behavior, combined with some Linux
distributions's packaging of LLVM separately from Clang, causes
confusion.  To allay such confusion, rearrange the configure checks to
do two things:

1) Check for the clang binary prior to checking for clang's shared
   libraries; it should be more obvious what you need to install from
   an error about clang, and installing clang should pull in the
   necessary clang libraries, avoiding the following scenario:

     - run configure
     - get error message about libclang
     - install libclang
     - run configure
     - get error message about clang

2) Provide some context for what to do to avoid this error; the user may
not understand why we need a separate C/C++ compiler when they already
have a perfectly suitable one on their system.
Attachment #8904621 - Flags: review?(giles)
Comment on attachment 8904621 [details] [diff] [review]
clarify bindgen error messages around clang

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

I'm not entirely happy with this approach, but perhaps I'm confused. Can you explain how the sequence in (1) occurs? The original report was about llvm-config being part of the llvm system package, but libclang being a separate, uninstalled package (clang-libs or libclang1-4.0) so I don't understand why the build would then complain about clang. 

It's only `libclang` which is necessary for building stylo, not the clang frontend itself. Basically, we run a binding generator which uses libclang to parse C++ header files. Worse, it needs to be a fairly recent version, which is why bootstrap installs a mozilla build instead of just the system package.

Giving better error messages is definitely an improvement. If I'm correct about just needing libclang I think we could resolve the bug just by adding some instructions about what to do. Keep in mind there are two audiences: those building the development tip of Firefox to contribute to it, and those building release Firefox for downstream packaging or use, who may need to use system resources.
Attachment #8904621 - Flags: review?(giles) → review-
(In reply to Ralph Giles (:rillian) | needinfo me from comment #4)
> It's only `libclang` which is necessary for building stylo, not the clang
> frontend itself. Basically, we run a binding generator which uses libclang
> to parse C++ header files. Worse, it needs to be a fairly recent version,
> which is why bootstrap installs a mozilla build instead of just the system
> package.

No, we actually need clang too. Well, the clang-sys crate wants it. (iirc)
(Assignee)

Comment 6

6 months ago
Comment on attachment 8904621 [details] [diff] [review]
clarify bindgen error messages around clang

glandium is correct: the clang-sys crate requires the clang binary, as well as requiring libclang, so we really do need to check for both things.

re-requesting review accordingly.
Attachment #8904621 - Flags: review- → review?(giles)
Comment on attachment 8904621 [details] [diff] [review]
clarify bindgen error messages around clang

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

I see. bindgen calls through clang-sys::support::Clang to invoke the clang executable to get the default search paths for include files, which apparently aren't available just through libclang APIs. And toolkit/moz.configure checks for the executable up front so it can set CLANG_PATH for clang-sys can find it correctly. Sorry for the confusion.
Attachment #8904621 - Flags: review?(giles) → review+
(Assignee)

Comment 8

6 months ago
No worries, thanks for the review!  It is not at all obvious that we would require clang.  Maybe we should comment on that somewhere in bindgen_config_paths?
(Assignee)

Updated

6 months ago
Assignee: nobody → nfroyd
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

Comment 9

6 months ago
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/58e5b2cc2d8d
clarify bindgen error messages around clang; r=rillian
https://hg.mozilla.org/mozilla-central/rev/58e5b2cc2d8d
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.