Skip to content

Resurrect #1735 / #2044 (-link-defaultlib-shared)#2443

Merged
kinke merged 1 commit intoldc-developers:masterfrom
kinke:defaultlib-shared
Jan 12, 2018
Merged

Resurrect #1735 / #2044 (-link-defaultlib-shared)#2443
kinke merged 1 commit intoldc-developers:masterfrom
kinke:defaultlib-shared

Conversation

@kinke
Copy link
Member

@kinke kinke commented Dec 10, 2017

No description provided.

@kinke
Copy link
Member Author

kinke commented Dec 14, 2017

Some feedback about the switch name please, so that we can deliver it in v1.7. Note that the probably more important thing here is that shared default libs are used by default when generating a shared library (as long as they are available in the first place, e.g., not on Windows).

The breaking change for distros remains - they'd either need to adopt the -shared suffix for the shipped libs or patch out the hardcoded suffix.

@JohanEngelen
Copy link
Member

TL;DR:
I don't have a strong opinion about the naming of the flags. My main point is that it'd be nice if the boolean flags have a verb in it.
Regarding including it in 1.7: such an intentional breaking change (which we know people will experience) is not something expected after a beta is already out.

For the naming, I think I prefer the -link- prefix, such that it is clear that one is not specifying the libraries, but is linking with them because with -link-, there is a verb in the name.
Right now you have:

-defaultlib=<string>
-debuglib=<string>
-defaultlib-debug=<bool>  (uses -debuglib if true)
-defaultlib-shared=<bool>  (adds -shared suffix if true, possibly something more complex in future)

How about:

-defaultlib=<string>
-defaultlib-debug=<string>
-link-defaultlib-debug=<bool>
-link-defaultlib-shared=<bool>

How much do we want to break things, in order to improve things? I like "std" better than "default", because it fits better with common parlance. Also, it's unfortunate that the naming does not indicate that one should pass all libraries to a single -defaultlib command, I think "libs" plural would solve that:

-stdlibs=<string>
-stdlibs-debug=<string>
-link-stdlibs-debug=<bool>
-link-stdlibs-shared=<bool>

@JohanEngelen
Copy link
Member

JohanEngelen commented Dec 17, 2017

Just read #2044 again. About "default" vs "std", I guess you have a point about "std" perhaps referring only to Phobos. So then "defaultlibs" (plural) would be the most clear? (but then not worth the breakage)

@kinke
Copy link
Member Author

kinke commented Dec 17, 2017

Thx for the feedback.

So then "defaultlibs" (plural) would be the most clear? (but then not worth the breakage)

Exactly my thoughts.

Right now you have: [...] -debuglib=

That one's hidden now, deprecated so to speak, but still there for backwards and DMD compatibility ((L)DMD also has -debuglib).

Wrt. adding a link- verb prefix, I don't have a strong opinion, but omitting it leads to -defaultlib-{debug,shared} being listed right below -defaultlib=... in the cmdline help.

Regarding including it in 1.7: such an intentional breaking change (which we know people will experience) is not something expected after a beta is already out.

I don't have a strong opinion on this either, but I thought that getting rid of the current invalid-instruction issue would outweigh the breakage. Non-distro users would only experience the breakage if they compile themselves with BUILD_SHARED_LIBS=ON (now with -shared lib suffix too). So I'd almost exclusively expect distros shipping with shared libs only to be affected. Debian and Fedora are both at v1.5.0, snap at v1.4 etc., so breaking from 1.7 beta1 to final shouldn't be much of an issue for 'unofficial' package maintainers.

Edit: Oh I forgot the breaking change for people currently resorting to -shared -defaultlib=druntime-ldc-shared,phobos2-ldc-shared -debuglib=druntime-ldc-debug-shared,phobos2-ldc-debug-shared (with implicit -defaultlib-shared => ...-shared-shared). So yeah, delaying it to v1.8 sounds reasonable.

@JohanEngelen
Copy link
Member

Oh, so I misunderstood maybe. Let's remove the deprecated flags from the discussion. So then the recommended interface is this (which works for most use cases, the hidden options are there for more advanced stuff) ?

-defaultlib=<string>
-defaultlib-debug=<bool>  (appends "-debug")
-defaultlib-shared=<bool>  (appends "-shared")

About ordering in the help output: we can make categories/sections to help with that; combining all link related flags in one section. -defaultlib-link-debug is also an option, but less nice.

@kinke
Copy link
Member Author

kinke commented Dec 17, 2017

