Page MenuHomePhabricator

SONAME introduce option CLANG_FORCE_MATCHING_LIBCLANG_SOVERSION
ClosedPublic

Authored by thieta on Aug 23 2022, 10:20 AM.

Details

Summary

This reverts commit bc39d7bdd4977a953b2e102f8f7eb479ad78984e.

rename CLANG_SONAME to LIBCLANG_SOVERSION

[clang][cmake] introduce option CLANG_FORCE_MATCHING_LIBCLANG_SOVERSION

Diff Detail

Event Timeline

thieta created this revision.Aug 23 2022, 10:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 23 2022, 10:20 AM
thieta requested review of this revision.Aug 23 2022, 10:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 23 2022, 10:20 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
thieta retitled this revision from Revert "libclang.so: Make SONAME the same as LLVM version" to SONAME introduce option CLANG_FORCE_MATCHING_LIBCLANG_SOVERSION.Aug 23 2022, 10:41 AM
jrtc27 requested changes to this revision.Aug 23 2022, 12:16 PM
jrtc27 added a subscriber: jrtc27.
jrtc27 added inline comments.
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.

This revision now requires changes to proceed.Aug 23 2022, 12:16 PM

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.

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).

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.

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.

brooks added a subscriber: brooks.Aug 23 2022, 3:48 PM
thieta updated this revision to Diff 455087.Aug 23 2022, 11:35 PM

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:

https://discourse.llvm.org/t/rationale-for-removing-versioned-libclang-middle-ground-to-keep-it-behind-option/64410/32

thieta updated this revision to Diff 455088.Aug 24 2022, 12:04 AM

Fixed variable name

@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.

This revision was not accepted when it landed; it landed in state Needs Review.Aug 24 2022, 11:36 PM
This revision was automatically updated to reflect the committed changes.
h-vetinari added inline comments.Aug 25 2022, 12:52 AM
clang/tools/libclang/CMakeLists.txt
9–12

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...

thieta added inline comments.Aug 25 2022, 12:53 AM
clang/tools/libclang/CMakeLists.txt
9–12

Thanks - will fix it in main as a NFC.

h-vetinari added a comment.EditedAug 25 2022, 1:00 AM

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)?

thakis added a subscriber: thakis.Aug 25 2022, 4:08 AM

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.)

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.)

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.