Use submodules for some dependencies. #15195

aarlt · 2024-06-12T12:37:55Z

clonker · 2024-06-14T08:16:35Z

cmake/fmtlib.cmake

-else()
-	FetchContent_MakeAvailable(fmtlib)
-endif()
+add_subdirectory(${CMAKE_SOURCE_DIR}/deps/fmt)


you could add a check whether the submodule(s) have been initialized and otherwise issue a cmake config error

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.

@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.

r0qs · 2024-06-14T10:35:54Z

We also need to update the build scripts, like build.sh. But I'm wondering why not just add the git submodule update --init in the CI build script and consequently all CI steps that build the code would already setup the submodules in the beginning of the build process.

r0qs · 2024-06-14T10:44:38Z

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.

scripts/install_evmone.ps1

scripts/create_source_tarball.sh

.gitignore

ekpyron · 2024-06-24T09:54:27Z

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)

aarlt · 2024-07-01T10:13:23Z

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.

I think this was a really good idea! It simplified this PR a lot, we don't need to initialize the submodules explicitly anymore.

aarlt · 2024-07-04T10:01:15Z

CMakeLists.txt

@@ -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)


@ekpyron had the idea to only implicitly initialise the submodules, if they where not yet initialised. I think that makes a lot of sense.

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.

true! that is better!

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 :-).

cameel

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.

scripts/install_evmone.ps1

CMakeLists.txt

cameel · 2024-07-11T17:37:34Z

cmake/fmtlib.cmake

-else()
-	FetchContent_MakeAvailable(fmtlib)
-endif()
+add_subdirectory(${CMAKE_SOURCE_DIR}/deps/fmt)


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.

cmake/nlohmann-json.cmake

.gitmodules

cmake/range-v3.cmake

scripts/create_source_tarball.sh

aarlt · 2024-07-17T12:06:18Z

Ok, I just tested the resulting source archive with homebrew:
The build of the old version failed as expected:

==> Fetching solidity
==> Downloading https://github.com/ethereum/solidity/releases/download/v0.8.25/solidity_0.8.25.tar.gz
==> Downloading from https://objects.githubusercontent.com/github-production-release-asset-2e65be/40892817/eb9d3485-38fd-4b46-8736-508fef9fb1af?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=releaseassetproduction%2F20240717%2Fu
################################################################################################################################################################################################################################ 100.0%
==> cmake ..
Last 15 lines from /Users/alex/Library/Logs/Homebrew/solidity/01.cmake:
-- Found Boost::program_options at
-- Found Boost::system at
CMake Error at /opt/homebrew/Library/Homebrew/cmake/trap_fetchcontent_provider.cmake:12 (message):
  Refusing to populate dependency 'fmtlib' with FetchContent while building
  in Homebrew, please use a formula dependency or add a resource to the
  formula.
Call Stack (most recent call first):
  /opt/homebrew/Cellar/cmake/3.30.0/share/cmake/Modules/FetchContent.cmake:2468:EVAL:1 (trap_fetchcontent_provider)
  /opt/homebrew/Cellar/cmake/3.30.0/share/cmake/Modules/FetchContent.cmake:2468 (cmake_language)
  /opt/homebrew/Cellar/cmake/3.30.0/share/cmake/Modules/FetchContent.cmake:2314 (__FetchContent_MakeAvailable_eval_code)
  cmake/fmtlib.cmake:19 (FetchContent_MakeAvailable)
  CMakeLists.txt:46 (include)


-- Configuring incomplete, errors occurred!
[...]

The new source archive seem to work as expected (no warning regarding FetchContent) However, I'm not able to build it on my machine - it looks like that boost does not contain arm64 for some reason. Not sure why.

==> Fetching solidity
==> Downloading file:///tmp/solidity_0.8.27-nightly-2024-07-16-a4d611be.tar.gz
Already downloaded: /Users/alex/Library/Caches/Homebrew/downloads/e143f92127cf47889e18123dcf7a0e82aae2c0cf2f03fbccb7db6fb2dfcf19cc--solidity_0.8.27-nightly-2024-07-16-a4d611be.tar.gz
==> cmake ..
==> make install
Last 15 lines from /Users/alex/Library/Logs/Homebrew/solidity/02.make:
  "boost::filesystem::detail::canonical(boost::filesystem::path const&, boost::filesystem::path const&, boost::system::error_code*)", referenced from:
      solidity::test::isValidSemanticTestPath(boost::filesystem::path const&) in Common.cpp.o
      solidity::frontend::test::CommandLineInterfaceTest::cli_input::test_method() in CommandLineInterface.cpp.o
      solidity::frontend::test::CommandLineInterfaceTest::cli_input::test_method() in CommandLineInterface.cpp.o
      solidity::frontend::test::CommandLineInterfaceTest::cli_ignore_missing_some_files_exist::test_method() in CommandLineInterface.cpp.o
      solidity::frontend::test::CommandLineInterfaceTest::cli_paths_to_source_unit_names_no_base_path::test_method() in CommandLineInterface.cpp.o
      solidity::frontend::test::CommandLineInterfaceTest::cli_paths_to_source_unit_names_no_base_path::test_method() in CommandLineInterface.cpp.o
      solidity::frontend::test::CommandLineInterfaceTest::cli_paths_to_source_unit_names_base_path_same_as_work_dir::test_method() in CommandLineInterface.cpp.o
      solidity::frontend::test::CommandLineInterfaceTest::cli_paths_to_source_unit_names_base_path_same_as_work_dir::test_method() in CommandLineInterface.cpp.o
      ...
ld: symbol(s) not found for architecture arm64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make[2]: *** [test/soltest] Error 1
make[1]: *** [test/CMakeFiles/soltest.dir/all] Error 2
make: *** [all] Error 2

However, I guess we could just ignore that. Building manually from source is working without any problems.

aarlt · 2024-07-17T12:50:29Z

Ok, I found the problem: I had two conflicting boost versions installed. After removing the "old" version, everything works as expected:

==> Fetching solidity
==> Downloading file:///tmp/solidity_0.8.27-nightly-2024-07-16-a4d611be.tar.gz
Already downloaded: /Users/alex/Library/Caches/Homebrew/downloads/e143f92127cf47889e18123dcf7a0e82aae2c0cf2f03fbccb7db6fb2dfcf19cc--solidity_0.8.27-nightly-2024-07-16-a4d611be.tar.gz
==> cmake ..
==> make install
🍺  /opt/homebrew/Cellar/solidity/2024-07-16: 8 files, 45.0MB, built in 1 minute 47 seconds
==> Running `brew cleanup solidity`...
Disable this behaviour by setting HOMEBREW_NO_INSTALL_CLEANUP.
Hide these hints with HOMEBREW_NO_ENV_HINTS (see `man brew`).

clonker

given the source archive works now it LGTM

cameel

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/range-v3.cmake

cameel · 2024-07-24T19:11:31Z

cmake/EthDependencies.cmake

+				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.")


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:

Suggested change

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.")

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(...) ...

cameel · 2024-07-24T19:53:54Z

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.

cmake/nlohmann-json.cmake

clonker · 2024-07-30T07:24:20Z

cmake/EthDependencies.cmake

+		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'."


"just a reminder to potentially update" but a FATAL_ERROR message :D

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.

cameel

#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.

cameel · 2024-08-01T20:57:03Z

cmake/EthDependencies.cmake

+		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'."


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.

cmake/fmtlib.cmake

clonker

checked that solc-js still works with these changes, ekpyron checked that ppa still does its thing, code LGTM

aarlt force-pushed the some_dependencies_as_git_submodules branch 4 times, most recently from 23c86c9 to 4ee9a65 Compare June 12, 2024 21:25

aarlt requested a review from cameel June 12, 2024 22:18

aarlt added the build system 🏗️ label Jun 13, 2024

clonker reviewed Jun 14, 2024

View reviewed changes

cameel reviewed Jun 14, 2024

View reviewed changes

scripts/install_evmone.ps1 Outdated Show resolved Hide resolved

ekpyron reviewed Jun 24, 2024

View reviewed changes

scripts/create_source_tarball.sh Outdated Show resolved Hide resolved

ekpyron reviewed Jun 24, 2024

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

aarlt force-pushed the some_dependencies_as_git_submodules branch 3 times, most recently from e917a87 to 9701a3e Compare June 26, 2024 15:21

aarlt force-pushed the some_dependencies_as_git_submodules branch from 9701a3e to c7c6728 Compare July 2, 2024 15:33

aarlt commented Jul 4, 2024

