-
Notifications
You must be signed in to change notification settings - Fork 72
Fix macOS CMake build: emit INTERFACE libraries for header-only support targets #1444
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
Draft
KHicketts
wants to merge
1
commit into
google:main
Choose a base branch
from
KHicketts:interface
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+40
−88
Draft
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
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'm guessing since these are autogenerated, the fix is actually in some generation script that I don't have access to?
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.
You have guessed correctly! Originally I wanted to open source our generation script but it's relatively entangled with our internal tooling atm and I don't have a good fix for it. But I can certainly reflect this change upstream and post here once the CMake is updated.
If you could help me out with understanding this fix that would be appreciated. My CMake knowledge is relatively limited. My understanding was
PUBLICwas a superset of functionality ofINTERFACE. Is that correct? And if so, why does swapping toINTERFACEfix the CMake build on mac?Thank you for the PR!
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.
:) thanks for the fast response!
That's a very fair question. It took me a while to understand it as well.
add_library(blah INTERFACE) [instead of STATIC] is the bit that actually fixes the build. [stops the empty archive creation]
You are correct about PUBLIC ----> PUBLIC = PRIVATE (let's .cc files see path) + INTERFACE (exposes path to consumers)
The PUBLIC -> INTERFACE changes are required because we changed the library to INTERFACE - we get a configuration error otherwise. CMake won't let you keep it as public.
In reality the PRIVATE part wasn't doing anything. It only applies to compiling the libraries own cc files and there weren't any so it's safe to drop it. PUBLIC - PRIVATE = INTERFACE.
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.
Thanks for the explanation I was missing the change in behavior from swapping STATIC -> INTERFACE. I've updated our CMakeLists.txt and they should now use INTERFACE everywhere in place of STATIC/PUBLIC.
It's exciting to hear you're trying out Crubit on mac. As you can see we have not done extensive testing for it yet. Is mac the host platform or the target platform?
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.
Thanks very much Ethan :). Mac is just my dev box. I wanted to look into getting this working with gcc as an experiment. Would be great to chat about it if you have bandwidth. 👀 It's easy to find a Hicketts on LinkedIn.
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 would be happy to do that! I'll reach out on LinkedIn. We also recently started a discord server, so it's easier for us to talk to OSS users: https://discord.gg/SpBTWJrH