-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
Tweak advanced CMake options #15377
Tweak advanced CMake options #15377
Conversation
option(PEDANTIC "Enable extra warnings and pedantic build flags. Treat all warnings as errors." ON) | ||
option(PROFILE_OPTIMIZER_STEPS "Output performance metrics for the optimiser steps." OFF) | ||
option(USE_SYSTEM_LIBRARIES "Use system libraries" OFF) | ||
option(ONLY_BUILD_SOLIDITY_LIBRARIES "Only build solidity libraries" OFF) | ||
option(STRICT_NLOHMANN_JSON_VERSION "Strictly check installed nlohmann json version" ON) |
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.
I wanted to rename this to avoid confusion with STRICT_Z3_VERSION
, which works a bit differently, but then I thought that we might be better off just removing the version check altogether.
It seems to have been added way back in #3913. At that time we were still using jsoncpp, testing with Travis and downloading the library via CMake. Now all of that has changed and I think that the risk of getting the wrong version is much lower, especially with it now coming from a submodule.
nlohmann JSON is supposed to be more resilient and strict than jsoncpp so hopefully any version that builds and passes our test suite is fine to use anyway. We could have a check against the min version, but we should not have to enforce an exact one. We don't do that for fmtlib or ranges either. Having a specific version in the submodule is suffcient IMO.
"Only build library targets that can be statically linked against. Do not build executables or tests." | ||
OFF | ||
) | ||
mark_as_advanced(PROFILE_OPTIMIZER_STEPS) |
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.
This option is a hacky dev-only thing too so I think it should be marked as advanced
as well. We don't want to provide any backward-compatibility guarantees for it either.
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.
Looks good to me aside from that variable name remark which I don't think is mission critical in any case
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
0cf81aa
to
4891627
Compare
- The git submodule is pinned to that version, which should be enough of a hint that this is the version we expect. If someone builds with a different version and it passes tests, we should not block that.
4891627
to
3184534
Compare
Some tweaks to names and descriptions of options added in #15195. See #15081 (comment) for context.
I'd like to get that in before the release, while the options are still relatively fresh so that we're not stuck with them as is forever.