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

Making CMake/common usable as a .gitmodule in subprojects #551

Merged
merged 3 commits into from
May 22, 2017

Conversation

rdumusc
Copy link

@rdumusc rdumusc commented May 19, 2017

The new CLONE_SUBPROJECTS option offers extra granularity between building with subprojects and clone operations. This is what we discussed today but I am not sure if such fine-grained control is needed. We could also simply replace DISABLE_SUBPROJECTS with a single SUBPROJECTS=ON switch. Both solutions would satisfy the criteria of no clone operations during the configure step by default.

Copy link
Member

@tribal-tec tribal-tec left a comment

Choose a reason for hiding this comment

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

Looks good. Missing Changelog though. For integration into the projects wrt submodule instead of gitexternals, you would like to that for the release already?

@eile
Copy link
Member

eile commented May 22, 2017

Is there a use case where using subprojects and cloning sub projects needs to be separated? I would have one option for both, SUBPROJECTS=ON as you suggested.

@rdumusc
Copy link
Author

rdumusc commented May 22, 2017

Right, doing the changelog now.
Yes, we could do it for the release while we are touching all projects, if everybody agrees.
Yes, now that I've thought about it again I believe this is the best approach. Having subprojects ON is a reasonable default, because if you have (one or more) subproject sources you most likely want to use them. What is important is that users have the flexibility to get the subprojects the way they want (manual git clone, automatic clone missing projects with -DCLONE_SUBPROEJCTS=ON, scp to a remote machine, pointing to an existing COMMON_SOURCE_DIR, etc...), without side-effect on the sources during the configure phase by default, which is the major complaint we received. Having a single SUBPROJECTS=ON switch does not give the same flexibility.

@@ -1,8 +1,22 @@
# CMake Modules

This repository contains common CMake modules and a collection of find scripts
to locate non-CMake dependencies. To use it, create a .gitexternals file in your
project with the content:
to locate non-CMake dependencies. The recommended way to use it is:
Copy link
Member

Choose a reason for hiding this comment

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

As a matter of fact, the only recommended way I would document is the new submodule one. I was considering after we have updated all our projects, we can remove GitExternal.cmake and move what we still need for subproject cloning to SubProject.cmake. If we don't reach a conclusion now, we could discuss this in the next team-meeting.

@rdumusc
Copy link
Author

rdumusc commented May 22, 2017

good to merge?

@tribal-tec
Copy link
Member

+1 from my side

execute_process(COMMAND "${GIT_EXECUTABLE}" submodule init
WORKING_DIRECTORY "${DIR}" OUTPUT_QUIET)
execute_process(COMMAND "${GIT_EXECUTABLE}" submodule deinit CMake/common
WORKING_DIRECTORY "${DIR}" OUTPUT_QUIET ERROR_QUIET)

Choose a reason for hiding this comment

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

This looks suspicious to me. If you do this and I need to update the CMake/common submodule in my working copy do I need to git submodule init again?

Copy link
Author

Choose a reason for hiding this comment

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

This deinit only applies to subprojects. In for some reason you want to have a clone of CMake/common in a subproject, then yes, you need to init it there. That seems logical to me, do you have a use case in mind where this isn't what you expect?

Choose a reason for hiding this comment

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

Not that it deserves attention. I thought it was applying also to the top level CMake/common

Copy link
Author

Choose a reason for hiding this comment

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

no, because the top-level one is cloned directly as a git submodule with clone --recursive, it no longer goes through git externals.

@rdumusc rdumusc merged commit bff2a12 into Eyescale:master May 22, 2017
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.

4 participants