This reverts commit bc39d7bdd4977a953b2e102f8f7eb479ad78984e.
rename CLANG_SONAME to LIBCLANG_SOVERSION
[clang][cmake] introduce option CLANG_FORCE_MATCHING_LIBCLANG_SOVERSION
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Comment Actions
IMO looks good, thanks!
For reference & completeness, this came from https://github.com/h-vetinari/llvm-project/tree/libclang after discussion in https://discourse.llvm.org/t/rationale-for-removing-versioned-libclang-middle-ground-to-keep-it-behind-option/64410
| clang/CMakeLists.txt | ||
|---|---|---|
| 467 | OFF by default changes behaviour, which seems irresponsible so late in the release cycle | |
| clang/tools/libclang/CMakeLists.txt | ||
| 7 | Here says VERSION, clang/CMakeLists.txt says SOVERSION | |
| 15–18 | This is highly subjective. Many believe the default was worse due to (a) confusion (b) technical issues when coinstalling multiple LLVM versions. Ping-ponging like this is just creating a mess. | |
Comment Actions
Thanks for the review. Given that you have concerns, could you voice them in a larger forum (https://discourse.llvm.org/t/rationale-for-removing-versioned-libclang-middle-ground-to-keep-it-behind-option/64410), where so far the direction was in favour of going back to the status of LLVM 14 (but with an opt-out for those who prefer equality).
| clang/CMakeLists.txt | ||
|---|---|---|
| 467 | I disagree. This is the status of the last released version. | |
| clang/tools/libclang/CMakeLists.txt | ||
| 7 | Thanks for catching this! | |
| 15–18 | The ping pong has not been released yet, and so it's "just" happening as part of the development process so far. It's subjective how this changes ranks vis-à-vis someone's personal preferences/priorities, but I'd argue it's objectively better from a technical perspective, as it keeps strictly more information as otherwise. On top of that we're solving (b) for those who do not want to make use of this additional information, or are bothered by it. | |
Comment Actions
My concerns have already been raised by others in that thread and related issues, I see no point in restating them yet again. I don't see consensus, I see a handful of people discussing reverting a change that broke a whole bunch of assumptions made by real-world code.
| clang/CMakeLists.txt | ||
|---|---|---|
| 467 | It's the status of the last release candidate, something which downstreams are encouraged to consume early in order to give feedback. If we only need to care about the last released version when making changes then what's the point of having a release branch with policies around what's ok to backport? | |
| clang/tools/libclang/CMakeLists.txt | ||
| 15–18 | Except people package the rc's and get annoyed when major changes like this are made; if we ask for people to test things, then go and revert something significant at the last minute, that's not good practice. | |
Comment Actions
My concerns have already been raised by others in that thread and related issues, I see no point in restating them yet again. I don't see consensus, I see a handful of people discussing reverting a change that broke a whole bunch of assumptions made by real-world code.
I don't think that's a fair characterization. The release manager (as well as author of bot the original change & the reversion) and the interim release manager discussed this and came to the same conclusion despite being very aware of the complaints. Other than that there was one comment against where the person did not respond to a request for further information & 3 people who were asking to keep the ABI information.
| clang/CMakeLists.txt | ||
|---|---|---|
| 467 | This kind of change is a good example why there are release candidates. We raised this shortly after rc2 (I built them as fast as possible, given that limits on public CI means this is a multistep process that can take a bit). | |
| clang/tools/libclang/CMakeLists.txt | ||
| 15–18 | Which is why we're trying to get this into the next/last rc. | |
Comment Actions
Updated the option to default to ON - meaning we keep the current
behavior found in main. This rationel for this is explained in my post here:
Comment Actions
@jrtc27 What do you think about this patch with the default flipped? I think this is how it should land personally as discussed on discourse.
I have been trying to listen in how people want to handle this and I hope this is a good middle ground that we can agree on for 15.
| clang/tools/libclang/CMakeLists.txt | ||
|---|---|---|
| 10–13 | Sorry I didn't get to comment in time, but now that the default was switched from OFF to ON, the comments here are lying... | |
| clang/tools/libclang/CMakeLists.txt | ||
|---|---|---|
| 10–13 | Thanks - will fix it in main as a NFC. | |
Comment Actions
I have another question, probably mainly for @tstellar (since I don't understand the clang/tools/libclang/libclang.{exports,map} infrastructure). Now that we're defaulting back to the equality case, would we need to reinstate libclang.exports (probably conditional on CLANG_FORCE_MATCHING_LIBCLANG_SOVERSION == ON)?
Comment Actions
Was this reviewed by anyone on the original change? As far as I can tell, there was no agreement on the original change or here that reverting is the way to go. Was this discussed elsewhere?
(I don't have an opinion on which approach is better myself.)
Comment Actions
The bulk of the discussion was done in this thread: https://discourse.llvm.org/t/rationale-for-removing-versioned-libclang-middle-ground-to-keep-it-behind-option/64410
I think the original revert in https://reviews.llvm.org/D129160 was probably a little hasty. But since this was raised pretty late in the 15.x release process this is the compromise that was the least bad IMHO. I had some offline discussions with Tom and Hans about it as well and the conclusion is that we should decide a proper way forward but the end of 15.x release window was the wrong place to have that discussion. I hope we can sort this out before 16 is branched and released.
OFF by default changes behaviour, which seems irresponsible so late in the release cycle