-
-
Notifications
You must be signed in to change notification settings - Fork 227
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
skip optional packages when not used #1148
base: master
Are you sure you want to change the base?
Conversation
Thanks a lot for taking the time to look into this, as well as writing up an explanation! I've just briefly verified the problem and adhoc I'm not sure about what is the best solution. Making the set of selections generally configuration dependent would severely reduce the value of this feature, though, because reproducible dependency trees wouldn't be possible across configurations/platforms anymore. As far as I see it, this is a fundamental issue caused by the notion of how an optional package gets chosen (i.e. by just listing it in dub.selections.json) and no algorithmic fix can provide a sufficient solution. Extending the format to make this more explicit (e.g. |
Thanks for looking into this. You are right about the fact that my fix makes the selections file configuration dependent. That behaviour is most unwanted. I overlooked that. Adding a chosen flag does indeed seem to work, as far as I can tell. Will see if I can write a new pr. Hope to get it done by the end of next week. |
FWIW, from the point of view of someone relatively new to dub and D who has been hit with this slightly obscure issue, I have struggled to understand the details, and it has taken a lot of experimentation to fully understand what is going on. For me, this proposed solution is convoluted and difficult to follow and seems to make an already counter-intuitive system even more so. But it does seem that it would solve the problem. The idea of having to configure dub.sdl/json, then build the project so that dub.selections.json is created, then edit the dub.selections.json file to select optional dependencies (either by deleting selected dependencies or by adding a 'chosen' flag) just seems awkward and non-intuitive. Would it not seem more logical that all manual settings and overrides be placed in dub.sdl/json, leaving dub.selections.json be created entirely automatically, never to be edited by hand. In that regard, would it not be clearer to allow dependencies to be disabled in dub.sdl, for example by specifying I'm fully expecting someone who understands this better than me to give a simple reason why this would be undesirable and I apologise in advance for wasting your time! |
This pr is indeed sub-par. I wanted to change as little as possible while still fixing the issue. And, as Sönke observed, this pr also ties the dub.selections.json to a specific configuration. Which is not wanted at all. I like your idea to avoid editing the dub.selections.json file manually. The only responsibility of that file should be to control versions. While I am thinking more about this I think the root problem is not the optional dependency itself, but the default optional dependencies. I think that default optional dependencies is a wrong feature. Take vibe-d as an example. If the user doesn't explicitly state in the dub.json to include openssl/botan, I think none of them should be used. And vibe should revert to no ssl support. On the other hand, I suspect that default optional dependencies exists to allow easy setup of a vibe-d project with all functionality available (https/etc). @s-ludwig what do you think? |
The question of editing dub.selections.json manually is basically orthogonal to the question of storing an explicit "chosen" field. The goal would be in any case to have some way to do this using the CLI and not having to edit the file (e.g. But one area where the "chosen" idea doesn't really play well is for optional dependencies that are buried in the dependency graph. Something like #1148 (comment) in addition to the current approach, augmented with an explicit "chosen" flag, would surely lead to a more complete approach. There is also the idea of introducing additive "features" in addition to exclusive "configurations". Those could be a better target for an "optional default" functionality. Possibly optional dependencies could be faded out completely long-term when such a system exists. The only drawback is that this couldn't be used to gracefully handle dependencies that don't exist, which optional dependencies can. |
I like @arpie42 About the Sorry for going so much into detail, but I see a small edge case where I have 2 packages that both have the same optional dependency, but I only want one of the packages to use it. Otherwise it seems like a good idea. As long as we also have your proposed dub cli command to unset the chosen flags. |
This PR fixes dub pulling in an optional dependency when it is not used.
I was having problems getting vibe-d to compile on arch linux (see vibe-d/vibe.d#1769), because the deimos openssl bindings target an old and obsolete openssl version that is no longer on arch linux. When trying to switch to botan and avoiding the (optional) openssl I found out that dub was still pulling in openssl and compiling vibe-d with Have_openssl (which in turn causes vibe-d to import openssl).
Here follows some background on optional dependencies, or at least how I understand them.
Dub supports optional dependencies. These dependencies should not get compiled unless the end-user explicitly puts them in its dub.selections.json. There are also default optional dependencies, which will, when the end-user doens't have a dub.selections.json, automatically store the default optional dependency into the dub.selections.json.
So, vibe-d (or the vibe-d:stream subpackage) has a default optional dependency on openssl, and an optional dependency on botan. This means the end-user (me) can choose whether to depend on openssl or on botan. Since I had trouble with openssl on arch linux, I decided to use botan. And the problem described above presented itself.
Here follows an account of me digging through dub's codebase and trying to understand what happens:
When dub collects the transient dependencies, dub will do it for ALL configurations. Which I think is because it doesn't know which configuration it will use yet (chicken-egg problem).
As it happens botan has a subconfiguration with a dependency on openssl. I am not using that subconfiguration but dub regardlessly will append it to its list of dependencies. (And I suppose it has too. But I am not sure.)
At which point dub realises I have a missing dependency and will start to upgrade my project. Here dub fetches openssl, adds it as a dependency to my dub.selections.json and writes the file to disk.
Then it starts its configuration reduction. Here it correctly determines that botan isn't actually using the subconfiguration with openssl, and drops it. But when it gets to vibe-d:stream it notices the openssl dependency in the dub.selections.json and makes the once optional dependency non-optional. Afterwards it will start the build generator and will start building all targets.
As for the solution in the PR, I am not really pleased with it, to put it very mildly. On the other hand, I could not see any other solution besides refactoring existing code.
Here is the breakdown of the changes I felt I had to made.
Initially dub doesn't know which configuration is used for a dependency, so when collecting dependencies for a package it collects ALL dependencies from ALL sub-configurations. After it has successfully selected one configuration for each dependency, I modified dub to redo the collection of dependencies, but this time with the proper config (so it will know which subconfigs - and therefor deps - it can skip).
I wanted to fix the upgrade code, but when dub executes the upgrade code, the configurations aren't available. So, instead I settled for purging any added unused dependency after configuration is determined. (And then write dub.selections.json again). This has the downside that doing
dub upgrade
will STILL pull in any optional dependency (when that dependency is used in a subconfiguration of another dependency) and then writes that list of dependencies to dub.selections.json.There is a lot more to say on the subject, and I feel I haven't expressed everything clearly.
Regardless, this PR does fix the problem I have, but I wonder at what cost...