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

cmake: set the soname on the shared library #10023

Closed
wants to merge 5 commits into from
Closed

Conversation

bagder
Copy link
Member

@bagder bagder commented Dec 3, 2022

Addresses KNOWN_BUG 15.1

Here we go again!

@bagder bagder added the cmake label Dec 3, 2022
@jzakrzewski
Copy link
Contributor

This will not be consistent with autotools on all platforms (I'd really like to know the reason libtool decides to have 100 different ways to compute this). But will work on normal Linux distributions, and it's quite open for extension.
I think we can roll with that for starters.

@bagder
Copy link
Member Author

bagder commented Dec 3, 2022

I think libtool works on more esoteric platforms than cmake does (or where our users will use it), so possibly this will not need the same amount of crazy logic.

@jzakrzewski
Copy link
Contributor

jzakrzewski commented Dec 3, 2022

Sure. Nevertheless, I have identified following common platforms (each group use different calculations):

  • AIX, Linux, GNU/kFreeBSD, (modern) FreeBSD, Haiku, SunOS (version 5, 6)
  • HP-UX, NetBSD, OpenBSD, SunOS (version < 5)
  • SCO_SV, UnixWare, UNIX_SV, XENIX
  • Windows
  • Cygwin
  • All Apple OSes

My old PR trying to solve this has been deemed too complicated.

@bagder
Copy link
Member Author

bagder commented Dec 3, 2022

I suppose that answers the question why libtool has so many different ways...

My old PR trying to solve this has been deemed too complicated.

This functionality is not possible to provide for all these platforms without a lot of complication. It is a complication I wish cmake itself provided for us, but since it doesn't I think we need to provide the logic if we ever want the cmake build to seriously become a serious autoconf alternative for curl builds.

each group use different calculations

I suppose this also means that my very simplified approach in this PR is then completely wrong for several of those groups?

@vszakats
Copy link
Member

vszakats commented Dec 4, 2022

Tested this with curl-for-win and it did not break the trick used to suppress soname with autotools builds (libcurl dll versioning is not generally desired for standalone/native libcurl Windows builds). It also didn't disturb CMake Windows builds. So, it seems fine from these ends.

@jzakrzewski
Copy link
Contributor

Basically your changes will work correctly for the first group I mentioned.
On plain windows, AFAIR it's also OK because a) SONAME does not exist, b) there's no version number in the library name anyway (right?). Note that Cygwin is another story.

As for the others, the good question is if anybody really cares.

It is a complication I wish cmake itself provided for us

Well, CMake let's us set whatever SONAME/VERSION we want, it just does not deal with the autotools version scheme.

I could try to submit my changes again, and see if you all would want it the codebase. Otherwise, it's probably better if it works at least partially, then not at all. But I'm wondering if an option to either build without SONAME, or to override it would be a good idea in this case.

@bagder
Copy link
Member Author

bagder commented Dec 5, 2022

Well, CMake let's us set whatever SONAME/VERSION we want, it just does not deal with the autotools version scheme.

And therein lies the problem. The libtool version setup is made like to that to hide the platform complexities. It allows us to set one string and have that used automagically to set the correct number on numerous platforms. Without us having to know all those platforms. Without libtool since cmake doesn't help us here, we need to do that translation ourselves.

I could try to submit my changes again, and see if you all would want it the codebase. Otherwise, it's probably better if it works at least partially, then not at all. But I'm wondering if an option to either build without SONAME, or to override it would be a good idea in this case.

I think we should make an effort to do the right thing, yes! An option to switch it off or override is probably a good idea.

@jzakrzewski
Copy link
Contributor

I'll see if I can put something together this week.

@jzakrzewski
Copy link
Contributor

Unfortunately, life happens, and there's no way in hell I will be able to do anything till February.
If you want to temporary improve the situation on Linux you could merge this PR.
Sorry.

@bagder
Copy link
Member Author

bagder commented Dec 9, 2022

If you want to temporary improve the situation on Linux you could merge this PR.

It might be a small step forward anyway. But how will this affect non-Linux builds?

@bagder bagder marked this pull request as ready for review December 9, 2022 14:22
@jzakrzewski
Copy link
Contributor

Not sure about Windows, but for other platforms the SONAME and/or file name may diverge from the one produced by the autotools.

@vszakats
Copy link
Member

vszakats commented Dec 9, 2022

According to curl-for-win tests, the Windows CMake build is not affected.

@bagder
Copy link
Member Author

bagder commented Dec 12, 2022

but for other platforms the SONAME and/or file name may diverge from the one produced by the autotools.

Does this mean that we cannot merge any fix for SONAME in cmake without trying to fix the entire world's set of platforms at once or should we merge the first take and let the ones you run into problems provide fixes?

@jzakrzewski
Copy link
Contributor

I think we shouldn't introduce an incorrect change. Maybe just guard your changes with a condition like:

 if(CMAKE_SYSTEM_NAME STREQUAL "AIX" OR
    CMAKE_SYSTEM_NAME STREQUAL "Linux" OR
    CMAKE_SYSTEM_NAME STREQUAL "GNU/kFreeBSD" OR

    # FreeBSD comes with the a.out and elf flavours
    # but a.out was supported up to version 3.x and
    # elf from 3.x. I cannot imagine someone runnig
    # CMake on those ancient systems
    CMAKE_SYSTEM_NAME STREQUAL "FreeBSD" OR

    CMAKE_SYSTEM_NAME STREQUAL "Haiku")

?
It will still be an improvement, at the same time it won't screw it up for other platforms.

... and make cmake use the components, and only set SONAME and VERSION for
platforms we think this works on.

Assisted-by: Jakub Zakrzewski
@bagder
Copy link
Member Author

bagder commented Dec 13, 2022

Thanks. Adapted to that, but also made the cmake version use the same numbers from the soname makefile with the necessary math.

Copy link
Contributor

@jzakrzewski jzakrzewski left a comment

Choose a reason for hiding this comment

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

Apart from the repeated properties, LGTM

lib/CMakeLists.txt Outdated Show resolved Hide resolved
@bagder bagder closed this in 5de6848 Dec 15, 2022
@bagder bagder deleted the bagder/cmake-soname branch December 15, 2022 11:37
vszakats added a commit to vszakats/curl that referenced this pull request Dec 12, 2023
- 898b012 curl#1288
- 5de6848 curl#10023
- bunch of others that are completed
- `NTLM_WB_ENABLED` is implemented in a basic form, and now also
  scheduled for removal, so a TODO at this point isn't useful.

And this 'to-check' item:

Q: "The cmake build selected to run gcc with -fPIC on my box while the
   plain configure script did not."

A: With CMake, since 2ebc74c curl#11546
   and fc9bfb1 curl#11627, we explicitly
   enable PIC for libcurl shared lib. Or when building libcurl for
   shared and static lib in a single pass. We do this by default for
   Windows or when enabled by the user via `SHARE_LIB_OBJECT`.
   Otherwise we don't touch this setting. Meaning the default set by
   CMake (if any) or the toolchain is used. On Debian Bookworm, this
   means that PIC is disabled for static libs by default. Some platforms
   (like macOS), has PIC enabled by default.
   autotools supports the double-pass mode only, and in that case
   CMake seems to match PIC behaviour now (as tested on Linux with gcc.)

Closes #xxxxx
vszakats added a commit to vszakats/curl that referenced this pull request Dec 12, 2023
- 898b012 curl#1288
- 5de6848 curl#10023
- bunch of others that are completed
- `NTLM_WB_ENABLED` is implemented in a basic form, and now also
  scheduled for removal, so a TODO at this point isn't useful.

And this 'to-check' item:

Q: "The cmake build selected to run gcc with -fPIC on my box while the
   plain configure script did not."

A: With CMake, since 2ebc74c curl#11546
   and fc9bfb1 curl#11627, we explicitly
   enable PIC for libcurl shared lib. Or when building libcurl for
   shared and static lib in a single pass. We do this by default for
   Windows or when enabled by the user via `SHARE_LIB_OBJECT`.
   Otherwise we don't touch this setting. Meaning the default set by
   CMake (if any) or the toolchain is used. On Debian Bookworm, this
   means that PIC is disabled for static libs by default. Some platforms
   (like macOS), has PIC enabled by default.
   autotools supports the double-pass mode only, and in that case
   CMake seems to match PIC behaviour now (as tested on Linux with gcc.)

Follow-up to 5d5dfdb curl#12500

Closes #xxxxx
vszakats added a commit that referenced this pull request Dec 13, 2023
- manual completed: 898b012 #1288
- soname completed: 5de6848 #10023
- bunch of others that are completed
- `NTLM_WB_ENABLED` is implemented in a basic form, and now also
  scheduled for removal, so a TODO at this point isn't useful.

And this 'to-check' item:

Q: "The cmake build selected to run gcc with -fPIC on my box while the
   plain configure script did not."

A: With CMake, since 2ebc74c #11546
   and fc9bfb1 #11627, we explicitly
   enable PIC for libcurl shared lib. Or when building libcurl for
   shared and static lib in a single pass. We do this by default for
   Windows or when enabled by the user via `SHARE_LIB_OBJECT`.
   Otherwise we don't touch this setting. Meaning the default set by
   CMake (if any) or the toolchain is used. On Debian Bookworm, this
   means that PIC is disabled for static libs by default. Some platforms
   (like macOS), has PIC enabled by default.
   autotools supports the double-pass mode only, and in that case
   CMake seems to match PIC behaviour now (as tested on Linux with gcc.)

Follow-up to 5d5dfdb #12500

Reviewed-by: Jay Satiro
Closes #12509
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

3 participants