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

Use Tags for Dependencies with FetchContent_Declare #97

Closed
dschiller opened this issue Dec 6, 2023 · 4 comments · Fixed by #98
Closed

Use Tags for Dependencies with FetchContent_Declare #97

dschiller opened this issue Dec 6, 2023 · 4 comments · Fixed by #98
Labels
enhancement New feature or request

Comments

@dschiller
Copy link

dschiller commented Dec 6, 2023

Is your feature request related to a problem? Please describe.
You introduced a breaking change with 393a85e for all your dependencies (CMaize).

Describe the solution you'd like
For all your dependencies you also need to pin to tags with the GIT_TAG attribute (see https://cmake.org/cmake/help/latest/module/FetchContent.html) to not introduce breaklng changes.

Describe alternatives you've considered
Forking your CMakeTest repro and fix the FetchContent_Declare usage by using tags.

Additional context
It is good practice to pin to a tag instead to the master branch for dependencies. This is related to #64

@dschiller dschiller added the enhancement New feature or request label Dec 6, 2023
ryanmrichard added a commit that referenced this issue Dec 6, 2023
@ryanmrichard
Copy link
Contributor

@dschiller apologies, but I want to make sure I understand exactly what needs pinning. Are you referring pinning the version of CMaize we use in CMakeTest? That would require adding GIT_TAG here. If so, I absolutely agree. We should have been doing that from the start, and I just opened #98 to address that. Otherwise, can you please provide a bit more information as to what broke?

@dschiller
Copy link
Author

dschiller commented Dec 6, 2023

Yes Ryan, pinning the version of CMaize and all future Dependencies of CMakeTest to a specific tag. Regarding our broken code we believe that you changed the Cpp repository from public to private which made our code fail as we didn't update to use CMaize that time you switched to CMaize. However, pinning to a tag should be used for your repositories anyways for dependencies.

@ryanmrichard
Copy link
Contributor

@dschiller just to clarify, CMaize is Cpp. We renamed it. Nonetheless, #98 pins CMakeTest's dependencies so hopefully this issue won't come up again.

@dschiller
Copy link
Author

Okay, thank you for the clarification Ryan. And also thank you a lot for this fast solution!

ryanmrichard added a commit that referenced this issue Dec 6, 2023
* Update get_cmaize.cmake

Fixes #97

* versions CMakePPLang too

* bump minimum CMake version

* address Zach's comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants