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

If a name isn't provided, try to infer it from the git repo #202

Merged
merged 10 commits into from
Feb 17, 2021

Conversation

iboB
Copy link
Member

@iboB iboB commented Feb 17, 2021

Two reasons behind this PR

More compact CPMAddPackage calls

CPMAddPackage(
  GITHUB_REPOSITORY catchorg/Catch2
  VERSION 2.5.0
)

Less room for error in pasting CPMAddPackage calls

To me, and I suspect to many other users most packages look like this:

CPMAddPackage(
  NAME PackageName
  GITHUB_REPOSITORY user/PackageName
  VERSION x.y.z
)

When adding a new package, this gets pasted around and two identical occurrences of "PackageName" need to be replaced. If we follow the DRY principle, this repetition should be avoided and in the case of git repositories this is easy.

The old behavior still works for backwards compatibility and for the rare cases where the package name and the repo name aren't the same.

Copy link
Member

@TheLartians TheLartians left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, I think this is a great idea to shorten the syntax and avoid repetition!

cmake/CPM.cmake Outdated Show resolved Hide resolved
@TheLartians
Copy link
Member

Also feel free to update the main example in the readme to show off the shorter syntax! :-)

- Also reordered tests to ensure that the result is actually unset when needed
cmake/CPM.cmake Outdated Show resolved Hide resolved
@iboB
Copy link
Member Author

iboB commented Feb 17, 2021

Check. Suggested changes were added.

Copy link
Member

@TheLartians TheLartians left a comment

Choose a reason for hiding this comment

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

Awesome, thanks for the changes!

@TheLartians TheLartians merged commit 4cbf443 into cpm-cmake:master Feb 17, 2021
@TheLartians
Copy link
Member

Feature released in v0.30.0! 🎉

@gudnimg
Copy link

gudnimg commented Feb 17, 2021

@TheLartians
Hi, I'm glad to see all the improvements... I have one idea to improve the README:

This is what it looks like right now:

ExternalProject works similarly as FetchContent, however waits with adding dependencies until build time. This has a quite a few disadvantages, especially as it makes using custom toolchains / cross-compiling very difficult and can lead to problems with nested dependencies.

I think it makes sense to highlight the keywords ExternalProject and FetchContent like this: ExternalProject FetchContent or even link the words to the CMake documentation.

Idea 1

ExternalProject works similarly as FetchContent, however waits with adding dependencies until build time. This has a quite a few disadvantages, especially as it makes using custom toolchains / cross-compiling very difficult and can lead to problems with nested dependencies.


I just learned right now as I wrote this that you can create a link for coded words with

  • `keyword ` (keyword) :D)

Or like this:

Idea 2

ExternalProject works similarly as FetchContent however waits with adding dependencies until build time. This has a quite a few disadvantages, especially as it makes using custom toolchains / cross-compiling very difficult and can lead to problems with nested dependencies.

Edit: It's like this: [`ExternalProject`](https://cmake.org/cmake/help/latest/module/ExternalProject.html)

@TheLartians
Copy link
Member

@GunZi200 thanks for the suggestions! I think there are definitely some improvements that can be made to the readme. As the section you're referring to is written for users who already know both commands I think the link isn't necessary but highlighting the code does make sense. If you want, feel free to open a PR and we can discuss further changes there.

@iboB iboB mentioned this pull request Feb 18, 2021
@iboB iboB deleted the pkg-name-from-git-uri branch January 29, 2022 04:13
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

3 participants