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

Disabled forced Windows import library suffix, if static library is not built #16324

Closed
wants to merge 2 commits into from

Conversation

RubisetCie
Copy link

Hello everyone.

A short problem I have at my job, we wish to build a shared version of the curl library from source, but on Windows, the "_imp" mandatory library suffix was a problem.

As this suffix is not needed when only the shared version is built and not the static version, this simple change should have no other impact.

@vszakats
Copy link
Member

vszakats commented Feb 13, 2025

Can you show an example CMake configuration where you experienced this and the expected vs actual result? (and also your C compiler vendor/version)

edit: without those, my initial thought was that you wanted to set BUILD_SHARED_LIBS=OFF, but instead set BUILD_STATIC_CURL=ON, and this doesn't do what you wanted.

In the latter case, both shared and static libcurl are built, thus the suffix is required (for MSVC), and this setting tells to build curl.exe against the static libs.

To disable building the shared lib, I suggest trying the first option: BUILD_SHARED_LIBS=OFF. In this case only the static libcurl is built, with no suffix added.

@RubisetCie
Copy link
Author

Thank you for the reply.
I think I misspoke; I know how to use BUILD_SHARED_LIBS and BUILD_STATIC_LIBS to compile respectively shared version and static version of libcurl.

In my case, I'm on Windows, and using BUILD_SHARED_LIBS=ON and BUILD_STATIC_LIBS=OFF (I only care for shared libcurl).

On MSVC, it will output two files:

  • libcurl.dll
  • libcurl_imp.lib

That's good and all, but I want to get rid of the "_imp" suffix on the .lib.
Currently, I can't easily because if I set the parameter IMPORT_LIB_SUFFIX="", it will be automatically set back to "_imp"...

My pull request aims to fix that; since the only ".lib" file I'm building is the import library, there shouldn't be any confusion.

I hope you understand my issue better...

@vszakats
Copy link
Member

vszakats commented Feb 13, 2025

Thanks for explaining!

You're right. I confused behaviour with libssh2, which does what I described.

The reason it works that way in curl is compatibility with how it worked
before implementing shared/static libcurl in a single pass: 1199308 #11505

It'd be arguably better to only add a suffix if both shared and static are built,
but this would break compatibility for default builds. I personally also don't like
suffixing the implib name, and had an earlier iteration with a question to change
to # TODO: Change to set(STATIC_LIB_SUFFIX "_static")? which would suffix
the static lib. This feels more aligned with common usage (though there is
absolutely no general agreement on this wrt MSVC libs).

Then a search https://github.com/search?q=libcurl_imp&type=code convinced
me that dependents rely on this behaviour and kept _imp for compatibility:
eba7067

Question how to fix this while keeping everyone happy?

Using BUILD_STATIC_CURL here doesn't seem to catch the root issue IMO.

Accepting an override via IMPORT_LIB_SUFFIX might work, when it's not
resulting in a collision with an unsuffixed static lib.

@vszakats vszakats added the Windows Windows-specific label Feb 13, 2025
@testclutch
Copy link

Analysis of PR #16324 at 53394db7:

Test 2070 failed, which has NOT been flaky recently, so there could be a real issue in this PR.

Test 1631 failed, which has NOT been flaky recently, so there could be a real issue in this PR.

Test 363 failed, which has NOT been flaky recently, so there could be a real issue in this PR.

Generated by Testclutch

@vszakats
Copy link
Member

vszakats commented Feb 14, 2025

Proposal to allow overriding suffixes at will, with detection
of the fatal cases: #16332

@vszakats vszakats closed this in 1b87357 Feb 16, 2025
@vszakats
Copy link
Member

Thanks for your report/PR, merged #16332 to fix this.

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.

3 participants