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 FetchContent instead of submodules #6322

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Conversation

jmoralez
Copy link
Collaborator

Proposes using FetchContent instead of submodules to make it easier to install the library, i.e. avoid issues like #6297

@jmoralez
Copy link
Collaborator Author

jmoralez commented Mar 5, 2024

@jameslamb do you support this? I'm not sure if we could do this

# Remove 'region', 'endregion', and 'warning' pragmas.
# This won't change the correctness of the code. CRAN does
# not allow you to use compiler flag '-Wno-unknown-pragmas' or
# pragmas that suppress warnings.
echo "Removing unknown pragmas in headers"
for file in $(find . -name '*.h' -o -name '*.hpp' -o -name '*.cpp'); do
sed \
-i.bak \
-e 's/^.*#pragma clang diagnostic.*$//' \
-e 's/^.*#pragma diag_suppress.*$//' \
-e 's/^.*#pragma GCC diagnostic.*$//' \
-e 's/^.*#pragma region.*$//' \
-e 's/^.*#pragma endregion.*$//' \
-e 's/^.*#pragma warning.*$//' \
"${file}"
done
find . -name '*.h.bak' -o -name '*.hpp.bak' -o -name '*.cpp.bak' -exec rm {} \;

with FetchContent.

@jameslamb
Copy link
Collaborator

@jameslamb do you support this? I'm not sure if we could do this

I'm -1 on this.

Things I'm concerned about:

These are totally solvable problems, but I don't think they're worth the effort just for the benefit "build works without you needing to init git submodules".

What do you think about creating a script in cmake/ like this:

function(assert_submodules_initialized)
  if(NOT EXISTS "${CMAKE_SOURCE_DIR}/external_libs/fmt")
    message(
       FATAL_ERROR
       "Missing required source directory: 'external_libs/fmt'. Run 'git submodule update --init --recursive' from the root of the repo and re-run cmake"
     )
  endif()
  # ... and then another for each of the other submodules ...
endfunction()

And then sourcing it and calling that function early in CMakeLists.txt?

assert_submodules_initialized()

I think that'd generate a clearer configure-time error than "fast_double_parser.h: no such file or directory", hopefully clear enough to allow people to fix their builds without opening issues here.

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

2 participants