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

formula: add FETCHCONTENT_FULLY_DISCONNECTED to std_cmake_args #17075

Merged
merged 1 commit into from Apr 12, 2024

Conversation

alebcay
Copy link
Member

@alebcay alebcay commented Apr 11, 2024

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

The FetchContent module can be used by CMake package authors to download/fetch resources at build time. The addition of this flag should disable such behavior.

See also the conversation at https://patchwork.yoctoproject.org/project/oe-core/patch/20220209125315.3177811-1-ross.burton@arm.com/#1436.

Also see the module documentation from CMake at https://cmake.org/cmake/help/latest/module/FetchContent.html, which bears this info and warning:

When this option is enabled, no attempt is made to download or update any content. It is assumed that all content has already been populated in a previous run or the source directories have been pointed at existing contents the developer has provided manually (using options described further below). When the developer knows that no changes have been made to any content details, turning this option ON can significantly speed up the configure stage. It is OFF by default.

The FETCHCONTENT_FULLY_DISCONNECTED variable is not an appropriate way to prevent any network access on the first run in a build directory. Doing so can break projects, lead to misleading error messages, and hide subtle population failures. This variable is specifically intended to only be turned on after the first time CMake has been run. If you want to prevent network access even on the first run, use a dependency provider and populate the dependency from local content instead.

I would posit that it would be desirable for builds to break if they attempt to download further resources, since packagers/reviewers might not notice the quick line or two in the build log that indicates that a resource/dependency is being downloaded. This behavior could be explicitly enabled if someone adds the equivalent flag to turn it OFF, of course.

Another alternative to explore is to add some kind of custom dependency provider as noted in the CMake doc. But it sounds like if we just "blackhole" content fetches, CMake will fall back to the default provider anyways.


A more powerful and probably more robust approach would be to offer full network isolation for formulae that we expect to be able to build offline (basically, most C/C++ things).

@alebcay alebcay marked this pull request as draft April 11, 2024 17:28
Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

This makes sense to me! As you say: can always be removed for individual formulae if needed but this default makes more sense to me 👍🏻

Copy link
Member

@carlocab carlocab left a comment

Choose a reason for hiding this comment

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

Love this, thank you

@alebcay alebcay marked this pull request as ready for review April 12, 2024 13:25
@carlocab carlocab merged commit b16112f into Homebrew:master Apr 12, 2024
23 checks passed
carlocab added a commit to Homebrew/homebrew-core that referenced this pull request Apr 15, 2024
carlocab added a commit to Homebrew/homebrew-core that referenced this pull request Apr 15, 2024
kevinAlbs added a commit to kevinAlbs/homebrew-brew that referenced this pull request Apr 16, 2024
This is to address a build failure reported in MONGOCRYPT-667
Homebrew includes FETCHCONTENT_FULLY_DISCONNECTED=ON as part of Homebrew/brew#17075
kevinAlbs added a commit to kevinAlbs/homebrew-brew that referenced this pull request Apr 16, 2024
This is to address a build failure reported in MONGOCRYPT-667
Homebrew includes FETCHCONTENT_FULLY_DISCONNECTED=ON as part of Homebrew/brew#17075
dmoody256 pushed a commit to mongodb/homebrew-brew that referenced this pull request Apr 16, 2024
* update to libmongocrypt 1.9.0

* set FETCHCONTENT_FULLY_DISCONNECTED=OFF

This is to address a build failure reported in MONGOCRYPT-667
Homebrew includes FETCHCONTENT_FULLY_DISCONNECTED=ON as part of Homebrew/brew#17075
@craigscott-crascit
Copy link

Hi folks, CMake co-maintainer and author of the FetchContent module here. I was made aware of this PR from an issue in CMake's issue tracker (https://gitlab.kitware.com/cmake/cmake/-/issues/25946). Please do not use FETCHCONTENT_FULLY_DISCONNECTED for this. As the highlighted documentation in the PR description says, it must not be used on the first run, and this PR violates that.

A dependency provider is what you should be using to intercept all find_package() and FetchContent_MakeAvailable() calls. This works for CMake 3.24 and later. Your dependency provider can do more sensible things when it sees such calls, including providing warnings or errors for things that cannot be provided (be careful about calls to find_package() for things like FindThreads, which are provided directly by CMake and should always be valid). If you only need to intercept FetchContent_MakeAvailable() and not find_package(), your provider can specify that when registering it.

