Open rikkimax opened 10 months ago
github-actions[bot]
commented
10 months ago ✅ PR OK, no changes in deprecations or warnings
Total deprecations: 14
Total warnings: 0
Build statistics:
statistics (-before, +after)
executable size=5331080 bin/dub
-rough build time=65s
+rough build time=64s
rikkimax
commented
10 months ago Building test_registry ~master: building configuration [application]
Linking test_registry
Finished To force a rebuild of up-to-date targets, run again with --force
Failed to listen on :::3283
Failed to listen on [0.0.0.0:3283](http://0.0.0.0:3283/)
object.Exception@../../../.dub/packages/vibe-d/0.9.7/vibe-d/http/vibe/http/server.d(2098): Failed to listen for incoming HTTP connections on any of the supplied interfaces.
Trying to download maven-dubpackage (1.0.5)
----------------
??:? object.Throwable.TraceInfo core.runtime.defaultTraceHandler(void*) [0x10388df72]
Warning Package maven-dubpackage not found for maven repository at http://localhost:3283/maven/release/dubpackages: Failed to download http://localhost:3283/maven/release/dubpackages/maven-dubpackage/maven-metadata.xml
Error No package maven-dubpackage was found matching the dependency 1.0.5
Error: :17 command failed
[ERROR] Script failure.Looks like it just needs retriggering.
rikkimax
commented
10 months ago Looks like I was wrong about the parent process environment, you need Config.newEnv for that. Oh well will add it because we kinda do need explicit control over all environment variables after this.
s-ludwig
commented
10 months ago Honestly, I find this approach rather strange from an outside point of view. Relying on this would mean that you can only execute the application using "dub run", without the special knowledge of setting LD_LIBRARY_PATH. To me it seems that either the application should make use of rpath for shared library that are loaded at application startup time, or modify the search path at runtime for any dynamically loaded libraries.
rikkimax
commented
10 months ago I'm not sure that there is a better approach to this problem.
Dub does not handle installation, so it shouldn't be assisting in setting RPATH which will need to be distro + linker specific.
This means every executable that uses a shared library dependency package on Posix is going to have to manually set RPATH in some way or have the user set up the environment variable.
Sure we can add a directive for setting RPATH to handle the linker side or have the user set the environment variable in the executable package, but that doesn't solve the distro paths problem which dub shouldn't be attempting to do.
Overall, we can't win. The best we can do is make dub run and dub test work out of the box, and document that if you run manually or handle installation you're on your own which is still a better situation than we have now.
rikkimax
commented
10 months ago Also worth considering that assuming knocker package has a shared library package dependency (it doesn't but its a simple example):
dub run knocker -- $PORTS
The successful execution of this will depend on whether you are running Windows or *nix.
Given that you're running a tool that isn't being developed that is quite a problem.
s-ludwig
commented
10 months ago What I mean is that an executable that expects shared libraries that are outside of the standard search paths to be linked at startup needs to set its rpath properly in order to be useful in the general case. It would be highly unusual to require the user to set LD_LIBRARY_PATH, irrespective of whether the application has been installed as a distribution package or not.
So, instead of adding the target path to LD_LIBRARY_PATH, the executable should instead have its rpath set to $ORIGIN. If DUB offers a way to build an application that way, then it should do that automatically.
s-ludwig
commented
10 months ago That also shouldn't hurt when the same executable gets distro packaged so that the libraries go into a standard library location instead of the executable directory, since rpath just adds to the normal search path.
rikkimax
commented
10 months ago Dub currently doesn't do anything to assist you with this.
It will likely be linker+target specific to perform.
So there is some concern that it does mean we end up marrying dub to unknown tool behavior that we do not control.
Given these facts, I'm not willing to implement it. To me at least it seems to be going outside of the bounds of dub and into installer territory which we have deemed out of scope.
However, if somebody wants to do their own PR to compete, I'm happy to allow them to take responsibility for it :)
s-ludwig
commented
10 months ago Okay, but that is the basic philosophy of DUB - abstract away platform specifics as far as possible. This just seems to be another linker flag generated depending on whether there are any shared library targets that a binary depends on, but maybe I'm missing something.
rikkimax
commented
10 months ago It is possible that setting RPATH could be as simple as adding a linker argument per compiler.
But it may also be possible that it is linker-specific, not just compiler or target.
I'm just not keen on the tradeoffs to the alternatives of this PR.
Perhaps @kinke is willing to take ownership of an alternative since they are the other 'owner' of shared library support in dub.
rikkimax
commented
10 months ago DUB projects relying on non-standard OS behavior, such as loading dynamic libraries from the project directory, should rather set
All dub projects that use shared libraries as dependencies are going to need some directives that look like that to make it both run or test.
This isn't a special case, it is ALL of them.
It also applies to quick and dirty executables just as much as it does to well packaged up ready-to-ship production software. We need a better solution here, that IMO does not require modifying the executable.
I am personally heavily against doing this without any user input, especially modifying/ignoring user's already set LD_LIBRARY_PATH environment variables from the recipe or environment variable. This will probably break most run scripts that already had this workaround in.
This only affects dub run and dub test, the user has given their input as to what they intend for it to do.
How can it break existing scripts? It prepends, not replaces. However, I can agree that it should append rather than prepend, that way anything the user has specified is tried first.
mengu
commented
8 months ago I'll second this request. I was building this example application from dlangui and all I wanted to do is run dub -v and have it running.
Relying on this would mean that you can only execute the application using "dub run", without the special knowledge of setting LD_LIBRARY_PATH.
that's not a terrible idea, that's a great idea. anyone who wonders how this or DYLD_LIBRARY_PATH works can dig into how static and dynamic linking works. please merge this PR.
s-ludwig
commented
8 months ago that's not a terrible idea, that's a great idea. anyone who wonders how this or DYLD_LIBRARY_PATH works can dig into how static and dynamic linking works. please merge this PR.
Stating that this is a "great idea" is not an argument and does not magically make it a true statement. Also, by that same logic, you can go and just look up how dynamic linking works and set your LD_LIBRARY_PATH appropriately before calling dub run.
mengu
commented
8 months ago that's not a terrible idea, that's a great idea. anyone who wonders how this or DYLD_LIBRARY_PATH works can dig into how static and dynamic linking works. please merge this PR.
Stating that this is a "great idea" is not an argument and does not magically make it a true statement. Also, by that same logic, you can go and just look up how dynamic linking works and set your LD_LIBRARY_PATH appropriately before calling
dub run.
It's not the same logic. Yours is an assumption. And it's an assumption on couple of things. In order to "go and just look up how dynamic linking works and set your LD_LIBRARY_PATH appropriately before calling dub run", one needs to:
dub run.What did the user want? They just wanted to try out an example.
s-ludwig
commented
8 months ago You don't need to know anything additional in the case of "dub run" over running the executable directly (apart from maybe the assumption that the parent environment gets inherited to the final process, which really is the norm). You previously dismissed the case where you run the executable directly with your "can look it up" argument, the same can be said for "dub run", simple as that.
If, for whatever reason, you decide to set up your package so that the examples require a strange dynamic library setup like that, then the first question would be what you are trying to solve that way. It surely isn't what dynamic libraries are usually meant to solve, as they are still all built together. Obviously, neither dynamic loading (plugins, which need to be looked up in a particular directory anyway), nor dynamic library updates (system package manager) are the reason.
Honestly, if anything, we need a proper solution here that results in a working executable/library combination. This is nothing but a hack that pushes the problem back by one step, without even hinting to a real solution.
rikkimax
commented
8 months ago @kinke you are the other owner of shared library support in dub, any opinions?
kinke
commented
7 months ago Heh, no I don't own anything in dub, except maybe for some workarounds and hacks I'm not too proud of. ;)
The problem here AFAICT is that we have to deal with 2 use cases - 1) a local-only build of some executable/shared lib, to be used on the dev box exclusively, and should 'just work', and 2) the case of an application/library bundle intended for distribution.
Now on Windows, there's not much room AFAIK - we generally need to copy all DLLs to the executable dir, in both cases.
On Posix however, the 1st case could be handled by
But for the redistributable bundle on Posix, AFAIK one normally makes sure the .so/dylibs have an SONAME, so that the DT_NEEDED ref in the executable is a pure filename without dirs, and then copy the libs next to the executable (or some other place), and bake that directory as RUNPATH into the executable. Setting/changing RUNPATH can be done by postprocessing via patchelf; but I think changing the DT_NEEDED entries to filenames-only isn't feasible or at least not similarly trivial (would have to patch the SONAME of the libs too?).
I'm not sure what the best way to handle both of these use cases is, and don't really have the time/motivation to think about it carefully.
rikkimax
commented
7 months ago Yes, my argument is that they should be handled with different solutions.
Without causing a surprise when dub sets up a default that is different from what the platform defines and has security consequences.
This PR only solves one of these issues, it doesn't need to solve both.
kinke
commented
7 months ago Oh sorry, now I actually took a glance at the PR. ;) - Hmm yeah setting appropriate env vars for apps run through dub doesn't sound too bad to me, to cover use case 1 without breaking use case 2.
Some thoughts:
dynamicLibrary deps?dynamicLibrary deps? Edit: To overcome having to copy all of those manually to the executable's directory via copyFiles.
rikkimax
commented
7 months ago
- Only do this for apps/test runners with
dynamicLibrarydeps?
That would be special casing, so when debugging builds you would need to question if a shared library was a dependency or not in dub.
Makes things more complicated without much value I think.
- Don't add the executable directory, but all output directories of all (direct + indirect)
dynamicLibrarydeps? Edit: To overcome having to copy all of those manually to the executable's directory viacopyFiles.
There possibly are desirable traits for this. However I am unsure of the wider implications of changing how files get copied after compilation.
kinke
commented
7 months ago Oh wait, we nowadays copy the dynamicLibrary artifacts to the executable's output dir automatically already, on Posix too. So yeah, nothing needed on Windows, and for non-Apple Posix here, prepending the executable dir to LD_LIBRARY_PATH should be enough.
If this is restricted to dub projects having (direct or indirect) dynamicLibrary deps, I'm pretty sure nothing changes for 99+% of all dub projects, but paves the way for dub run/test just working out of the box for projects with dynamicLibrary deps.
And use case 2 is covered by running patchelf to bake a suitable RUNPATH into the redistributable executable/.so/.dylib; all shared-library deps already copied to the output directory.
schveiguy
commented
3 months ago This doesn't set the LD_LIBRARY_PATH to include any dependency shared libs, right? It also does not work if the library is somewhere else.
I don't agree with this change. rpath is the better option, and that is doable using normal lflags.
I do this in my raylib-d library: https://github.com/schveiguy/raylib-d?tab=readme-ov-file#linking-instructions-in-dubjson
rikkimax
commented
3 months ago This doesn't set the LD_LIBRARY_PATH to include any dependency shared libs, right? It also does not work if the library is somewhere else.
All dependency shared libraries get copied into the same directory as the executable if they were built by dub.
It does not nor is intended to solve those found outside of dub.
rikkimax
commented
3 months ago Interestingly, Debian discourages RPATH entirely.
schveiguy
commented
3 months ago It also has bad things to say about LD_LIBRARY_PATH.
But in any case, automatically setting LD_LIBRARY_PATH is at best going to make the user unsure how to properly run the built executable ("I run it with dub, and it runs fine, but just running the exe itself doesn't!"), and at worst going to load the wrong libraries where users did not expect it. And when that happens, it will be quite the head scratcher. Especially if dub run doesn't behave the same as just executing the file.
I prefer the user manually set the LD_LIBRARY_PATH, or use rpath commands in the dub build.
See changelog for more information.
Of note (that was not mentioned), is that the environment variables were not inheriting from the parent process previously.
So while I was at it, I also added it back to the default. Most likely whoever implemented the setting of environment variables forgot that providing an AA to
std.processwill ignore the parent environment. I didn't find a ticket associated with this.EDIT: I would consider a test for this, but it's going to be extremely flaky, any change in build environment such as CI runner could change the result.