-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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 submodules for some dependencies. #15195
Conversation
23c86c9
to
4ee9a65
Compare
cmake/fmtlib.cmake
Outdated
else() | ||
FetchContent_MakeAvailable(fmtlib) | ||
endif() | ||
add_subdirectory(${CMAKE_SOURCE_DIR}/deps/fmt) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you could add a check whether the submodule(s) have been initialized and otherwise issue a cmake config error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The check has now been added in the main CMakeFiles.txt
, but I think that it might be better done as a macro that we'd execute separately for each submodule in its .cmake
file. This would be more robust against adding new submodules in the future - the current check assumes that all submodules are initialized when fmt
is, but that won't be true in such a situation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aarlt What about this one? They way you have it now (i.e. having a non-empty check for every module) probably solves the problem of supporting the addition of new modules (assuming that git submodule update --init
will not fail when some submodules are initialized and some are not), but I still think that initializing each submodule in its .cmake
file would be a better way to organize things. It would put everything related to a specific dependency together and also we would not need to repeat the USE_SYSTEM_LIBRARIES
check.
We also need to update the build scripts, like build.sh. But I'm wondering why not just add the |
Actually, maybe even better, I think we could do all in cmake using execute_process, something like: find_package(Git)
if(Git_FOUND)
execute_process(COMMAND git submodule update --init ...)
...
endif() And also add the error message as suggested by @clonker if the submodules are not initialized correctly. |
At least scripts/release_ppa.sh will also need adjustment - although we can do that separately, we'll have to confirm that that works with a prerelease PPA build - but we shouldn't forget in any case. Might be worthwhile to check if any other release build mechanisms also need adjustments, resp. to confirm that they work (like docker builds and such) |
e917a87
to
9701a3e
Compare
I think this was a really good idea! It simplified this PR a lot, we don't need to initialize the submodules explicitly anymore. |
9701a3e
to
c7c6728
Compare
CMakeLists.txt
Outdated
@@ -20,6 +20,25 @@ include(EthToolchains) | |||
include(EthPolicy) | |||
eth_policy() | |||
|
|||
set(SUBMODULE_CONTENTS "${CMAKE_SOURCE_DIR}/deps/fmt") | |||
file(GLOB SUBMODULE_CONTENTS "${SUBMODULE_CONTENTS}/*") | |||
if (SUBMODULE_CONTENTS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ekpyron had the idea to only implicitly initialise the submodules, if they where not yet initialised. I think that makes a lot of sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd still recommend to check for the .git
file in the submodules' subdirectories instead of checking if there's anything in there. It will also only be created if git submodule update
is run, the init
step itself doesn't place it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true! that is better!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to be careful here, though, to make sure that this works in the packaged source archives (e.g. our source archives currently don't contain a toplevel .git
file).
I.e. we have to double-check that builds from the tarball created by scripts/create_source_tarball.sh
behave sanely.
Ah, I just see that #15195 (comment) already brought that up :-).
c7c6728
to
6140cb9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some things that need to be adjusted but the overall approach seems sound. It's simpler than I thought actually. I assumed we'd need to check out the submodules at every step, but apparently having CMake do this is enough.
This should be marked as fixing #15137.
Do we need a changelog entry? I guess this should be mostly transparent to users, except for possibly leaving the releases we used to download as trash under /deps/
?
Also, please make sure you manually test building from the source archive and a Homebrew build before we merge this. For the former, having #12900 would have been really handy.
cmake/fmtlib.cmake
Outdated
else() | ||
FetchContent_MakeAvailable(fmtlib) | ||
endif() | ||
add_subdirectory(${CMAKE_SOURCE_DIR}/deps/fmt) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The check has now been added in the main CMakeFiles.txt
, but I think that it might be better done as a macro that we'd execute separately for each submodule in its .cmake
file. This would be more robust against adding new submodules in the future - the current check assumes that all submodules are initialized when fmt
is, but that won't be true in such a situation.
08bfd00
to
a4d611b
Compare
Ok, I just tested the resulting source archive with homebrew:
The new source archive seem to work as expected (no warning regarding
However, I guess we could just ignore that. Building manually from source is working without any problems. |
Ok, I found the problem: I had two conflicting boost versions installed. After removing the "old" version, everything works as expected:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
given the source archive works now it LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a bit of cleanup I think this PR should still get before we merge it (see my comments), but everything actually important has been fixed and the logic seems correct so I'm already giving my general approval.
cmake/EthDependencies.cmake
Outdated
message(FATAL_ERROR "Failed to initialize submodules: 'git submodule update --init' failed.") | ||
endif() | ||
else() | ||
message(FATAL_ERROR "Failed to initialize submodules: error executing 'git submodule update --init': git was not found.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, if this happens, you didn't really run the command so it's irrelevant. Only the fact that git is not installed matters:
message(FATAL_ERROR "Failed to initialize submodules: error executing 'git submodule update --init': git was not found.") | |
message(FATAL_ERROR "Failed to initialize submodules: 'git' command not found.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I'd generally recommend dealing with special cases first since it requires less nesting and makes flow clearer:
if (NOT Git_FOUND)
message(FATAL_ERROR ...)
endif()
execute_process(...)
...
Oh, one more thing. Once we merge this, it would be good to have a follow-up PR with an update to latest versions of these libraries. Especially range-v3 seems to be already at 11.x, while we're still on 9.x. |
3b5d225
to
33ceccf
Compare
cmake/EthDependencies.cmake
Outdated
if (NOT ${potential_submodule_count} EQUAL ${expected_potential_submodule_count}) | ||
message(FATAL_ERROR | ||
"There are more directories (potential submodules) found (${potential_submodule_count}) in '${CMAKE_SOURCE_DIR}/deps' than expected (${expected_potential_submodule_count}). " | ||
"This is just a reminder to potentially update e.g. '${CMAKE_SOURCE_DIR}/cmake/EthDependencies.cmake'." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"just a reminder to potentially update" but a FATAL_ERROR
message :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, for this to be actually useful, a FATAL_ERROR
seemed like a better choice. Otherwise I'm not sure anyone will notice this warning. It's just the message did not really fit sucn an error. IMO it should have just been reworded, not changed into warning :)
But I also think we'd be fine without this check. It's a lot of code for an unlikely case where someone forgets to commit a new submodule, which they'll immediately notice due to CI failures and fix.
What I was more worried about earlier was instead a situation where someone properly adds and pushes a module but then everyone who pulls that code gets an error because cmake assumes that it's already initialized because it only checks fmtlib
. But you already fixed that by extending the if
. The warning wasn't really necessary.
7f9ac79
to
24dbe1c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#15195 (comment) and #15195 (comment) (the top one) were still not addressed or at least answered.
But anyway, I'm approving again since those don't strongly impact the functionality, that's just cleanup.
cmake/EthDependencies.cmake
Outdated
if (NOT ${potential_submodule_count} EQUAL ${expected_potential_submodule_count}) | ||
message(FATAL_ERROR | ||
"There are more directories (potential submodules) found (${potential_submodule_count}) in '${CMAKE_SOURCE_DIR}/deps' than expected (${expected_potential_submodule_count}). " | ||
"This is just a reminder to potentially update e.g. '${CMAKE_SOURCE_DIR}/cmake/EthDependencies.cmake'." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, for this to be actually useful, a FATAL_ERROR
seemed like a better choice. Otherwise I'm not sure anyone will notice this warning. It's just the message did not really fit sucn an error. IMO it should have just been reworded, not changed into warning :)
But I also think we'd be fine without this check. It's a lot of code for an unlikely case where someone forgets to commit a new submodule, which they'll immediately notice due to CI failures and fix.
What I was more worried about earlier was instead a situation where someone properly adds and pushes a module but then everyone who pulls that code gets an error because cmake assumes that it's already initialized because it only checks fmtlib
. But you already fixed that by extending the if
. The warning wasn't really necessary.
24ce304
to
4ecabbc
Compare
4ecabbc
to
63d87a2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
checked that solc-js
still works with these changes, ekpyron checked that ppa still does its thing, code LGTM
Fixes #15137.