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

On Windows support disabling of forcing of all object files in a static library to be in a DLL #2615

Closed
wants to merge 1 commit into from

Conversation

rikkimax
Copy link
Contributor

@rikkimax rikkimax commented Mar 6, 2023

Augments #2614

I'll do dub-docs once given all clear.

@kinke is there anything else you want me to do?

@Geod24
Copy link
Member

Geod24 commented Mar 6, 2023

I have a few issues with this:

  • Having separate things for project and package has historically worked very poorly;
  • There are no tests;
  • It's blankly stating that the previous PR introduced a regression;
  • I'm not quite sure what even te problem is here, and why this is a good solution;

Copy link
Member

@WebFreak001 WebFreak001 left a comment

Choose a reason for hiding this comment

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

needs a test to at least demonstrate what this is even doing

@kinke
Copy link
Contributor

kinke commented Mar 6, 2023

I think what almost all users would want is something like symmetryinvestments@f841c6d - being able to override default -fvisibility=public -dllimport=all (designed for a scenario where every dub dependency is a DLL) with e.g. -fvisibility=hidden -dllimport=defaultLibsOnly for an isolated DLL (potentially containing static lib deps, and only exporting explicitly exported symbols).

@Geod24
Copy link
Member

Geod24 commented Mar 6, 2023

@kinke : We need to get the flag propagation fixed. I had the same problem with -preview=in and that's why we have the ugly dependency dflags hack.

@rikkimax rikkimax closed this Mar 6, 2023
@kinke
Copy link
Contributor

kinke commented Mar 6, 2023

Yeah - including all those duplicates when a flag is passed down to all deps and then passed upwards again. I've seen pages (on a FullHD monitor) of identical flags in ugly-ass cmdlines.

@rikkimax
Copy link
Contributor Author

rikkimax commented Mar 6, 2023

What I'm thinking is we need to have a directive to set the visibility. So that dub can actually understand it and add behavior like dllimport (which is kinda a hack we, unfortunately, need currently) and /WHOLEARCHIVE:.

If we combine that directive with my work and add a way to disallow it, not just allow eliding, that should cover our basis here.

I think I'll leave it to you @kinke to get this to completion.

@kinke
Copy link
Contributor

kinke commented Mar 6, 2023

I think /WHOLEARCHIVE shouldn't be needed in most cases. It's only needed for some static lib deps of a 'default' DLL compiled with -dllimport=all. I'd recommend using a sourceLibrary for such scenarios (but that target type should be selected by the parent project, not the dependency itself, which currently has to expose a dub config with matching target type - IMO totally bananas).

Wrt. making all of this a bit easier - that'd be nice of course, but the IMO biggest problem is that Windows makes everything regarding shared libs at least an order of magnitude more complex than Posix. And I'd try to avoid extending dub by ugly stuff only needed for Windows.

@rikkimax
Copy link
Contributor Author

rikkimax commented Mar 6, 2023

Problem is, once it is needed you have people who don't know what a linker is asking why it doesn't work. When you give them an explanation they complain very loudly and are more likely to leave D even if it isn't our fault.

Ignoring it isn't a good solution.

@kinke
Copy link
Contributor

kinke commented Mar 6, 2023

I probably shouldn't have changed the default compile-DLL flags with LDC on Windows with dub v1.30 yet, as it introduced such problems for existing DLL projects. It newly enables working with shared-lib dependencies, but broke at least some existing isolated DLLs.

But what's IMO important is taking the time to come up with a good, generic-as-possible solution. E.g., this specific problem could also be handled by making dub default to building all dependencies of a shared library project as shared libs themselves (recursively).

symmetryinvestments@f841c6d then allows to customize exports and imports for a DLL or executable. I haven't tested that in production yet but will do, hopefully pretty soon. It would cover 2 problems we currently have at Symmetry:

  • The main executable is linked against shared druntime/Phobos via -link-defaultlib-shared, so that we can load D plugins (shared libs) which also use that shared druntime/Phobos (incl. shared GC, shared thread lists etc. etc.). All static lib dependencies of the executable need to be compiled with -dllimport=defaultLibsOnly (or -link-defaultlib-shared, which defaults to that), and dub currently doesn't handle that.
  • We have about 25 plugins - pretty isolated shared libs compiled with -fvisibility=hidden -dllimport=defaultLibsOnly, so that they only export a few select symbols and only share druntime/Phobos. All static lib deps of these plugins need to be compiled with the same flags. They all depend on one base project, so by adding -fvisibility=hidden -dllimport=defaultLibsOnly for that single project and making sure these propagate up and down the dub dependencies graph, that is handled in a single central place.

@rikkimax
Copy link
Contributor Author

rikkimax commented Mar 6, 2023 via email

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

Successfully merging this pull request may close these issues.

5 participants