-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support extra compiler flags during building #39191
Conversation
daa178a
to
12a936a
Compare
What would be a good place to document this feature? |
We currently don't have documentation for directly using the build-runtime.sh script since we direct people to use the root build scripts, so there isn't actually a good place to put docs. Can you add some comments around the changes you've made to explain them? That should cover it enough. |
Many Linux distributions like to use an extra set of compiler flags (via `CFLAGS`, `CXXFLAGS` and `LDFLAGS`) to produce builds that are hardened against vulnerabilities and exploits. The flags sometimes also enable extra warnings to inform packagers about potential memory issues. This pach adds support for that to dotnet/runtime. The obvious method to make this work is to just export the `CFLAGS`, `CXXFLAGS`, and `LDFLAGS` directly. This, however, enables those flags during configure-time (aka `cmake` without `--build` too). That means several cmake configure tests get executed with unexpected compiler flags. These configure tests can then report incorrect results. For example, https://bugzilla.redhat.com/show_bug.cgi?id=1712158 demonstrates an issue where the check for `strerror_r` in the runtime comes to the wrong conclusion because `-Wall` is enabled and a variable is unused. A slightly longer fix is to support another set of environment variables, and use them to set `CFLAGS`, `CXXFLAGS`, `LDFLAGS`, but only for the invocation of `cmake --build`/`make`. See dotnet#35727 for the complete details. Fixes dotnet#35727
12a936a
to
a22ec71
Compare
Do the changes look okay? |
Assuming there are no objections or further reviews required, can someone please merge this? I don't have merge powers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you!
@janvorli, for couple of years, the extra flags support has been via |
Hmm, I've forgotten about that. @omajid I suppose that would work for you as well, so we can revert this change, right? |
I thought my use case was not being met. But maybe I was wrong. If we revert this now, can I try do another PR if it turns out the |
I wonder if you could try the other way first and we would revert the change only if the other way doesn't work for you? |
In addition to cmakeargs support; the environment variable support is actually quite the opposite: we fixed Clear Linux build issue: #2188, where the default environment had CFLAGS, CFFLAGS and CXXFLAGS etc. set, and was conflicting with one of the requirements of coreclr. We cleared that conflicting value explicitly in cmake script. This means this PR is not needed for any of the flags, we can set the flags in the environment and it should just work: # option 1
CFLAGS={some value} ./build.sh # should work without this change
# option 2
./build.sh -cmakeargs -DCMAKE_C_FLAGS={some value} # should work without this change |
Many Linux distributions like to use an extra set of compiler flags (via
CFLAGS
,CXXFLAGS
andLDFLAGS
) to produce builds that are hardened against vulnerabilities and exploits. The flags sometimes also enable extra warnings to inform packagers about potential memory issues.This pach adds support for that to dotnet/runtime.
The obvious method to make this work is to just export the
CFLAGS
,CXXFLAGS
, andLDFLAGS
directly. This, however, enables those flags during configure-time (akacmake
without--build
too). That means several cmake configure tests get executed with unexpected compiler flags. These configure tests can then report incorrect results.For example, https://bugzilla.redhat.com/show_bug.cgi?id=1712158 demonstrates an issue where the check for
strerror_r
in the runtime comes to the wrong conclusion because-Wall
is enabled and a variable is unused.A slightly longer fix is to support another set of environment variables, and use them to set
CFLAGS
,CXXFLAGS
,LDFLAGS
, but only for the invocation ofcmake --build
/make
.See #35727 for the complete details.
Fixes #35727
Some questions:
cmake --build
/make
in the runtime build? I couldn't find any other place wherecmake --build
is called.EXTRA_CFLAGS
a good enough name? Any suggestions for a better name?