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.

Sign up for GitHub

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

Jump to bottom

Fix two CMake errors #16465

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

Fix two CMake errors #16465

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
098de2b
[CMake] Prevent a CMake error when roottest's remote is not called or…
hageboeck Feb 26, 2024
dc98856
[CMake] Re-enable FindGLEW on APPLE.
hageboeck Jul 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,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.

if(NOT "${matching_refs}" STREQUAL "")
set(${__FETCHURL_VARIABLE} ${__ORIGIN_PREFIX}/${__REPO_NAME} PARENT_SCOPE)
return()
Expand Down Expand Up @@ -641,8 +641,12 @@ if(testing)
endif()
if(DEFINED repo_dir)
execute_process(COMMAND ${GIT_EXECUTABLE} --git-dir=${repo_dir}/.git
remote get-url origin OUTPUT_VARIABLE originurl OUTPUT_STRIP_TRAILING_WHITESPACE)

remote get-url origin OUTPUT_VARIABLE originurl OUTPUT_STRIP_TRAILING_WHITESPACE
RESULT_VARIABLE query_result
ERROR_VARIABLE query_error)
if(NOT query_result EQUAL 0)
message(STATUS "Searching for \"origin\" repo of roottest: ${query_error}")
endif()
else()
# The fetch URL of the 'origin' remote is used to determine the prefix for other repositories by
# removing the `/root(\.git)?` part. If `GITHUB_PR_ORIGIN` is defined in the environment, its
Expand All @@ -654,7 +658,7 @@ if(testing)
remote get-url origin OUTPUT_VARIABLE originurl OUTPUT_STRIP_TRAILING_WHITESPACE)
endif()
endif()
string(REGEX REPLACE "/root(test)?(\.git)?$" "" originprefix ${originurl})
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)
Expand Down
10 changes: 5 additions & 5 deletions cmake/modules/SearchInstalledSoftware.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -650,11 +650,11 @@ if((opengl OR cocoa) AND NOT builtin_glew)
find_package(GLEW REQUIRED)
else()
find_package(GLEW)
# Bug was reported on newer version of CMake on Mac OS X:
# https://gitlab.kitware.com/cmake/cmake/-/issues/19662
# https://github.com/microsoft/vcpkg/pull/7967
if(GLEW_FOUND AND APPLE AND CMAKE_VERSION VERSION_GREATER 3.15)
message(FATAL_ERROR "Please enable builtin Glew due bug in latest CMake (use cmake option -Dbuiltin_glew=ON).")
if(GLEW_FOUND AND APPLE AND CMAKE_VERSION VERSION_GREATER 3.15 AND CMAKE_VERSION VERSION_LESS 3.25)
# Bug in CMake on Mac OS X until 3.25:
# https://gitlab.kitware.com/cmake/cmake/-/issues/19662
# https://github.com/microsoft/vcpkg/pull/7967
message(FATAL_ERROR "Please enable builtin Glew due a bug in CMake's FindGlew < v3.25 (use cmake option -Dbuiltin_glew=ON).")
unset(GLEW_FOUND)
elseif(GLEW_FOUND AND NOT TARGET GLEW::GLEW)
add_library(GLEW::GLEW UNKNOWN IMPORTED)
Expand Down
Loading