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

Make all static library exports contribute to dlls on Windows #2614

Merged
merged 1 commit into from Mar 5, 2023

Conversation

rikkimax
Copy link
Contributor

@rikkimax rikkimax commented Mar 5, 2023

@rikkimax
Copy link
Contributor Author

rikkimax commented Mar 5, 2023

Looks like dmd-master failing isn't related to this PR.

Copy link
Contributor

@thewilsonator thewilsonator left a comment

Choose a reason for hiding this comment

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

Any way to test this?

source/dub/compilers/ldc.d Outdated Show resolved Hide resolved
changelog/static_libraries_exported_dll.dd Outdated Show resolved Hide resolved
@rikkimax
Copy link
Contributor Author

rikkimax commented Mar 5, 2023

Any way to test this?

The last time I looked at the test suite it didn't run on Windows and it wouldn't be the simplest of test to write, since you'd need a static lib + DLL + something to verify symbols were exported.

Scream test might be the best we get, and people who use it will certainly scream quickly if it's not in.

@thewilsonator
Copy link
Contributor

Scream test might be the best we get, and people who use it will certainly scream quickly if it's not in.

Lol, fair enough.

@dlang-bot dlang-bot merged commit 076022c into dlang:master Mar 5, 2023
@kinke
Copy link
Contributor

kinke commented Mar 6, 2023

This was merged too soon; this can have undesired side effects like larger binaries and link-time regressions. We've had a similar regression just a few weeks back at work - some library containing a module depending on some 3rd-party lib only available on Linux, and that module (totally unused on Windows) forcefully being linked in suddenly causing lots of undefined symbol errors. The really bad thing here is that unlike default -fvisibility=public -dllimport=all, these linker flags cannot be overridden/prevented.

And there are other ways to work around this problem; -dllimport=all was designed for all other D library deps being DLLs. When really wanting to link other libs statically into the DLL (edit: a DLL compiled with -dllimport=all), a sourceLibrary would work best in terms of generated code (but suboptimal regarding build times). Compiling the static lib as single object via -singleobj would also work in almost all cases, but come with similar disadvantages as /WHOLEARCHIVE.

@rikkimax
Copy link
Contributor Author

rikkimax commented Mar 6, 2023

What we could do is add a build requirement to turn this behavior off for a given shared library. So if you need control you have it, but the default will still be it is most likely going to work.

@Geod24
Copy link
Member

Geod24 commented Mar 6, 2023

No test, no review, let's revert: #2616

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

__InterfaceZ is not exported on Windows
5 participants