Skip to content

[FIXUP] cmake: Improve interface libraries code #72

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

Closed
wants to merge 51 commits into from

Conversation

hebasto
Copy link
Owner

@hebasto hebasto commented Jan 9, 2024

We use interface libraries to encapsulate global / common / group build options. However, the current cmake-staging branch has a few issues:

  1. An interface library name should be more expressive for better reading and reasoning about the code (pointed by theuni and TheCharlatan during the recent offline meeting).
  2. The core library is referenced in the "cmake: Check system symbols" commit, which comes before introducing it in the "cmake: Add platform-specific definitions and properties" one.
  3. The get_target_interface helper function shouldn't belong to the ProcessConfigurations module.

This PR fixes all issues mentioned above.

Unfortunately, I failed to put this PR changes into the fixup / autosquash routine. So the plan is to undraft it just after the next sync to the master branch, then merge this PR and adjust the commit history in the following one.

@hebasto
Copy link
Owner Author

hebasto commented Jan 9, 2024

cc @theuni @TheCharlatan

@theuni
Copy link

theuni commented Jan 9, 2024

Thanks for the renames. LGTM.

hebasto and others added 28 commits January 9, 2024 14:21
Co-authored-by: Cory Fields <[email protected]>
Co-authored-by: Vasil Dimov <[email protected]>
To configure CMake for cross-compiling, use
`--toolchain depends/${HOST}/share/toolchain.cmake` command-line option.

if(HARDENING)
add_library(hardening INTERFACE)
add_library(hardening_interface INTERFACE)
target_link_libraries(core_interface INTERFACE hardening_interface)

Choose a reason for hiding this comment

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

Why is this moved up here?

Copy link
Owner Author

@hebasto hebasto Jan 9, 2024

Choose a reason for hiding this comment

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

To make both lines visually connected. The benefit of interface libraries (EDIT: in comparison to keeping build options in variables) is that the target_link_libraries command might be placed anywhere after the add_library one.

@hebasto hebasto marked this pull request as ready for review January 9, 2024 16:14
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 ACK 1e6c90f

@hebasto
Copy link
Owner Author

hebasto commented Jan 9, 2024

Added a few minor fixes and improvements I found during rearranging the commit history.

@hebasto
Copy link
Owner Author

hebasto commented Jan 9, 2024

So the plan is to undraft it just after the next sync to the master branch, then merge this PR and adjust the commit history in the following one.

Please see #76. That PR is expected to got, not this one.

There must be zero diff between them.

@hebasto
Copy link
Owner Author

hebasto commented Jan 16, 2024

Closing. See #76 (comment).

@hebasto hebasto closed this Jan 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants