Repository navigation
Conversation
Select fallback sources after library detection, retain previously installed component targets, and check required offline sources explicitly. Add local Git regression tests to the CMake CI jobs. Co-authored-by: Codex <noreply@openai.com>
dimpase
left a comment
dimpase
left a comment
There was a problem hiding this comment.
lgtm, @d-torrance
d-torrance
added
build issue
d-torrance
left a comment
d-torrance
left a comment
There was a problem hiding this comment.
Thanks for the contribution! I have a few comments and questions
| @@ -0,0 +1,130 @@ | |||
| """Exercise selective CMake checkout with local Git repositories (no network).""" | |||
There was a problem hiding this comment.
What's the purpose of this python script?
There was a problem hiding this comment.
it's to be run in CI. 🤖 says:
The Python script tests the new submodule-selection logic using temporary local Git repositories,
without network access or compiling M2.
It checks that required submodules are fetched at their pinned revisions,
unused dependencies are skipped, and offline builds and failed updates are handled correctly.
It runs in CI to catch regressions.
There was a problem hiding this comment.
This seems like overkill to check something as simple as "did we check out the right submodules"? Also the cmake/tests directory is new, and we don't have any similar Python scripts anywhere.
I'd recommend swapping this out for a simple shell command in the workflow. Claude suggested the following:
- name: Check unused submodules were not checked out
if: matrix.build-system == 'cmake'
run: |
for m in bdwgc flint frobby givaro fflas_ffpack googletest; do
if git submodule status ../../submodules/$m | grep -qv '^-'; then
echo "::error::submodule $m was checked out"; exit 1
fi
doneThere was a problem hiding this comment.
In fact, we could drop the "if" since the autotools build already has this behavior
Co-authored-by: Codex <noreply@openai.com>
Set the alias before submodule selection and provide it in the standalone test fixture. Co-authored-by: Codex <noreply@openai.com>
There was a problem hiding this comment.
There's already an option to disable cloning submodules, which you could enable on github builds if you'd like. Moving submodule handling all to this file is a good idea, but otherwise the rest of this PR adds unnecessary complexity imo.
|
python stuff with the associated CI step dropped, OK now? |
| EXCLUDE_FROM_ALL ON | ||
| STEP_TARGETS install | ||
| ) | ||
| set(GOOGLETEST_FOUND ${GTEST_FOUND}) |
There was a problem hiding this comment.
Why did you move this line all the way to line 121 as opposed to ~688?
There was a problem hiding this comment.
because GOOGLETEST_FOUND is needed on the line 122, where submodules.make is included. The latter uses it on line 8.
| # https://github.com/ivmai/bdwgc/ | ||
| # TODO: add environment variables GC_LARGE_ALLOC_WARN_INTERVAL and GC_ABORT_ON_LEAK | ||
| # Note: Starting with 8.0, libatomic_ops is not necessary for C11 or C++14. | ||
| if(_m2_build_bdwgc) |
There was a problem hiding this comment.
Why are these checks necessary? Even if we don't need to clone a submodule, or even if git isn't available, we can safely call ExternalProject_Add here, no?
There was a problem hiding this comment.
well, not quite. ExternalProject_Add without a download method requires a nonempty source directory—even if its target won’t be built. Therefore those guards. Cf https://cmake.org/cmake/help/latest/module/ExternalProject.html#download-step-options
There was a problem hiding this comment.
That's why we called file(TOUCH ${_submodule}/.nogit).
There was a problem hiding this comment.
isn't it cleaner just not have directories if there is no stuff there?
(and as submodules are gone here, calling anything nogit is just wrong)
So this just treating the dependencies in question as normal external dependencies, without any old hacks left)
There was a problem hiding this comment.
oops, not all submodules are gone, I am conflating other PRs here
antonleykin
left a comment
antonleykin
left a comment
There was a problem hiding this comment.
I don't understand much of the submodules mechanics, but as long as this PR simplifies things, it is fine to merge.
| set(_m2_required_submodules submodules/memtailor submodules/mathic submodules/mathicgb) | ||
| foreach(_name IN ITEMS bdwgc flint frobby givaro fflas_ffpack googletest) |
There was a problem hiding this comment.
Hardcoding this list here makes maintenance of submodules more mysterious and ad hoc.
| foreach(_path IN LISTS _m2_required_submodules) | ||
| file(GLOB _m2_submodule_files "${CMAKE_SOURCE_DIR}/${_path}/*") | ||
| list(FILTER _m2_submodule_files EXCLUDE REGEX "/\\.(git|nogit)$") |
There was a problem hiding this comment.
What's this block doing? You said you're not using .nogit files.
There was a problem hiding this comment.
Older non-Git configurations may have left .nogit placeholders in empty submodule directories. Neither these nor .git metadata count as source files.
I can add this as a source comment if you like.
This check can be removed some time later.
|
@dimpase could you clarify what is the precise issue that this PR solves, or in what way it simplifies the submodule handling in cmake? |
|
checking out a large submodule takes a while. It's a waste of internet bandwidth and time if it is not needed. |
| foreach(_entry IN LISTS _m2_gitmodule_paths) | ||
| string(REGEX REPLACE "^[ \t]*path[ \t]*=[ \t]*" "" _path "${_entry}") | ||
| string(STRIP "${_path}" _path) | ||
| if(NOT _path MATCHES "^M2/") | ||
| continue() | ||
| endif() | ||
| string(REGEX REPLACE "^M2/" "" _path "${_path}") | ||
|
|
||
| if(_path MATCHES "^submodules/([^/]+)$") | ||
| set(_name "${CMAKE_MATCH_1}") | ||
| string(TOUPPER "${_name}" _upper) | ||
| set(_option_name "${_upper}") | ||
| if(_name STREQUAL "googletest") | ||
| set(_option_name GTEST) | ||
| endif() | ||
| if(NOT _option_name IN_LIST _m2_library_options) | ||
| # Engine sources have no system-library alternative. | ||
| set(_m2_build_${_name} TRUE) | ||
| else() | ||
| _m2_select_fallback("${_name}") | ||
| endif() | ||
| if(_m2_build_${_name}) | ||
| list(APPEND _m2_required_submodules "${_path}") | ||
| list(APPEND _m2_update_submodules "${_path}") | ||
| endif() | ||
| else() | ||
| # Keep the editor submodule's existing warning/skip behavior when absent. | ||
| list(APPEND _m2_update_submodules "${_path}") | ||
| endif() | ||
| endforeach() | ||
| if(NOT _m2_update_submodules) | ||
| message(FATAL_ERROR "No M2 submodule paths found in ${_m2_gitmodules}") | ||
| endif() |
There was a problem hiding this comment.
@antonleykin, I understand exactly how the submodule mechanics works, and genuinely do not think this is a simplification of what we already have. It shaves ~20 seconds off from CMake's configure step, but trying to decipher this block here wasted 20 minutes of my time and I still don't fully understand it. As I see it, merging it makes the system more complicated and dependent on a coding agent for maintenance.
That said, as I said in the other CMake PR, feel free to disregard my review and merge if anyone who wants it can say that they 100% understand it and take responsibility for its maintenance.
|
20 seconds? it's easily takes 20 seconds on a very fast net to check out flint (if your disk is slow it will take more) - which will also take 200Mb of disk. This sort of setup just looks bad in the eyes of potential users. They think that if the devs are too lazy to optimise the build, the quality of the soft isn't great, either... |
|
Here's a GitHub build before this PR, from 12:03:43 -> 12:03:58 (15s): and here is the latest build in this PR 22:24:04 -> 22:24:06 (2s): I'm not opposed to the concept of only fetching the necessary submodules, only that your coding agent did so in an unnecessarily complicated way. I doubt the users will notice either way. Also, I can give many situations in which this PR won't even solve the problem you're suggesting. For instance, if there's a linking error between the libraries (this happens to me regularly if different libraries are linked with different gmp versions), cmake defaults to building all the libraries, so everything submodule will be fetched anyway. |
Report the linker output and selected GMP paths by default instead of discarding every library discovery result. REBUILD_LIBRARIES_ON_LINK_FAILURE restores the previous automatic fallback when requested. Always rerun the probe so corrected paths take effect after a failed configuration.
The child CMake configuration otherwise searches independently and can pick a system GMP even when M2 found GMP under another prefix, such as Linuxbrew. Pass GMP_ROOT to prefer the same installation.
| "Use -DBUILD_LIBRARIES=ALL to explicitly rebuild all supported dependencies.\n\n" | ||
| "To restore automatic rebuilding after a failed link check, configure with " | ||
| "-DREBUILD_LIBRARIES_ON_LINK_FAILURE=ON.\n\n" | ||
| "Link check output:\n${LIBRARY_COMPATIBILITY_OUTPUT}") |
There was a problem hiding this comment.
Did you just pass my gmp library mismatch example to your coding agent?! The example I gave is one of many situations that library incompatibility can happen, and most often the solution is indeed rerunning the build-libraries target, as the message you deleted suggested, and giving a fatal error means the user can't even do that! Moreover, even in the limited gmp version mismatch situation, passing -DBUILD_LIBRARIES=... will not prevent this fatal error, which means it'll be impossible to build M2!
There was a problem hiding this comment.
I haven't deleted anything
There was a problem hiding this comment.
giving a fatal error is not the end. There is an option to explicitly stick to the current behaviour.
Moreover, one of the last commits fix a bug which let frobby to shop around for a gmp, rather than using one found by the cmake builder already
There was a problem hiding this comment.
an incompatibility like this is a bug in the build system. It's not something one is forced to live with forever
There was a problem hiding this comment.
What happened is that not all such cases were checked. Sorry. A learning curve :-(
I started digging why. Apparently, as 🤖 was previously told here not to write tests for cmake files, cf (d2ddc78) it literally replaced tests written in Python it runs on its code with sequences of commands it executed "manually". Not dissimilar to humans here. :-)
Perhaps it's not a good idea with AI-written build systems to keep to old habits of not writing tests...
Keep compatible fallback installations, but retry a failed combined link check without the installed libraries explicitly selected by BUILD_LIBRARIES. Only enable their rebuilds when the remaining libraries link successfully. Invalidate the selected ExternalProject configure/build/install/test stamps so build-libraries repairs the installation. A successful post-build probe retains it, avoiding repeated rebuilds with the same cached option.
|
perhaps the bottom line here is that software in submodules become a problem for the build system as soon as they start competing with their system-wide analogs; while it's possible to have "rebuild everything from scratch" mode working OK, anything beyond this is a major project, needing testing of the build system itself, etc. perhaps we just need a carpet "ignore submodules" option in cmake to have a chance for reliable quick rebuilds. |
CMake currently initializes every Git submodule during configuration, even when it finds the corresponding system libraries. For example, a system FLINT installation avoids compiling FLINT but still causes its repository to be downloaded. This adds unnecessary work on slow connections and in fresh CI checkouts.
Select submodules after library detection and
BUILD_LIBRARIESoverrides. Always retain the bundled engine sources and Emacs package; initialize BDWGC, FLINT, Frobby, Givaro, FFLAS-FFPACK, and GoogleTest only when their fallback is needed. Declare those fallback build rules only when selected, so empty unused source directories no longer prevent configuration. GoogleTest is unnecessary when testing is disabled.Previously installed fallback components retain their sources and build/test targets across reconfiguration, using their installation stamps.
GIT_SUBMODULE=OFFand source archives remain supported when required sources are present; missing required sources now produce an actionable error. Remove the old.nogitplaceholder workaround. Documentation on submodule selection,BUILD_LIBRARIES, and offline builds is staged in the draft wiki pages on dimpase/M2. These update the CMake build guide and related FAQ; they will be transferred to the upstream wiki after this PR is merged, incorporating any further review changes. The added README documentation has been removed from this PR.A failed combined library link check now stops configuration with the linker output, selected GMP headers/libraries, and
CMAKE_PREFIX_PATH, so it does not silently select broad fallback builds. Configure with-DREBUILD_LIBRARIES_ON_LINK_FAILURE=ONto restore the previous automatic fallback;BUILD_LIBRARIESremains available for explicitly selecting dependencies to rebuild. The probe is retried on every configuration so corrected paths take effect after a failure.Frobby's fallback configuration now receives the selected
GMP_ROOT, avoiding an independent default to system GMP when M2 selected a different prefix, such as Linuxbrew.Validation:
GMP_ROOTselects the same GMP prefix as the parent configuration.BUILD_LIBRARIES=FLINTcreates the fallback targets; a subsequent configuration with a simulated installation stamp retains its test target.actionlint -shellcheck='') and diff checks pass. Existing ShellCheck warnings in unchanged workflow commands remain. Local checks ran at low priority. No full M2 rebuild was performed.The development CMake workflow already starts with a non-recursive checkout and can benefit directly. The recursive checkout/source-snapshot handling in #4721 needs a separate follow-up to benefit from this selection as well. No network-speed improvement has been benchmarked.
🤖 gpt-6-astra worked on this 🤖
Review follow-up: removed
M2/cmake/tests/test_submodules.pyand the corresponding CMake CI step. The selective-checkout implementation is unchanged. Workflow syntax/expression validation passes with ShellCheck disabled (unchanged shell commands have existing warnings), and the removal passes diff checks.