[rootcling] Remove llvm::cl dependency by using ROOT's optparse#20903
[rootcling] Remove llvm::cl dependency by using ROOT's optparse#20903devajithvs wants to merge 2 commits into
llvm::cl dependency by using ROOT's optparse#20903Conversation
Test Results 20 files 20 suites 3d 4h 57m 19s ⏱️ Results for commit 8f54bc5. ♻️ This comment has been updated with latest results. |
c585da4 to
eb694e6
Compare
|
Note: clearly if we want to use |
Yes, it would be nice to have the header somewhere in |
|
For future reference, this is the reverse direction of #4171 |
eb694e6 to
5611b06
Compare
|
I just tested removing all |
ccb43c6 to
195fcf1
Compare
|
I am not sure what is the merit of going this direction. If we want to avoid the assert, it is rather a symptom rather than an issue. That is, when we see the assert we almost certainly have screwed up the setup of the llvm linking. |
The goal here is to make this a non-screwup and allow linking against the |
Yes, and that would cause symbols to clash when dlopening libraries in libCling that bring their own copy of llvm. Of course that assertion won’t fire however later when you deploy root it would cause issues. We’ve been discussing this over many years and there doesn’t seem to be a reasonable way forward. That’s why I wonder if it’s worth revisiting this topic rather than actually fixing llvm to not link libLLVM.so when we request static linking. |
When building against an external LLVM, the user tells us (more or less) that they don't care about symbol clashes. Otherwise they would use |
Yes, but what is the end goal? You can't do what you are doing for rootcling to libCling, right? If you do the same strategy you can't load things like libmessa which ships its own llvm. |
I don't understand this question, what does "rootcling to libCling" mean?
I don't see this being the case: |
What if they are different which is usually the case? Also, some distributions statically linked llvm in messa and who knows what else. I think going in this direction will cause a lot of pain for us because we don't know what the users are going to ask ROOT to |
Then you face the same problem as with any process that has two version of LLVM in it. Note that this is already the case today because an external LLVM is not built in a way that we can hide all symbols, even if linked in statically. The only way we can guarantee this is with
As mentioned above, I think this can already cause problems today because statically linking in external LLVM is not guaranteed to hide all symbols.
I would like to ask that you view the problem from a different angle: Today it is not possible to build ROOT against a shared |
For rootcling, yes. However, that's my point the current setup (before this PR) prevents us to screw up. If we let that work, the obvious next question is what happens to libCling.so? Does it depend (eg dlopen at runtime) on libLLVM.so?
I can see that in the case of If we want to link against root/interpreter/CppInterOp/lib/CppInterOp/CMakeLists.txt Lines 60 to 94 in a16cd17 |
First, this PR itself only modifies root/interpreter/CMakeLists.txt Lines 132 to 137 in a16cd17
Yes, eventually
I know this code, it's extremely hacky, and it doesn't work in all cases: If Clang is built against an LLVM with root/interpreter/CppInterOp/lib/CppInterOp/CMakeLists.txt Lines 95 to 107 in a16cd17 but it doesn't fully work, there are linking problems in current master when I tried two weeks ago.
|
I agree with that, however, rootcling heavily depends on llvm and there is no good reason to move away from llvm-based option parsing unless we want to achieve something broader. That means that those two things cannot really be decoupled because the first thing seen by itself is a waste of time/effort.
What I actually meant is that we have some way to signal that we are doing something wrong. The approach that we statically link rootcling will be gone iirc. For example, we have this code in
That is the reason I expressed concerns about this PR to begin with. I do not think we should go that route. We lower the route towards using system llvm but at the same time we lower the bar to introduce setups that are prone to subtle abi mismatches.
The approach is hacky but can achieve what you need with far less friction. If you share what did not work I'd be happy to help fixing it rather to go the current direction. Yes, the cmake approach is more of a workaround and where the real fix should be in clang. That is, when we install clang in the clang's cmake targets file we need to add logic that respects the fact if we selected static linking. If we did, it should not attach |
If I understand correctly what you're saying, #12156 for you is a "feature" to signal a potential, but not guaranteed problem to the user? For me, it is a bug in our build system that should eventually be fixed because that is where upstream LLVM is going.
I cannot argue against the point that there are (other) ways to screw up. But on the other hand, I firmly believe that it would be better to have the possibility than outright disallowing it. I would be ok with making this build mode conditional behind a (hidden) configuration flag, and otherwise clearly error during build rather than at run-time of
You can try it yourself: Build our fork of LLVM and Clang with |
Yes, this is an odd feature as we do not control the linker output but I see this as a feature saying -- you are on a very think ice and very likely you are doing something wrong.
If we figure out a more friendly message I am all ears. If you mean the libLLVM.so direction -- that's what's the claim since N years now but I do not think the community agrees completely with that. Partially because of the problem of leaked symbols and abi mismatches
So you are proposing to link rootcling against the system libLLVM.so, complain and move on? rootcling depends on libCling, that'd mean that we also will have to allow linking libCling.so to libLLVM.so which directly raises my concern again.
Ok, if we fixed these, would that be a feasible way forward for now, until we fix the clang issue? |
I want to link both
I don't think these can be fixed because we simply don't have the information about dependent LLVM libraries anymore. And it still doesn't allow building ROOT against |
Ok. In that case it seems we are completing the circle. I think we should not do this because of the arguments I raised. The problem we are discussing with symbols and jits is very subtle and complicated. I do not think there are a lot of people in the world who practically understand it not to speak for the average ROOT user who nominally wants to spend less time in building. EDIT: That means we can hardly rely on user's choice here.
I do not think we will need all of that information to get ROOT to work.
Yes, and my claim is that we should not help such setups to become footguns. In similar cases I'd usually say if you want to maintain such a feature I wouldn't stand in the way but this is different because it can subtly break distributions depending on what's installed on the system. Having that behind an one more option does not make me happier since a) the direction, in my opinion, is concerning as it would make things more fragile in an already fragile system; b) we already have more options that we want. In an incremental world we have a more conservative, less fragile and more sound to the current state of art way: strip away libLLVM.so as discussed and solve the 90% of the raised practical points. |
|
Dear all, I would like to revamp this PR.
@devajithvs could you take care? |
195fcf1 to
c038e90
Compare
c038e90 to
8c441c2
Compare
This would potentially remove issues like ``` CommandLine Error: Option 'W' registered more than once! ``` caused by `llvm::cl` registering the same options multiple times when ROOT is linked against a shared `libLLVM`.
This replaces many usages of `shortflags` with multiple characters whenever possible. Some flags are omitted (e.g., `-writeEmptyRootPCM`) as they cause the diff to be huge and can go in a separate PR.
8c441c2 to
8f54bc5
Compare
8f54bc5 to
b2af2a8
Compare
b2af2a8 to
8f54bc5
Compare
This Pull request:
This is a draft PR to test if it is possible to use optparse in rootcling:
bareclingsubcommand makes this slightly complicated, but not sure if it is worth adding subcommand feature to optparse.This patch potentially removes issues like
caused by
llvm::clregistering the same options multiple times when ROOT is linked against a sharedlibLLVM.(See: #12156)
Changes or fixes:
Checklist:
This PR fixes #