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

No check optional deps #832

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

cyisfor
Copy link

@cyisfor cyisfor commented Apr 29, 2016

Avoid stalling on polling online servers for versions of dependencies that we aren't going to use. Related to #831

Not sure if I did this right... works for stopping optional deps, but I don't have an optional dependency to actually select, to test if that sets depType to required early enough.

@MartinNowak MartinNowak added this to the 0.9.25 milestone May 1, 2016
package_indices[basepack] = pidx;
package_names[pidx] = basepack;
if (basepack == root_base_pack) configs = [root.config];
else if(ch.depType == DependencyType.required)
Copy link
Member

Choose a reason for hiding this comment

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

Looking at

dub/source/dub/dub.d

Lines 1218 to 1227 in 9a37109

if (m_options & UpgradeOptions.upgrade || !m_selectedVersions || !m_selectedVersions.hasSelectedVersion(dbasename)) {
// keep deselected dependencies deselected by default
if (m_selectedVersions && !m_selectedVersions.bare && dt == DependencyType.optionalDefault)
dt = DependencyType.optional;
ret ~= TreeNodes(d.name, d.spec.mapToPath(pack.path), dt);
} else {
// keep already selected optional dependencies if possible
if (dt == DependencyType.optional) dt = DependencyType.optionalDefault;
ret ~= TreeNodes(d.name, m_selectedVersions.getSelectedVersion(dbasename), dt);
}
, this should be ch.depType != DependencyType.optional.

Copy link
Member

Choose a reason for hiding this comment

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

Also there should be a space after the if.

@s-ludwig
Copy link
Member

s-ludwig commented May 2, 2016

Looks like we overlooked a valid case for the meta data cache. I'm not fully sure about it in general, yet, but one issue with this change is that it doesn't work correctly in all cases (in particular the case when there is a second non-optional dependency to the same package must be handled).

@MartinNowak
Copy link
Member

Can we get the configs lazily once an optional package is required as dependency?

@MartinNowak
Copy link
Member

Should we defer resolution of this bug until we rework the dependency resolver?

@s-ludwig
Copy link
Member

Yes, let's defer this to 1.0.0. It's annoying with no/slow internet connection, but can be worked around with the --nodeps switch. 1.0.0 is scheduled for June 15, so 0.9.25 needs to get out ASAP. Do you think that you'll have time to implement the dependency resolver until then (minus beta phase)? But my hope is that this won't change the public API, so that it could also wait for 1.1.0.

@s-ludwig
Copy link
Member

s-ludwig commented Jun 2, 2016

I've fixed the general #831 issue now with a different approach: #861 - However, this one may still be good to have, but I'll have to think this through in-depth to be sure about all side-effects.

@cyisfor
Copy link
Author

cyisfor commented Aug 27, 2016

@s-ludwig Well, thanks for fixing it. I don't care what approach as long as it works. I just wrote this attempt to avoid being a totally useless lump over in the issue section.

@s-ludwig
Copy link
Member

Definitely appreciated! My fix doesn't catch all the cases that this one does, so if it doesn't accidentally break something, it makes sense to integrate it.

@s-ludwig
Copy link
Member

I just thought of an issue with this approach: If the same package is referenced twice, once optional and once non-optional, if the optional dependency is visited first, it will end up with no version candidates, even though there is the other non-optional dependency. This could probably be fixed with a purely_optional_packages set and some twisting of the program logic, but it raises the question of the additional complexity still being worth it.

@MartinNowak
Copy link
Member

MartinNowak commented Sep 21, 2016

One could try to overwrite the configs of earlier optional packages of they are required later on.
Using a hash table for all_configs might be enough to do that.

I don't think we support resolving an optional dependency conflicting with a mandatory dependency of the same package.

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.

3 participants