View reviewed changes

aarlt force-pushed the some_dependencies_as_git_submodules branch from c7c6728 to 6140cb9 Compare July 4, 2024 14:44

cameel reviewed Jul 11, 2024

View reviewed changes

aarlt force-pushed the some_dependencies_as_git_submodules branch 4 times, most recently from 08bfd00 to a4d611b Compare July 16, 2024 10:37

clonker previously approved these changes Jul 24, 2024

View reviewed changes

cameel previously approved these changes Jul 24, 2024

View reviewed changes

aarlt dismissed stale reviews from cameel and clonker via 5e49262 July 29, 2024 11:38

aarlt force-pushed the some_dependencies_as_git_submodules branch 8 times, most recently from 3b5d225 to 33ceccf Compare July 29, 2024 16:56

aarlt mentioned this pull request Jul 29, 2024

Update fmtlib to v11.0.2. #15305

Merged

clonker reviewed Jul 30, 2024

View reviewed changes

cmake/nlohmann-json.cmake Show resolved Hide resolved

clonker reviewed Jul 30, 2024

View reviewed changes

aarlt force-pushed the some_dependencies_as_git_submodules branch 2 times, most recently from 7f9ac79 to 24dbe1c Compare July 30, 2024 19:01

cameel previously approved these changes Aug 1, 2024

View reviewed changes

aarlt dismissed cameel’s stale review via efede0b August 12, 2024 09:11

aarlt force-pushed the some_dependencies_as_git_submodules branch 3 times, most recently from 24ce304 to 4ecabbc Compare August 12, 2024 09:14

aarlt requested a review from clonker August 12, 2024 09:19

clonker reviewed Aug 13, 2024

View reviewed changes

cmake/fmtlib.cmake Outdated Show resolved Hide resolved

Use submodules for some dependencies.

63d87a2

aarlt force-pushed the some_dependencies_as_git_submodules branch from 4ecabbc to 63d87a2 Compare August 13, 2024 12:01

clonker approved these changes Aug 14, 2024

View reviewed changes

ekpyron added the 🟡 PR review label label Aug 15, 2024

aarlt merged commit c7adde3 into develop Aug 19, 2024
72 checks passed

aarlt deleted the some_dependencies_as_git_submodules branch August 19, 2024 09:15

r0qs mentioned this pull request Aug 21, 2024

Recursively clone solidity repository on solc-bin nightly builds. #15350

Closed

cameel mentioned this pull request Aug 30, 2024

Tweak advanced CMake options #15377

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use submodules for some dependencies. #15195

Use submodules for some dependencies. #15195

aarlt commented Jun 12, 2024 •

edited

Loading

clonker Jun 14, 2024

cameel Jul 11, 2024

cameel Jul 24, 2024

r0qs commented Jun 14, 2024

r0qs commented Jun 14, 2024

ekpyron commented Jun 24, 2024 •

edited

Loading

aarlt commented Jul 1, 2024

aarlt Jul 4, 2024

clonker Jul 4, 2024

aarlt Jul 4, 2024

ekpyron Jul 17, 2024

cameel left a comment

cameel Jul 11, 2024

aarlt commented Jul 17, 2024 •

edited

Loading

aarlt commented Jul 17, 2024

clonker left a comment

cameel left a comment

cameel Jul 24, 2024

cameel Jul 24, 2024

cameel commented Jul 24, 2024

clonker Jul 30, 2024

cameel Aug 1, 2024

cameel left a comment •

edited

Loading

cameel Aug 1, 2024

clonker left a comment

	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.")

Use submodules for some dependencies. #15195

Use submodules for some dependencies. #15195

Conversation

aarlt commented Jun 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r0qs commented Jun 14, 2024

r0qs commented Jun 14, 2024

ekpyron commented Jun 24, 2024 • edited Loading

aarlt commented Jul 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cameel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aarlt commented Jul 17, 2024 • edited Loading

aarlt commented Jul 17, 2024

clonker left a comment

Choose a reason for hiding this comment

cameel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cameel commented Jul 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cameel left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clonker left a comment

Choose a reason for hiding this comment

aarlt commented Jun 12, 2024 •

edited

Loading

ekpyron commented Jun 24, 2024 •

edited

Loading

aarlt commented Jul 17, 2024 •

edited

Loading

cameel left a comment •

edited

Loading