Skip to content

Conversation

cameel
Copy link
Collaborator

@cameel cameel commented Aug 30, 2024

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.

@cameel cameel requested review from aarlt and clonker August 30, 2024 15:26
@cameel cameel self-assigned this Aug 30, 2024
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)
Copy link
Collaborator Author

@cameel cameel Aug 30, 2024

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)
Copy link
Collaborator Author

@cameel cameel Aug 30, 2024

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.

clonker
clonker previously approved these changes Sep 1, 2024
Copy link
Member

@clonker clonker left a 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

aarlt
aarlt previously approved these changes Sep 2, 2024
Copy link
Collaborator

@aarlt aarlt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@cameel cameel dismissed stale reviews from aarlt and clonker via 0cf81aa September 2, 2024 11:04
@cameel cameel force-pushed the tweak-cmake-options branch 2 times, most recently from 0cf81aa to 4891627 Compare September 2, 2024 11:10
- 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.
@cameel cameel force-pushed the tweak-cmake-options branch from 4891627 to 3184534 Compare September 2, 2024 13:55
@cameel cameel enabled auto-merge September 2, 2024 13:55
@cameel cameel merged commit d6f9519 into develop Sep 2, 2024
1 check passed
@cameel cameel deleted the tweak-cmake-options branch September 2, 2024 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants