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

Prevent LLVM cmake from finding builtin zstd. #17909

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

Conversation

pcanal
Copy link
Member

@pcanal pcanal commented Mar 6, 2025

  • Fix removal of ROOTSYS from CMAKE_PREFIX_PATH.
    Current code was, if CMAKE_PREFIX_PATH had more than one element, essentially
    inadvertently replacing ROOTSYS by the current path. (eg. if CMAKE_PREFIX_PATH
    contains just : or :: or :somethingelse, it will add the current directory (top level
    of the build directory to the prefix path).

  • Fix find_package disabling for zstd.
    LLVM uses the lower case spelling for 'zstd' and thus find_package was still being called.

In practice this lead my build: -Dbuiltin_zstd=ON -Dbuiltin_xxhash=ON -Dminimal=ON -Dbuiltin_zlib=ON on AlmaLinux release 9.5 (Teal Serval) to pick up the lib/libstd.a for the LLVM libraries and fails to link rootcling_stage1 because of missing xxHash symbols.

This PR also fixes: its.cern.ch/jira/browse/ROOT-10537

This is somewhat related to #16285
and #8633

pcanal added 2 commits March 6, 2025 17:41
Current code was, if CMAKE_PREFIX_PATH had more than one element, essentially
inadvertently replacing ROOTSYS by the current path.
LLVM uses the lower case spelling for 'zstd' and thus find_package was still being called.
@pcanal pcanal requested review from bellenot and hageboeck March 6, 2025 17:53
@pcanal pcanal self-assigned this Mar 6, 2025
@pcanal pcanal added this to the 6.36.00 milestone Mar 6, 2025
@ferdymercury
Copy link
Contributor

Somewhat related: https://its.cern.ch/jira/browse/ROOT-10537

@pcanal
Copy link
Member Author

pcanal commented Mar 6, 2025

You are right. I will fix that issue as part of this PR.

Copy link

github-actions bot commented Mar 6, 2025

Test Results

    18 files      18 suites   4d 6h 21m 54s ⏱️
 2 725 tests  2 719 ✅ 0 💤 6 ❌
47 351 runs  47 344 ✅ 0 💤 7 ❌

For more details on these failures, see this check.

Results for commit 72e5f75.

♻️ This comment has been updated with latest results.

@pcanal
Copy link
Member Author

pcanal commented Mar 6, 2025

Sigh ... But sometimes we seem to want to use the builtin libzstd:

  input_line_1:1:2: fatal error: malformed or corrupted AST file: 'LLVM was not built with LLVM_ENABLE_ZSTD or did not find zstd at build time'
  #include <new>
   ^
  input_line_1:1:2: note: after modifying system headers, please delete the module cache at '/github/home/ROOT-CI/build/lib'

i.e. either libzstd is not installed locally or it is no longer found.

@pcanal
Copy link
Member Author

pcanal commented Mar 6, 2025

Sigh ... But sometimes we seem to want to use the builtin libzstd:

Right ... :( ... Actually the error message is triggered by reading the previously produced pcm files. I.e. since LLVM could not use the builtin and further did not actually run any find_package in that build (both zlib and zstd are built-in) there is no compression done on the pcm ... so it is a bit too bad that we can't have the builtins used by LLVM.
(and the builds are fixed by deleting the existing pcm files)

@pcanal pcanal added the clean build Ask CI to do non-incremental build on PR label Mar 6, 2025
@pcanal pcanal closed this Mar 6, 2025
@pcanal pcanal reopened this Mar 6, 2025
@pcanal pcanal requested a review from dpiparo March 6, 2025 20:37
@hageboeck
Copy link
Member

The cleanup of the prefix path looks reasonable.

Just one clarification regarding the pcms:

so it is a bit too bad that we can't have the builtins used by LLVM. (and the builds are fixed by deleting the existing pcm files)

When LLVM wasn't configured with compression libraries, this still results in functioning pcms, just uncompressed? In that case I think we can go ahead.

@pcanal
Copy link
Member Author

pcanal commented Mar 10, 2025

When LLVM wasn't configured with compression libraries, this still results in functioning pcms, just uncompressed?

That is correct.

In particular only remove full match.  This fixes: https://its.cern.ch/jira/browse/ROOT-10537
which saw:
```
  ...:${ROOTSYS}/lib:${ROOTSYS}/lib64:...
```
being replaced by:
```
  ...::64:...
```

being replaced by:
@pcanal
Copy link
Member Author

pcanal commented Mar 10, 2025

@hageboeck I updated to consistent apply the fix to all 3 variable ... and extend to fix https://its.cern.ch/jira/browse/ROOT-10537

Copy link
Member

@hageboeck hageboeck left a comment

Choose a reason for hiding this comment

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

I updated to consistent apply the fix to all 3 variable

Great, I was going to ask that! 🙂
I think I found a mistake (the replacements should start from the original ENV, not from the temporary variable, which is always empty.
And using a larger regex, one can probably save a few lines.

CMakeLists.txt Outdated
Comment on lines 62 to 64
string(REPLACE ":${_path}:" ":" _temp_envvar "${_temp_envvar}")
string(REGEX REPLACE "^${_path}:" "" _temp_envvar "$ENV{${ENV_VAR}}")
string(REGEX REPLACE ":${_path}$" "" _temp_envvar "${_temp_envvar}")
Copy link
Member

Choose a reason for hiding this comment

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

It looks like the lines were reordered, so the last argument needs to change.
And after the first line dealt with :A:, you should get away with a single regex:

Suggested change
string(REPLACE ":${_path}:" ":" _temp_envvar "${_temp_envvar}")
string(REGEX REPLACE "^${_path}:" "" _temp_envvar "$ENV{${ENV_VAR}}")
string(REGEX REPLACE ":${_path}$" "" _temp_envvar "${_temp_envvar}")
string(REPLACE ":${_path}:" ":" _temp_envvar "$ENV{${ENV_VAR}}")
string(REGEX REPLACE ":?${_path}:?" "" _temp_envvar "${_temp_envvar}")

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 looks like the lines were reordered, so the last argument needs to change.

Indeed, exactly what happeed :(

Copy link
Member Author

Choose a reason for hiding this comment

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

And after the first line dealt with :A:, you should get away with a single regex:

string(REGEX REPLACE ":?${_path}:?" "" _temp_envvar "${_temp_envvar}")

I think they would still wrong partially replace

/some/other/path/${_path}
${_path}64
...:${_path}64:...
etc.

CMakeLists.txt Outdated
Comment on lines 65 to 66
if ("${_temp_envvar}" STREQUAL "${_path}")
set(ENV{ENV_VAR} "")
Copy link
Member

Choose a reason for hiding this comment

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

This should be unnecessary if the above suggestion works as I think it works. The replacement should take care of all of

  • A:
  • :A
  • A

Comment on lines +74 to +75
# if we leave the ':' it will result in an empty entry in the CMAKE_PREFIX_PATH
# which will interpreted as the current directory.
Copy link
Member

Choose a reason for hiding this comment

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

This comment seems outdated now.

Copy link
Member Author

Choose a reason for hiding this comment

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

The comment is maybe misplaced but still relevant. For the other paths, there is no side effect to having :: whereas it is disastrous for CMAKE_PREFIX_PATH.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clean build Ask CI to do non-incremental build on PR in:Build System
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants