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

[llvm-shlib] Fix the version naming style of libLLVM for Windows #85710

Merged
merged 1 commit into from
Mar 19, 2024

Conversation

mstorsjo
Copy link
Member

This reverts the changes from 91a3846 for Windows targets. The changes in that commit don't work as expected for Windows targets (those parts of llvm_add_library don't quite behave the same for Windows), while the previous status quo (producing a library named "libLLVM-.dll") is the defacto standard way of doing versioned library names there, contrary to on Unix.

After that commit, the library always ended up named "libLLVM.dll", executables linking against it would reference "libLLVM.dll", and "libLLVM-.dll" was provided as a symlink.

Thus revert this bit back to as it were, so that executables actually link against a versioned libLLVM, and no separate symlink is needed.

The only thing that might be improved compared to the status quo as it was before these changes, is that the import library is named "lib/libLLVM-.dll.a", while the common style would be to name it plainly "lib/libLLVM.dll.a" (even while it produces references to "libLLVM-.dll", but none of these had that effect for Windows targets.

(As a side note, the llvm-shlib library can be built for MinGW, but not currently in MSVC configurations.)

This reverts the changes from 91a3846
for Windows targets. The changes in that commit don't work
as expected for Windows targets (those parts of llvm_add_library
don't quite behave the same for Windows), while the previous status
quo (producing a library named "libLLVM-<major>.dll") is the
defacto standard way of doing versioned library names there,
contrary to on Unix.

After that commit, the library always ended up named "libLLVM.dll",
executables linking against it would reference "libLLVM.dll", and
"libLLVM-<major>.dll" was provided as a symlink.

Thus revert this bit back to as it were, so that executables
actually link against a versioned libLLVM, and no separate
symlink is needed.

The only thing that might be improved compared to the status quo
as it was before these changes, is that the import library is
named "lib/libLLVM-<major>.dll.a", while the common style would
be to name it plainly "lib/libLLVM.dll.a" (even while it produces
references to "libLLVM-<major>.dll", but none of these had that
effect for Windows targets.

(As a side note, the llvm-shlib library can be built for MinGW, but
not currently in MSVC configurations.)
@mstorsjo mstorsjo requested a review from tstellar March 18, 2024 22:29
@mstorsjo mstorsjo merged commit cb2ca23 into llvm:main Mar 19, 2024
5 checks passed
@mstorsjo mstorsjo deleted the fix-shlib branch March 19, 2024 06:49
@mstorsjo mstorsjo added this to the LLVM 18.X Release milestone Mar 19, 2024
@mstorsjo
Copy link
Member Author

/cherry-pick cb2ca23

@llvmbot
Copy link
Collaborator

llvmbot commented Mar 19, 2024

Failed to cherry-pick: cb2ca23

https://github.com/llvm/llvm-project/actions/runs/8338749880

Please manually backport the fix and push it to your github fork. Once this is done, please create a pull request

@mstorsjo
Copy link
Member Author

/cherry-pick ec2b752 f849805 cb2ca23

llvmbot pushed a commit to llvmbot/llvm-project that referenced this pull request Mar 19, 2024
…m#85710)

This reverts the changes from 91a3846
for Windows targets. The changes in that commit don't work as expected
for Windows targets (those parts of llvm_add_library don't quite behave
the same for Windows), while the previous status quo (producing a
library named "libLLVM-<major>.dll") is the defacto standard way of
doing versioned library names there, contrary to on Unix.

After that commit, the library always ended up named "libLLVM.dll",
executables linking against it would reference "libLLVM.dll", and
"libLLVM-<major>.dll" was provided as a symlink.

Thus revert this bit back to as it were, so that executables actually
link against a versioned libLLVM, and no separate symlink is needed.

The only thing that might be improved compared to the status quo as it
was before these changes, is that the import library is named
"lib/libLLVM-<major>.dll.a", while the common style would be to name it
plainly "lib/libLLVM.dll.a" (even while it produces references to
"libLLVM-<major>.dll", but none of these had that effect for Windows
targets.

(As a side note, the llvm-shlib library can be built for MinGW, but not
currently in MSVC configurations.)

(cherry picked from commit cb2ca23)
@llvmbot
Copy link
Collaborator

llvmbot commented Mar 19, 2024

/pull-request #85746

@mstorsjo
Copy link
Member Author

As a headsup to @lazka and @mati865 wrt msys2 packaging: LLVM 18.1.0 and 18.1.1 had the llvm dylib erroneously named libLLVM.dll, while this PR restores it to be libLLVM-18.dll (and I'm suggesting backporting it).

I didn't include any extra backwards compat symlink named libLLVM.dll pointing at libLLVM-18.dll though, so any msys2 packages that you've built on top of LLVM 18, that depend on libLLVM, will break and fail to find their dependencies on update of LLVM to later 18.x (if this is backported). So they will need to be rebuilt after restoring the name of the library here.

I guess you can add a temporary symlink/copy within the msys2 packaging of later 18.1.x to smooth out this - but I felt it's the best tradeoff to not provide a symlink on the LLVM side here by default, as symlinks often end up as duplicate copies on Windows.

(This issue with symlinks ending up as large duplicate files can be noticed in the release packages at https://github.com/mstorsjo/llvm-mingw/releases - the llvm-mingw-*-ucrt-x86_64.zip packages were around 135 MB each up to 18.1.0 RC2, but after RC3 they grew to 158 MB, as the package contained two separate copies of the libLLVM DLL.)

@lazka
Copy link

lazka commented Mar 19, 2024

Thanks. Note that it's libLLVM-18.1.dll in MSYS2 atm since we pached it differently to align with unix (major.minor being the new ABI version since v18 from what I understand)

Maybe we'll leave it as is, and revert to the upstream schema with 19(?)

@mstorsjo
Copy link
Member Author

Thanks. Note that it's libLLVM-18.1.dll in MSYS2 atm since we pached it differently to align with unix (major.minor being the new ABI version since v18 from what I understand)

Oh, ok, I see - yeah in that case, if you've already patched around it, this isn't an issue at all. The earlier changes slipped past me as they didn't break the build for me.

@lazka
Copy link

lazka commented Mar 19, 2024

They did break llvm-config (which is used by meson) afair, but I think that's not included in llvm-mingw.

We would have reported it eventually, but there seemed various things in flux re versions already.

@mstorsjo
Copy link
Member Author

They did break llvm-config afair, but I think that's not included in llvm-mingw.

Oh, right, that's a good point. Let me check whether we'd need to tweak that along with this change here.

@lazka
Copy link

lazka commented Mar 19, 2024

If its like with v17 it should be good I think

@mstorsjo
Copy link
Member Author

If its like with v17 it should be good I think

Yep, I checked, and e.g. llvm-config --libs mc prints -lLLVM-19git right now, which is the right thing for the mingw target.

For the unix targets, it should still work as we do provide libLLVM-19git.so as a symlink to libLLVM.so.19.0git. If we change it to return -lLLVM instead, please only do that change for unix targets, as the mingw/windows one still should keep using the -lLLVM-19git form.

llvmbot pushed a commit to llvmbot/llvm-project that referenced this pull request Mar 19, 2024
…m#85710)

This reverts the changes from 91a3846
for Windows targets. The changes in that commit don't work as expected
for Windows targets (those parts of llvm_add_library don't quite behave
the same for Windows), while the previous status quo (producing a
library named "libLLVM-<major>.dll") is the defacto standard way of
doing versioned library names there, contrary to on Unix.

After that commit, the library always ended up named "libLLVM.dll",
executables linking against it would reference "libLLVM.dll", and
"libLLVM-<major>.dll" was provided as a symlink.

Thus revert this bit back to as it were, so that executables actually
link against a versioned libLLVM, and no separate symlink is needed.

The only thing that might be improved compared to the status quo as it
was before these changes, is that the import library is named
"lib/libLLVM-<major>.dll.a", while the common style would be to name it
plainly "lib/libLLVM.dll.a" (even while it produces references to
"libLLVM-<major>.dll", but none of these had that effect for Windows
targets.

(As a side note, the llvm-shlib library can be built for MinGW, but not
currently in MSVC configurations.)

(cherry picked from commit cb2ca23)
chencha3 pushed a commit to chencha3/llvm-project that referenced this pull request Mar 23, 2024
…m#85710)

This reverts the changes from 91a3846
for Windows targets. The changes in that commit don't work as expected
for Windows targets (those parts of llvm_add_library don't quite behave
the same for Windows), while the previous status quo (producing a
library named "libLLVM-<major>.dll") is the defacto standard way of
doing versioned library names there, contrary to on Unix.

After that commit, the library always ended up named "libLLVM.dll",
executables linking against it would reference "libLLVM.dll", and
"libLLVM-<major>.dll" was provided as a symlink.

Thus revert this bit back to as it were, so that executables actually
link against a versioned libLLVM, and no separate symlink is needed.

The only thing that might be improved compared to the status quo as it
was before these changes, is that the import library is named
"lib/libLLVM-<major>.dll.a", while the common style would be to name it
plainly "lib/libLLVM.dll.a" (even while it produces references to
"libLLVM-<major>.dll", but none of these had that effect for Windows
targets.

(As a side note, the llvm-shlib library can be built for MinGW, but not
currently in MSVC configurations.)
@pointhex pointhex mentioned this pull request May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants