8000 CMake: INCLUDE_DIRECTORIES -> TARGET_INCLUDE_DIRECTORIES by AndreyG · Pull Request #4284 · libgit2/libgit2 · GitHub
[go: up one dir, main page]
Skip to content

CMake: INCLUDE_DIRECTORIES -> TARGET_INCLUDE_DIRECTORIES #4284

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

CMake: INCLUDE_DIRECTORIES -> TARGET_INCLUDE_DIRECTORIES #4284

wants to merge 2 commits into from

Conversation

AndreyG
Copy link
Contributor
@AndreyG AndreyG commented Jun 26, 2017

It's preferable to use target_include_directories instead of include_directories in modern CMake. It's also more consistent with target_link_libraries.

@AndreyG AndreyG mentioned this pull request Jun 26, 2017
Closed
@ethomson
Copy link
Member

Why are you dropping the explicit linkages? This looks weird: libgit2_clar does not link to libgit2 and this suggests to me that it does. It's already not obvious that we do not link to libgit2 (we can't, we need to test internal functions). This makes that less obvious still.

If you're worried about this not being sufficiently DRY, then why not set a variable to this set of libraries and then let TARGET_LINK_LIBRARIES point to both?

@pks-t
Copy link
Member
pks-t commented Jun 26, 2017

I'd ask for not touching that part at all regarding linkage, as I'm already doing this in #4282 in the way @ethomson asks for. The other part regarding the includes is still viable, though

@ethomson
Copy link
Member
ethomson commented Jun 26, 2017 •
edited
Loading

Agree. 👍 Thanks @pks-t

@AndreyG
Copy link
Contributor Author
AndreyG commented Jun 26, 2017

Ok, I will fix it. But it seems quite strange for me that tests for the library don't link to that library.

@ethomson
Copy link
Member

Again - they can't, at least not without making everything public. We want to be able to test our buffer implementation and our posix emulation on Windows, and we do not want those to be a public, exported part of the library.

@AndreyG
Copy link
Contributor Author
AndreyG commented Jun 26, 2017

I have reworked my commits to leave git history clean.

@ethomson
Copy link
Member

🎉 Awesome, thanks, @AndreyG !

@AndreyG
Copy link
Contributor Author
AndreyG commented Jun 26, 2017

@ethomson thank you for clarification!

@pks-t
Copy link
Member
pks-t commented Jun 27, 2017

I'd still prefer to pull only the commit regarding target_include_directories to keep conflicts with #4282 as small as possible.

@AndreyG
Copy link
Contributor Author
AndreyG commented Jun 27, 2017

@pks-t It will be OK for me if you just cherry-pick my commit 905d80f regarding target_include_directories to your branch #4282 and then close this pull request.

@pks-t
Copy link
Member
pks-t commented Jun 27, 2017

@AndreyG That's certainly helpful, thank you! I'll try to get Windows building with #4282 tomorrow, and if so I'll cherry-pick this. Otherwise I'll manually pick the single commit and merge

@pks-t
Copy link
Member
pks-t commented Jun 28, 2017

Manually merged via c26ce78. I've made a follow-up commit to also use TARGET_INCLUDE_DIRECTORIES for libgit2_clar.

Thanks for your work!

@pks-t pks-t closed this Jun 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@AndreyG @ethomson @pks-t
0