CMake cleanups and RPM-build fixing#1018
Conversation
60d295d to
9492d66
Compare
|
I support the intent; the implementation is questionable. Unless the dependency can be done with a pkg-config or a single CMake module, the entire detection should be in a file that is separated from |
|
I think, |
|
Though I agree it's the right way forward to avoid |
|
Fetch content is not the issue. We can avoid it and I would also be keen not to rely on it. The curent status breaks so many CMake idioms and constructs that it will become a serious pain point very soon.
If libxs and libxstream have a cmake build system then we should use it as it simplify things enormously. No custom findlibxs.cmzke etc... Less maintenance work.
I will help with fixing the remaining issues in dbcsr and open a PR on his branch so that work is not duplicated. It will have to wait tomorrow.
Sent from [Proton Mail](https://proton.me/mail/home) for Android.
…-------- Original Message --------
On Sunday, 06/07/26 at 15:29 SY Wang ***@***.***> wrote:
Growl1234 left a comment [(cp2k/dbcsr#1018)](#1018 (comment))
Though I agree it's the right way forward to avoid FetchContent in cmake, I'm a bit hesitate here. ***@***.***(https://github.com/hfp) what do you think?
—
Reply to this email directly, [view it on GitHub](#1018?email_source=notifications&email_token=AHAE5JJ3EAWDRBWGQX5C7ED46VU2JA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTINRUGI4DCMJXHA4KM4TFMFZW63VHMNXW23LFNZ2KKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#issuecomment-4642811788), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AHAE5JOFURRRUPPYGA374R346VU2JAVCNFSM6AAAAACZ5MAQNCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DMNBSHAYTCNZYHA).
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for [iOS](https://github.com/notifications/mobile/ios/AHAE5JMWDWUZCL2EZLDKMJT46VU2JA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTINRUGI4DCMJXHA4KM4TFMFZW63VHMNXW23LFNZ2KKZLWMVXHJKTGN5XXIZLSL5UW64Y) and [Android](https://github.com/notifications/mobile/android/AHAE5JI3SQBMPPWUA2UV7QD46VU2JA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTINRUGI4DCMJXHA4KM4TFMFZW63VHMNXW23LFNZ2KKZLWMVXHJLTGN5XXIZLSL5QW4ZDSN5UWI). Download it today!
You are receiving this because you commented.Message ID: ***@***.***>
|
|
Fetching content is part of a chain and the last resort/fallback and it's only if the library in question is requested and not picked-up differently. |
True, the question is more do we want to do this or not. I agree with @oschuett that, while convenient, it mixes two different things,
So it is a great functionality for json, putty_xml or any small libraries that have no options and no dependencies. It becomes more difficult when other dependencies should be built as well. |
|
I will work on this PR for the next hours or so. fetch_content is breaking already. |
I've added a |
|
we can apply the same treatment here than on CP2K. |
|
Yeah, but this currently does not affect CP2K CI and I would like to expect a success report there first.
…---Original---
From: "Taillefumier ***@***.***>
Date: Mon, Jun 8, 2026 18:18 PM
To: ***@***.***>;
Cc: "SY ***@***.******@***.***>;
Subject: Re: [cp2k/dbcsr] Fix libxs* discovery and revise options (CMake); fix RPM-build error (PR #1018)
mtaillefumier left a comment (cp2k/dbcsr#1018)
we can apply the same treatment here than on CP2K.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
|
And it seems the CMake path on the libxsmm side still needs to have a look... I'll soon come back to this. |
|
I will pull-in this PR or take over if you like? I had/have my changes in preparation as well. |
|
Could you make these changes available in a separate branch so wee can cherry pick accordingly. We should use cmake to build libxs and libxstream as it will remove two findpackage.cmake automatically. To be clear, do not use fetch_content inside a |
|
@hfp: I need your input on the dependency model of
@Growl1234 and I will take care of the CMake build system (I will, in any case). However being able to consult your own modifications will certainly help because we may miss important details. |
|
Thank you for taking care of CMake build system! Removing
You could have a look at libxsmm/libxsmm#1015 to see if improvements needed. |
273f78e to
7be9fd3
Compare
|
I am a bit puzzled about how this crosses. My idea would be to take those changes here and rework on top of mine. Once my PR is taken we can cooperatively revise it further. Would this work for you? |
Either way would work for me. Please feel free to proceed as you prefer. |
232b86d to
b72b8d5
Compare
This failure looks like an ABI mismatch between libxs and libxsmm... again the concerns we talked before... I will revert to hfp/libxs@89dc404 in GitHub CI for now, which remains the warning existing but avoids the runtime error. |
Do you have the test's output? I can see your last change passed (this is current state like green): If you change to LIBXS ccfb2b6069ba9bad033a550905cb032b9a7346f1 (latest), it should pass. I think you never tested this. I cannot find any commit that went through CI testing "ccfb2b6069ba9bad033a550905cb032b9a7346f1"? |
|
Sorry, I did it through force-push so the results are not preserved here. I can reuse the latest commit. It passed on building but failed on testing after build |
|
It passes... so maybe a false positive? Here's the previous message: |
Thanks for sharing! If it a transient failure it points towards a race condition. There is nothing pending in this direction and things are thoroughly tested in LIBXS. It can be also on the DBCSR side. Once we have this PR merged, there will be at least one more to bring-in released version instead of SHAs. Thanks for trying! |
I am affiliated - true, but only to mention "MKL" as an optional thing (if it helps or if it's applicable - fine). In fact, I did everything to leave out those product stuff not to mention that using LIBXSMM kernels will help on Alps or ARM in general (I hope!). I am interested in foundational things to perhaps advance science. So, LIBXS will play-out for actual science going forward. I will have some talk at CP2K's 25th anniversary about it. I have no idea what "InfinityHub" is, and wrt Spack - I like the approach and I am contributing to it. I am sorry, it's not my expertise to fit your CMake taste. LLMs took over (update for you) and so it will with all this "CMake religion" and Spack too. This PR reworked DBCSR's CMake with pieces unrelated to my work. I only feel blamed for my contribution. I never expected my contribution to CP2K/DBCSR to be fully transactional or atomic. My and your nice work here in this PR is only normal like making this work and I expect it (not necessarily a "weekend call" ;-). Over the course of this PR (and the CP2K one), I was almost excluded from my responsibility to "fix the things I broke". That's unexpected to me. To wrap up the current/merged state of DBCSR develop: it passed all tests as one can still see. I knew the OpenCL test etc needed a 2nd PR but that was/is (still) true.
I think we do not stop science because of CMake taste (see above). |
|
I think, we are not intending to blame the libxs contribution or question its scientific value. The concern is only about the downstream integration contract. This is not "A vs B"; it's "A and B both matter"... |
|
This is not a matter of “CMake taste”. Modern CMake projects across the ecosystem follow broadly similar patterns because that is what enables maintainability, reproducibility, and straightforward reuse in downstream projects and package managers. The PR in question was merged into develop without review and introduced changes that effectively require downstreams to mirror internal details of a dependency in order to keep existing builds working. Saying it is “standard practice” for a dependency to leak its internal implementation details into every project that uses it would need concrete evidence; my experience with CP2K, Spack and other HPC codes suggests the opposite. A clean CMake build system is not an aesthetic preference. It has a very real maintenance cost and impact for everyone involved: developers, maintainers, vendors, and packagers. Poorly defined dependencies and ad‑hoc integration work translate directly into extra time spent fixing toolchains, CI, and containers instead of improving the code or the science. Advancing science and following sound engineering practices are not in conflict, but they are also not the same thing. I also publish scientific work, so my concern here is not about slowing down science, but about basic engineering discipline around dependencies and releases so that a central library like DBCSR remains usable and stable for its downstream users |
I fully agree with this. |
|
I may send another PR regarding LIBXSMM and cleanup. It might be wise to still have a fallback to PkgConfig. |
alazzaro
left a comment
There was a problem hiding this comment.
I went through most of the changes, thanks to all for cleaning up and making it work again!
Motivation: cp2k/cp2k#5343 and cp2k/cp2k#5365
This PR makes the LIBXS/LIBXSMM/LIBXSTREAM handling more robust and less dependent on explicit ROOT variables or sibling source-tree layouts.
The dependency discovery now first tries pkg-config and standard CMake search paths, and only falls back to FetchContent when no local installation is found. This avoids unnecessary downloads when prebuilt libraries are already available. The
*_ROOTvariables are no longer used.It also introduces stable wrapper targets for LIBXS and LIBXSTREAM, updates the OpenCL backend to use these targets and the detected LIBXSTREAM paths, and fixes the exported DBCSR CMake package so downstream users can resolve the required dependencies.
In addition, the default options are made more conservative: MPI, LIBXS, and LIBXSMM are no longer enabled by default. Users now need to enable them explicitly when needed, which avoids confusion in a default installation and follows the usual opt-in convention for optional dependencies. Also moves all option description to the
# OPTIONSsection.The RPM-build is also fixed through: initializing tensor-test vectors directly to avoid GCC 16 nonnull warnings from initializer-list assignment that RPM-build treats as error.