The one caveat to the above is that dependency providers don't intercept a FetchContent_Populate() call, which was the old way before FetchContent_MakeAvailable() was added to CMake. Projects should be moving away from calling FetchContent_Populate(), and I intend to formally deprecate direct calls to that command in the next CMake release (3.30) or the one after.

As additional background, vcpkg also does/did (ab)use FETCHCONTENT_FULLY_DISCONNECTED for a similar reason as here in homebrew. I found that out because it broke certain use cases that were important to end users, in particular an increasingly common pattern used by companies. That scenario may be less relevant for homebrew, but I mention it to highlight that FETCHCONTENT_FULLY_DISCONNECTED can break things, and it really isn't the right tool for what you want to achieve here.

@MikeMcQuaid
Copy link
Member

@craigscott-crascit thanks for chiming in!

A dependency provider is what you should be using to intercept all find_package() and FetchContent_MakeAvailable() calls.

Do you have any more documentation about what a dependency provider is and how to create one?

That scenario may be less relevant for homebrew, but I mention it to highlight that FETCHCONTENT_FULLY_DISCONNECTED can break things, and it really isn't the right tool for what you want to achieve here.

In this case: we do actively want to break things 😅. We're slowly attempting to move our formula installs to not query the network at all during install time. To do so requires things to be "broken" in our CI process and building from source (which we do not support) so we can add the necessary FETCHCONTENT_FULLY_DISCONNECTED disable. This also makes it dramatically easier for us to figure out which CMake dependents are relying on fetching content at install time.

Unless I'm missing something: I don't see how this would affect things for users after the package has been installed from a binary with Homebrew, just our build process for our binary packages.

@craigscott-crascit
Copy link

Do you have any more documentation about what a dependency provider is and how to create one?

I'd start with the Dependency Providers section of the Using Dependencies Guide. That's a reasonable introduction to what they are. From there, the details of how to define and register a provider are in the Dependency Providers section of the cmake_language() documentation.

In this case: we do actively want to break things 😅.

Yes, but the way you're doing it can make it really hard to work out why something breaks, or if you're really unlucky, it won't break and the project will build something different to what was intended.

We're slowly attempting to move our formula installs to not query the network at all during install time. To do so requires things to be "broken" in our CI process and building from source (which we do not support) so we can add the necessary FETCHCONTENT_FULLY_DISCONNECTED disable. This also makes it dramatically easier for us to figure out which CMake dependents are relying on fetching content at install time.

Unless I'm missing something: I don't see how this would affect things for users after the package has been installed from a binary with Homebrew, just our build process for our binary packages.

If you set FETCHCONTENT_FULLY_DISCONNECTED to true, you're saying that you've previously run CMake and everything that FetchContent expects to be there is still there. FetchContent will then bypass all its logic, and CMake execution will continue assuming the dependency has been populated in the way FetchContent expected. When you set FETCHCONTENT_FULLY_DISCONNECTED to true on the first run, you end up with none of the population. When CMake execution continues, you're at the mercy of whatever tries to use something that isn't defined. This can be a variable, a function, a CMake target, a property or any other side effect that would normally occur from the FetchContent population. Depending on what the project tries to do, it might not error out, it might instead make a different decision based on what exists. The result might be the project was configured with the expectation that some component is enabled, but the internal logic of the project might decide to disable it. There would potentially be nothing obvious to tell the user this has occurred and they end up building the project in a different configuration to what they expected, with no warning about it. And such misconfigurations can be really hard to track down (speaking from my own experience with vcpkg doing exactly what you're doing here with homebrew).

@alebcay
Copy link
Member Author

alebcay commented May 1, 2024

I'd start with the Dependency Providers section of the Using Dependencies Guide. That's a reasonable introduction to what they are. From there, the details of how to define and register a provider are in the Dependency Providers section of the cmake_language() documentation.

From the linked doc, I saw this excerpt:

If the request is for one of the specified when the provider was set, CMake calls the provider's with a set of method-specific arguments. If the provider does not fulfill the request, or if the provider doesn't support the request's method, or no provider is set, the built-in find_package() or FetchContent_MakeAvailable() implementation is used to fulfill the request in the usual way.

From this reading - does this mean that if a custom dependency provider just outright denies the request (or does not fulfill it - unclear on the semantic differences in what way a custom provider could turn down a request), the default provider will be used anyways? If so, that sounds like a custom dependency provider would not be successful in preventing components from being fetched at build time (and halting the build process with an error condition).

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

5 participants