Installation with LWP < v6 can make SSL tests fail #88

Closed
ollyg opened this Issue Sep 25, 2013 · 13 comments

Projects

None yet

3 participants

@ollyg
ollyg commented Sep 25, 2013

Installing 0.4008 runs two tests for the new SSL functionality. On a system with LWP < version 6 they will fail because IO::Socket::SSL can be missing.

I asked on IRC and it was suggested that Starman should either depend on LWP::Protocol::https 6, or IO::Socket::SSL.

My personal preference is that SSL be a totally opt-in feature, and that SSL libs are not required for a basic installation.

If you need further details, please let me know.

@ap
ap commented Sep 26, 2013

I guess these tests should also skip if IO::Socket::SSL is not installed?

Also, right now IO::Socket::SSL is not listed as a dependency at all (because it doesn’t get detected by Dist::Zilla because it’s never even mentioned in the Starman code because Net::Server loads it for us if proto is ssl). I think that should be fixed by declaring an SSL feature in META and listing IO::Socket::SSL as its dependency – which I guess requires futzing the dist.ini.

Furthermore, the use of Test::Requires shows up as a testing dependency in META, but LWP::Protocol::https is not detected by Dist::Zilla either. If an SSL feature was declared in META then LWP::Protocol::https should also be listed there, as a testing dependency.

Yes?

@miyagawa
Owner

I guess these tests should also skip if IO::Socket::SSL is not installed?

LWP::Protocol::https has a hard requirement on IO::Socket::SSL, so there's no need to double check here.

it doesn’t get detected by Dist::Zilla because it’s never even mentioned

I use Milla, which doesn't use dzil's auto prereq gathering mechanism, intentinoally.

I think that should be fixed by declaring an SSL feature in META and listing IO::Socket::SSL as its dependency – which I guess requires futzing the dist.ini.

Maybe, but i'm not a big fan of optional dependencies declared like that, since it makes an undeterministic dependency behavior in the downstream.

Furthermore, the use of Test::Requires shows up as a testing dependency in META, but LWP::Protocol::https is not detected by Dist::Zilla either.

Again, there's no "detection" mechanism.

If an SSL feature was declared in META then LWP::Protocol::https should also be listed there, as a testing dependency.

if the "If" part is true then yes, but see above, I don't like declaring optional dependencies.

@miyagawa miyagawa closed this Sep 26, 2013
@ap
ap commented Sep 26, 2013

I'm not a big fan of optional dependencies declared like that, since it makes an undeterministic dependency behavior in the downstream.

Uhm, isn’t the behaviour already nondeterministic for downstream? How would declaring a feature make that worse?

Alternatively: what do we do to make it more deterministic? Create a mostly-dummy Starman::SSL distro with hard deps on Starman and IO::Socket::SSL for runtime and LWP::Protocol::https for testing, and move the SSL tests to the Starman::SSL distro with the skips removed?

@miyagawa
Owner

Uhm, isn’t the behaviour already nondeterministic for downstream?

It is deterministic in a way that depending on Starman doesn't install SSL related modules at all, and that you have to require them manually. Adding the features in theory would not make it "worse", but gives a hope that it could possibly be installed, which can not be relied on any way. So it doesn't make it anything "better".

Alternatively: what do we do to make it more deterministic? Create a mostly-dummy Starman::SSL distro with hard deps on Starman and IO::Socket::SSL for runtime and LWP::Protocol::https for testing

Yes, that's close to what David Golden, rjbs and folks at Lancaster agreed as a good practice, although not documented clearly yet - making a "feature bundle" for each group of deps and make a separate distribution for them.

@ap
ap commented Sep 26, 2013

What about the tests?

@miyagawa
Owner

I think Starman::Feature::SSL should not rely on Starman since it will make a circular dependency. I'd like to see a good practice documented somewhere soon.

@miyagawa
Owner

Tests can be moved to a separate dist, or stay there, i don't care much.

@ap
ap commented Sep 26, 2013

Why would that be a circular dep, would Starman depend on Starman::Feature::SSL? That seems strange to me, why would it? Wasn’t the whole point that the SSL deps are optional? If Starman::Feature::SSL (hm, I don’t know if I like the ::Feature in there) depended on Starman but not vice versa, you could just depend on Starman::SSL instead of Starman (or in addition to Starman) and the CPAN installer would always DTRT. If it doesn’t, then you get the deps without the module (the bones without the meat?).

@miyagawa
Owner

would Starman depend on Starman::Feature::SSL? That seems strange to me, why would it?

It will list Feature::SSL as an optional feature dependency. So that clients that support features will prompt users if they want to install them, and downstream can depend on Starman + Starman::Feature::SSL to make sure SSL support is required.

@miyagawa
Owner

you could just depend on Starman::SSL instead of Starman (or in addition to Starman)

I think downstream should depend features in addition to Starman. Right now SSL is the only feature, but when you have a bunch of features, depending multiple of them (without Starman) sounds a little odd, while Starman + zero or more feature bundles sounds saner.

Of course I understand your argument, you can argue one way or the other pretty easily.

@ap
ap commented Sep 26, 2013

OK, I’ve left a reply over there to see how people think about that.

@ap
ap commented Sep 26, 2013

when you have a bunch of features, depending multiple of them (without Starman) sounds a little odd

I agree. You don’t have to leave it out though… you can list it if you want. It’s just that, why should cpanm Starman::Feature::SSL refuse to be helpful when it knows what I’m trying to do? Anyway, I’ve mentioned that in the other thread to see what people think.

@miyagawa miyagawa referenced this issue in Perl-Toolchain-Gang/CPAN-Meta Sep 26, 2013
Closed

for optional_feature, soften "discourage" into "take caution" #29

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment