Skip to content
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

cmake: Disable EXPORT_COMPILE_COMMANDS for all subtree targets #206

Merged
merged 2 commits into from
May 23, 2024

Conversation

hebasto
Copy link
Owner

@hebasto hebasto commented May 21, 2024

Fixes the second point from bitcoin#29790 (comment):

  1. Missed disabling EXPORT_COMPILE_COMMANDS property for targets in the crc32c subtree.

@hebasto
Copy link
Owner Author

hebasto commented May 22, 2024

See the recent push to bitcoin#29790 for the CI logs.

Copy link

@theuni theuni left a comment

Choose a reason for hiding this comment

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

Hmm, leaning towards concept NACK I think.

I guess the idea here is to set/restore the global around the subtree includes rather than setting the property per-lib?

I think I'd rather be explicit, even if there's a small risk of forgetting to set it, we'd notice eventually.

cmake/module/SaveRestoreVariable.cmake Outdated Show resolved Hide resolved
Disable `EXPORT_COMPILE_COMMANDS` for `crc32c` subtree targets
Disable `EXPORT_COMPILE_COMMANDS` for `secp256k1` subtree targets
@hebasto
Copy link
Owner Author

hebasto commented May 22, 2024

Hmm, leaning towards concept NACK I think.

I guess the idea here is to set/restore the global around the subtree includes rather than setting the property per-lib?

I think I'd rather be explicit, even if there's a small risk of forgetting to set it, we'd notice eventually.

Thanks! Reworked.

@@ -57,3 +57,4 @@ if(MSVC)
endif()

target_link_libraries(secp256k1 PRIVATE core_base_interface)
set_target_properties(secp256k1 PROPERTIES EXPORT_COMPILE_COMMANDS OFF)
Copy link

Choose a reason for hiding this comment

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

What will happen here when we switch to using secp's CMake build?

Copy link
Owner Author

@hebasto hebasto May 22, 2024

Choose a reason for hiding this comment

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

What will happen here when we switch to using secp's CMake build?

I can see two options.

  1. We keep setting the target property externally as we currently do.

Or

  1. Upstream secp build system can force this property disabled when being a subproject.

Copy link

@TheCharlatan TheCharlatan left a comment

Choose a reason for hiding this comment

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

lgtm

@hebasto hebasto merged commit 6c2be70 into cmake-staging May 23, 2024
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests Tests, benchmarks, fuzzing, CI etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants