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

Fix two CMake errors #16465

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

hageboeck
Copy link
Member

@hageboeck hageboeck commented Sep 18, 2024

  • The first error fixed is an unquoted variable that creates an error in the case that the remote repos of roottest are not called origin. It's relatively common to give the remotes useful names, and those users shouldn't be punished with weird errors.
  • Re-enable FindGLEW. ROOT claims that the latest version of CMake has an error in FindGLEW. In fact, the error was fixed after cmake 3.25, so we can use system GLEW again starting from 3.26

@hageboeck hageboeck self-assigned this Sep 18, 2024
Copy link

github-actions bot commented Sep 18, 2024

Test Results

    18 files      18 suites   4d 10h 46m 8s ⏱️
 2 714 tests  2 707 ✅ 0 💤 7 ❌
46 027 runs  46 020 ✅ 0 💤 7 ❌

For more details on these failures, see this check.

Results for commit dc98856.

♻️ This comment has been updated with latest results.

Copy link
Member

@bellenot bellenot left a comment

Choose a reason for hiding this comment

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

LGTM, if the builds are green 😉

@@ -153,7 +153,7 @@ function(relatedrepo_GetClosestMatch)

# Otherwise, try to use a branch that matches `current_head` in the fork repository
execute_process(COMMAND ${GIT_EXECUTABLE} ls-remote --heads --tags
${__ORIGIN_PREFIX}/${__REPO_NAME} ${current_head} OUTPUT_VARIABLE matching_refs)
${__ORIGIN_PREFIX}/${__REPO_NAME} ${current_head} OUTPUT_VARIABLE matching_refs ERROR_QUIET)
Copy link
Member

Choose a reason for hiding this comment

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

Is that hiding the error (and other maybe relevant errors) rather than fixing it? What is the feedback now for a 'genuine' error?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is hiding the error, indeed. See how my ROOT configures before and after the patch.

--- /tmp/before.log	2024-09-19 14:49:37
+++ /tmp/after.log	2024-09-19 14:47:58
@@ -128,12 +128,6 @@
 -- RapidYAML not found, only compiling RooFit with nlohmann-json parser
 -- 543/896 C++ tutorials have been activated.
 -- 188/242 python tutorials have been activated.
-error: No such remote 'origin'
-fatal: '/roottest' does not appear to be a git repository
-fatal: Could not read from remote repository.
-
-Please make sure you have the correct access rights
-and the repository exists.
 -- Found roottest: /.../roottest
 -- Check for bitness: Found 64 bit architecture.
 -- Scanning subdirectories for tests...
@@ -166,6 +160,6 @@
  - Shared:         
 
 -- Enabled support for:  asimage builtin_clang builtin_cling builtin_freetype builtin_llvm builtin_openui5 builtin_vdt ccache cocoa dataframe davix gdml http imt libcxx opengl pyroot roofit webgui root7 rpath runtime_cxxmodules shared ssl tpython vdt xml xrootd
--- Configuring done (20.8s)
--- Generating done (28.1s)
+-- Configuring done (18.8s)
+-- Generating done (27.2s)

It seems that the feature is useless, since I have been running without whatever it does for years, and the only thing I occasionally have to do is update roottest. So the CMake is obviously written in a way that when those commands fail, everything continue to work.
My idea was therefore to not bother the users with the above error messages that seem to be without any consequence.

Copy link
Member Author

Choose a reason for hiding this comment

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

To be more precise: "useless" for a developer who controls his roottest clone. I imagine it exists for CI runs.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it is essential for the CI and we must see the error if any in this case. I would rather see a completely different change to prevent the call to relatedrepo_GetClosestMatch if we know it will fail for the reason you described.

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 4f5726b45b..bfb0196093 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -696,9 +696,13 @@ if(testing)
        endif()
     endif()
     string(REGEX REPLACE "/root(test)?(\.git)?$" "" originprefix ${originurl})
-    relatedrepo_GetClosestMatch(REPO_NAME roottest
-                                ORIGIN_PREFIX ${originprefix} UPSTREAM_PREFIX ${upstreamprefix}
-                                FETCHURL_VARIABLE roottest_url FETCHREF_VARIABLE roottest_ref)
+    if("${originprefix}" STREQUAL"")
+      message(NOTICE "Could not determine the 'origin' of the roottest repository.  Add remote named `origin` to suppress this message.")
+    else()
+      relatedrepo_GetClosestMatch(REPO_NAME roottest
+                                  ORIGIN_PREFIX ${originprefix} UPSTREAM_PREFIX ${upstreamprefix}
+                                  FETCHURL_VARIABLE roottest_url FETCHREF_VARIABLE roottest_ref)
+    endif()
     # Use `-Droottest_force_checkout=ON` to force fetch and checkout in an existing repository
     if(roottest_force_checkout)
        set(roottest_opts FORCE)

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I can probably write something to not do that call.

…igin.

When roottest is checked out, and none of its remote repos is called
origin, cmake configure fails because of an unquoted empty variable.
Furthermore, the useless error messages are silenced.
For CMake versions between 3.15 and 3.25, FindGLEW had a bug. Here, a
version check for >= 3.25 is added, so GLEW doesn't need to be a
builtin.
Copy link
Member

@pcanal pcanal left a comment

Choose a reason for hiding this comment

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

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.

3 participants