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

remove cpm from project #772

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

remove cpm from project #772

wants to merge 2 commits into from

Conversation

anonrig
Copy link
Member

@anonrig anonrig commented Nov 5, 2024

A lot of people are disabling CPM while adding Ada to the respective platforms. CPM requires network access by default even if the builder doesn't enable tests, tools, benchmarks etc.

Example: https://github.com/NixOS/nixpkgs/blob/7eee17a8a5868ecf596bbb8c8beb527253ea8f4d/pkgs/by-name/ad/ada/package.nix#L22

@anonrig anonrig requested a review from lemire November 5, 2024 11:56
@lemire
Copy link
Member

lemire commented Nov 5, 2024

@anonrig

CPM requires network access by default even if the builder doesn't enable tests, tools, benchmarks etc.

Can you elaborate?

CPM is only ever included when tests or tools or benchmarks is enabled (if I am wrong, then there is a bug in our config). See the configuration:

if(ADA_TESTING OR ADA_BENCHMARKS OR ADA_TOOLS)
  include(cmake/CPM.cmake)
   # CPM requires git as an implicit dependency
   find_package(Git QUIET)
   # We use googletest in the tests
   if(Git_FOUND AND ADA_TESTING)
     CPMAddPackage(
       NAME GTest
       GITHUB_REPOSITORY google/googletest
       VERSION 1.15.2
       OPTIONS  "BUILD_GMOCK OFF" "INSTALL_GTEST OFF"
     )

If (ADA_TESTING OR ADA_BENCHMARKS OR ADA_TOOLS) is false, then CPM is not used. So it cannot trigger network access.

Let us compare with what you are replacing it with...

 if(ADA_TESTING OR ADA_BENCHMARKS OR ADA_TOOLS)
   find_package(Git QUIET)
   # We use googletest in the tests
   if(Git_FOUND AND ADA_TESTING)
     FetchContent_Declare(
       GTest
       GIT_REPOSITORY https://github.com/google/googletest.git
       GIT_TAG v1.15.2
     )
     set(BUILD_GMOCK OFF CACHE BOOL "" FORCE)
     set(INSTALL_GTEST OFF CACHE BOOL "" FORCE)
     FetchContent_MakeAvailable(GTest)
   endif()

As far as I can tell, if (ADA_TESTING OR ADA_BENCHMARKS OR ADA_TOOLS) is false, then again, no network access, but that's not better than before. If (ADA_TESTING OR ADA_BENCHMARKS OR ADA_TOOLS) is true, then you get network access.

One of the key feature of CPM is that it allows new builds to be configured while offline if all dependencies are available locally. What CPM allows you to do is to build a local repository (which you can share between projects) on disk. CPM will first go there, check that the correct version is available, and only download if it must do so. Meanwhile FetchContent_Declare will definitively hit the network. Further, all CPMAddPackage can be automatically turned into find_package, make it easy to go all local... something that FetchContent_Declare won't do.

If you do not want people to have to do...

  cmakeFlags = [
    # uses CPM that requires network access
    (lib.cmakeBool "ADA_TOOLS" false)
    (lib.cmakeBool "ADA_TESTING" false)
  ];

then let us turn these things off by default...

option(ADA_TOOLS "Build cli tools (adaparse)" ON)

It is perfectly reasonable to do so. There is no reason why building ada should require building the tests and the tools. Of course, changing a default has consequences, but this can be done as part of major release.

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.

2 participants