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

Change cmake submodule macros #17

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Conversation

spahrenk
Copy link

@spahrenk spahrenk commented Jun 13, 2024

Description

CMake macros were added/altered to enable optional submodule initialization. Previously, all submodules listed in .gitmodules were updated by default. For use of HPWHsim by cse, it became evident that submodules used by HPWHsim for development and testing may not be needed in projects where use is limited to the HPWHsim library only, and it is preferable to disable automatic updating of those submodules when building cse. Those updates can be made conditional through individual, qualified calls to initialize_submodule(${submodule_name}), rather than a single call to initialize_submodules(). In either form, only submodules listed in .gitmodules will be updated. However, one could use update_submodule(${submodule_name}) to update a submodule without cross-referencing .gitmodules.

@spahrenk spahrenk self-assigned this Jun 13, 2024
@spahrenk spahrenk added the enhancement New feature or request label Jun 13, 2024
@spahrenk spahrenk requested a review from nealkruis June 13, 2024 20:41
@nealkruis
Copy link
Member

I think we can be more aggressive in these changes. If we do things cleanly, there shouldn't be a use case for initializing all submodules in .gitmodules anymore. I was thinking that we would have a single macro (call it add_submodule for now) that would allow vendor/CMakeLists.txt to look like this:

add_submodule(fmt)

if (${PROJECT_NAME}_BUILD_TESTING)
    add_submodule(googletest gtest)
endif()

By default, this macro will assume that the module, the directory, and the primary target all share the same name. If that's not the case, we can use additional arguments to make the appropriate distinctions.

The macro itself will look something like this:

macro(add_submodule module_name)
    if (NOT TARGET module_name) # Don't clone or add subdirectory if the module/target already exists
        update_submodule(module_name) # Maybe this could be expanded in this macro instead of being a separate macro?
        add_subdirectory(module_name)
    endif()
endmacro()

We could also adapt this to be more general to pass in:

  • A list of cache variables to set "ON"
  • A list of cache variables to set "OFF"
  • A list of cache variables to mark_as_advanced so they don't appear in the CMake GUI for higher level projects

See this article on how to handle extra arguments.

Let me know if I need to clarify anything here.

Copy link
Member

@nealkruis nealkruis left a comment

Choose a reason for hiding this comment

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

See comments in conversation thread...

@nealkruis nealkruis self-requested a review July 2, 2024 17:31
.gitmodules Outdated Show resolved Hide resolved
set(MacroArgs ${ARGN})
list(LENGTH MacroArgs NumArgs)

set(have_submodule FALSE)
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't appear to be used anywhere.

Copy link
Author

Choose a reason for hiding this comment

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

Used to count arguments. This has been rearranged.

if (NOT is_submodule)
string(COMPARE EQUAL ${line} "[submodule \"${module_ref_name}\"]" is_submodule)
if (is_submodule)
message(STATUS "\"${module_name}\" is a submodule of this project.")
Copy link
Member

Choose a reason for hiding this comment

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

Remove?

if(GIT_FOUND AND EXISTS "${PROJECT_SOURCE_DIR}/.git")
set(git_modules_file "${PROJECT_SOURCE_DIR}/.gitmodules")
if (EXISTS ${git_modules_file})
file(STRINGS ${git_modules_file} file_lines)
set(have_path FALSE)
Copy link
Member

Choose a reason for hiding this comment

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

This was already set to FALSE before.

Copy link
Author

Choose a reason for hiding this comment

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

but may be been changed within the loop

endif()
endif()
else()
message(STATUS "\"${module_name}\" is not a submodule of this project")
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be a FATAL_ERROR? Or maybe separate fatal errors for each situation:

  • It isn't a submodule in .gitmodules: "Submodule '${module_name}' not found."
  • It is a submodule, but you couldn't find the path: "Submodule '${module_name}' path not found."

Copy link
Author

Choose a reason for hiding this comment

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

In this case the module has not been added as a git submodule for this project, but may be a submodule of another submodule. For example, HPWHsim uses googletest via btwxt.


if(is_submodule)
if(have_path)
message(STATUS "Updating submodule \"${module_name}\" at ${submodule_path}")
Copy link
Member

Choose a reason for hiding this comment

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

I think this is where we want a status message since cloning can take a while and it explains what's happening on stdout. Generally, this should only happen for the initial clone of the repository (and not for updates). Maybe a better message would be: "Initializing ${submodule} submodule"?

WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}
RESULT_VARIABLE GIT_SUBMOD_RESULT)
if(GIT_SUBMOD_RESULT EQUAL "0")
message(STATUS "Successfully updated submodule \"${module_name}\"")
Copy link
Member

Choose a reason for hiding this comment

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

Remove?

endif()

if (TARGET ${module_name})
message(STATUS "Submodule \"${module_name}\" is a target.")
Copy link
Member

Choose a reason for hiding this comment

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

Remove?

message(STATUS "Submodule \"${module_name}\" is a target.")
else()
if(EXISTS "${module_path}")
message(STATUS "Adding subdirectory ${module_path} to this project")
Copy link
Member

Choose a reason for hiding this comment

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

Remove?


message("")
Copy link
Member

Choose a reason for hiding this comment

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

???

Copy link
Author

Choose a reason for hiding this comment

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

Give this version a try in a few situations and feel free to change the messaging however you prefer.

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 this pull request may close these issues.

2 participants