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

Please consider to use git submodules instead of FetchContent #539

Open
yurivict opened this issue Sep 1, 2024 · 7 comments
Open

Please consider to use git submodules instead of FetchContent #539

yurivict opened this issue Sep 1, 2024 · 7 comments

Comments

@yurivict
Copy link

yurivict commented Sep 1, 2024

File fetching isn't allowed during package build in any sane packaging system.
For this reason FetchContent isn't compatible with packaging.

Please consider going back to git submodules.

@grojo-ea
Copy link
Contributor

grojo-ea commented Sep 2, 2024

Hello,

Thanks for bringing this up. We were trying the FetchContent approach to try to break the circular submodule dependency issue when cloning EASTL with --recursive (see #410 and #433). I was unaware that this would break packaging.

I'm unfamiliar with how package managers do the packaging process. Would it be enough to make it so that we can build/use EASTL itself without FetchContent but keep the FetchContent declarations in for EASTLTest?

If that's the case I think we can potentially make EABase use FetchContent for its testing project and bring in EABase as a submodule. This is the only dependency that's needed to build EASTL and the test projects (which are the ones that generate the submodule cycles) could still use FetchContent.

@yurivict
Copy link
Author

yurivict commented Sep 2, 2024

I maintain the FreeBSD port for EASTL and in the ports framework the cmake setup is such that FetchContent just doesn't work, even for tests.
FetchContent instructions need to be patched away and replaced with pre-downloaded directories.

Maybe it would be a good idea to keep both git submodules and FetchContent? FetchContent can be activated when the directories aren't present.

Before the last update git submodules worked well for the port, regardless of any possible circular depencies.
Circular dependencies are essentially a bug in the repositories that have them. These problems should just be fixed (circular dependencies broken)?

@grojo-ea
Copy link
Contributor

grojo-ea commented Sep 2, 2024

the cmake setup is such that FetchContent just doesn't work, even for tests.

I guess a more specific question is: are the tests build and run as part of the packaging process? What I'm suggesting with using FetchContent only in test projects wouldn't invoke FetchContent unless you invoke CMake explicitly with -DEASTL_BUILD_TESTS=ON (or its equivalent in other libraries)

Circular dependencies are essentially a bug in the repositories that have them. These problems should just be fixed (circular dependencies broken)?

Agreed, that's what we're trying to fix. For some context we don't use CMake nor submodules internally, our test projects use the various libraries liberally but none of the tests projects depend on each other and no library depends on any test projects so there aren't really any circular dependencies, the problem is that we want the library an its corresponding tests in the same repository, and if we add the test's dependencies as submodules in the libray's repository that's where the circular dependencies arise.

@yurivict
Copy link
Author

yurivict commented Sep 2, 2024

Tests aren't built as part of a package, but they are run with a separate target during the port update.
They are run in the same environment so that FetchContent doesn't work during tests.

@grojo-ea
Copy link
Contributor

grojo-ea commented Sep 2, 2024

OK, thanks for clarifying. I'll think about it a bit in case there's an alternative but it sounds like we might have to go back to submodules.

Thanks again for bringing this up.

@2xsaiko
Copy link

2xsaiko commented Oct 3, 2024

FetchContent can work with packaging just fine, it might need some changes (IIRC the first argument to FetchContent_Declare has to match up with the exported target from the dependency). See https://cmake.org/cmake/help/latest/module/FetchContent.html#integrating-with-find-package

@gaaloe
Copy link

gaaloe commented Oct 18, 2024

How am I supposed to interact with the CMake FetchContent when installing EASTL? I can not grasp it, and my work-around is to do a git clone call of EABase.

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

No branches or pull requests

4 participants