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: Refactor targets export variable and improve comments #789

Merged
merged 4 commits into from Sep 17, 2020

Conversation

offlinemark
Copy link
Contributor

@offlinemark offlinemark commented Dec 3, 2019

Hi, this PR does the following:

  • Refactor the TARGETS_EXPORT_NAME variable. Previously it was completely unused and there were multiple instances of ${CMAKE_PROJECT_NAME}Targets copied around the file. This moves the variable definition to the top, and then replaces those all the copy-pasted code with the variable.
  • Corrects the comments. Previously there was a comment at the export() command that mentioned find_package. It's actually the install(EXPORT... at the bottom that's related to find_package. The export command purely generates a script in the build dir.

This is consistent with other usages of the file.
PROJECT_NAME is only useful if there are subprojects, and there
aren't any here.
Previously, it was totally unused.
The `export()` command purely exports a cmake script into the build
directory; it is not related to find_package at all. It is technically
not necessary. The main use case would be a user copying it into their
internal cmake/ dir in their project, and then using `include()`'ing
that file in their cmake.

The `install(EXPORT...` command is what actually generates a cmake
module that is put on the filesystem in a location that can be found by
`find_package` when in config mode.
@offlinemark
Copy link
Contributor Author

offlinemark commented Dec 3, 2019

Pinging some potential reviewers: @jasjuang @leethomason @comargo @lsolanka
thank you!

@jasjuang
Copy link
Contributor

jasjuang commented Dec 4, 2019

I would rather not create a new variable TARGET_EXPORT_NAME because IMO ${CMAKE_PROJECT_NAME}Targets is pretty self-explanatory. It all comes down to personal preferences I think. I will leave it to @leethomason.

@offlinemark
Copy link
Contributor Author

I think that's a fair point but I'll just point out that the variable was already there, just unused :) I would say I have a slight preference towards the variable, since I don't see a strong reason to avoid it, and I think it makes the code more maintainable.

@jasjuang
Copy link
Contributor

jasjuang commented Dec 4, 2019

What do you mean by TARGET_EXPORT_NAME is already there? Didn't you create this new variable with a set call? I did a search in the master branch for TARGET_EXPORT_NAME and nothing pops up.

@offlinemark
Copy link
Contributor Author

offlinemark commented Dec 4, 2019

Ah, the actual variable is TARGETS_EXPORT_NAME (plural). Sorry about that!

@jasjuang
Copy link
Contributor

jasjuang commented Dec 4, 2019

I see, if it were me I will simply delete the line that creates TARGETS_EXPORT_NAME instead of using it.

@leethomason leethomason merged commit a0ce552 into leethomason:master Sep 17, 2020
@offlinemark offlinemark deleted the mark/cmake-clean branch September 17, 2020 23:28
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.

None yet

4 participants