Yeah that'd be the proposed interface, expecting either

  • -defaultlib= (don't link against druntime/Phobos) or
  • [-defaultlib-debug] [-defaultlib-shared]

from the user in almost all cases. -defaultlib-shared=false may be used to enforce static default libs for a shared library (e.g., as enforced in ldc2.conf on Windows due to unavailable shared default libs) and is thus expected to be specified very rarely by users directly. An explicit -defaultlib-shared[=true] is mostly expected when generating executables to be linked against the shared libs.

[And -link-debuglib is still an alias for -defaultlib-debug for backwards compatibility.]

@kinke
Copy link
Member Author

kinke commented Jan 4, 2018

Agreed & ready to be merged?

@JohanEngelen
Copy link
Member

Let's add a verb in the boolean options, -defaultlib-link-debug and -defaultlib-link-shared? (adding it in the middle so the options are still listed next to -defaultlib)

@kinke
Copy link
Member Author

kinke commented Jan 7, 2018

I don't really like the verb in the middle. I just experimented with a dedicated category, that'd look like this (with current master, i.e., old interface):

USAGE: ldc2 [options] files --run Runs the resulting program, passing the remaining arguments to it

OPTIONS:

Default libraries to link with:

  -debuglib=<lib1,lib2,...>                 - Debug versions of default libraries (overrides previous)
  -defaultlib=<lib1,lib2,...>               - Default libraries to link with (overrides previous)
  -link-debuglib                            - Link with libraries specified in -debuglib, not -defaultlib

General options:

  -D                                        - Generate documentation
...

I'd rather have it listed below the general options (but haven't figured out how), otherwise I'd find it pretty nice with -link-defaultlib-{debug,shared}.

@JohanEngelen
Copy link
Member

Looks nice indeed with the category. The ordering is unfortunate, but I can live with it (it's not something super simple as sorting alphabetically, right?).
(Perhaps we should think about more categories, like for all the different instrumentation/sanitizer/profiling things)

... to -link-defaultlib-debug (with alias for backwards compatibility).

-link-defaultlib-shared is to be used for switching between static and
shared default libs to be linked with.

It defaults to true when generating shared libraries (if shared
druntime/Phobos are supported for the target, i.e., not for Windows).
This is a benign breaking change!
@kinke kinke force-pushed the defaultlib-shared branch from 15f45a3 to 58dfe7e Compare January 11, 2018 21:12
@kinke
Copy link
Member Author

kinke commented Jan 11, 2018

it's not something super simple as sorting alphabetically, right?

It is. ;)

Looks like this now, below General options and Generic options (-help, -version):

Libraries linked with by default:

  -defaultlib=<lib1,lib2,...>   - Default libraries to link with (overrides previous)
  -link-defaultlib-debug        - Link with debug versions of default libraries
  -link-defaultlib-shared       - Link with shared versions of default libraries

@kinke kinke changed the title Resurrect #1735 / #2044 (-defaultlib-shared) Resurrect #1735 / #2044 (-link-defaultlib-shared) Jan 11, 2018
@JohanEngelen
Copy link
Member

cool. lgtm.

Offtopic: do you think it's worthwhile grouping the instrumentation options?

@kinke
Copy link
Member Author

kinke commented Jan 11, 2018

Definitely worthwhile IMO, same as for the other groups you mentioned earlier. With -help-list, one can always get the flat alphabetically sorted list if need be.

@JohanEngelen
Copy link
Member

I was thinking of a single group combining instrumentation/sanitizer/profiling options. Don't know what to call it, but they all involve instrumentation / investigation of code. (should PGO be in there?)

@kinke kinke merged commit 1572c91 into ldc-developers:master Jan 12, 2018
@kinke kinke deleted the defaultlib-shared branch January 12, 2018 17:26
@p0nce
Copy link
Contributor

p0nce commented Jul 16, 2018

Minor remark :

This was a breaking change and in the 1.8 changelog what we see is

breaking

There is no mention of it being a breaking change (-shared's meaning has changed)
In the future it would help if BREAKING was marked prominently, that's the main reason to read a changelog.

@kinke
Copy link
Member Author

kinke commented Jul 18, 2018

Noted. In my defence, it wasn't necessarily a breaking change, as previously it mainly depended on how LDC was built (BUILD_SHARED_LIBS CMake setting). E.g., Debian/Ubuntu distro packages used to ship with shared defaultlibs only, and so there was no change for shared libs generated via -shared. I like to keep things tight, incl. and particularly the change log, so that people actually do read it, so mentioning -shared now linking against shared defaultlibs by default seemed enough to me. The change was most likely welcomed by 99% of users, which previously might have gotten an illegal-instruction startup error due to an assert(0) when using 2+ shared D objects, each with their own statically linked druntime. You are probably one of the few 1) redistributing an .so, 2) not wanting to bundle the shared defaultlibs with it, and 3) being certain that your .so is the only D one (i.e., no multiple druntimes in a process).

@p0nce
Copy link
Contributor

p0nce commented Jul 19, 2018

I agree our use case is niche but I think makes sense for anyone that will want to distribute D "plugins" for any software.

About point 1&2), redistributing rather small and self-contained shared libraries is pretty much standard in our line of work, and they must come in 2 archs for 2 OS, so it would be an enormous weight to bear to add the full shared libraries (I estimate 70mb). I work behind phone internet so small size are paramount! Moreover, phobos is only getting larger. I honestly don't think we have much of a choice with the "odd" things we do.

About your point 3), I'm not sure what using multiple runtimes would create as a problem, we used to do that before disabling it. We'd like to enable it again thanks to the work LDC did, if it's only about avoiding TLS no problem.

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.

3 participants