Fix recommend support #52

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
5 participants
@ghost
Contributor

ghost commented Oct 12, 2013

This implements the requires/recommends support that was reverted earlier. CPAN.pm's tests pass for me.

I also tested manually with these config options:

'build_requires_install_policy' => q[no],
'halt_on_failure' => q[1],
'recommends_policy' => q[1],
'suggests_policy' => q[1],

Of note, optional failures do not trigger halt_on_failure and required dependencies of optional dependencies stay optional and their failures do not cause problems either.

Here is an example of a live test with lots of recommends/suggests including some with intentional failures:

cpan[1]> test CPAN::Test::Dummy::Perl5::RecSug Path::Tiny
... [lots of stuff] ...
Result: PASS
  DAGOLDEN/CPAN-Test-Dummy-Perl5-RecSug-0.001.tar.gz
  /usr/bin/make test -- OK
... [lots of stuff] ...
Result: PASS
  DAGOLDEN/Path-Tiny-0.041.tar.gz
  /usr/bin/make test -- OK
Failures during this command:
 (optional) DAGOLDEN/CPAN-Test-Dummy-Perl5-Make-FailLate-1.02.tar.gz: make NO
 (optional) DAGOLDEN/CPAN-Test-Dummy-Perl5-Make-PerlReq-Fail-0.001.tar.gz: writemakefile NO '/Users/david/perl5/perlbrew/perls/perl-5.18.1/bin/perl Makefile.PL' returned status 65280
 (optional) ANDK/CPAN-Test-Dummy-Perl5-Build-Fails-1.03.tar.gz: make_test NO  

xdg added some commits Apr 2, 2012

First cut implementation of recommends/suggests support
When failures are reported, any optional prerequisites
have "(optional)" prepended to the line to clearly
indicate the nature of the failing distribution
avoid stopping queue on optional failures
This refactors the CPAN::Shell 'failed' method so that the queue
running can use the same logic to find out whether failures were
on a mandatory module or optional modules.

Prior to this change, a mandatory module could succeed, but
halt_on_failure would still stop because there were earlier optional
failures.
Reword failure message for clarity
If mandatory modules succeed but optional modules fail, the old text
looked a bit like "PASS..." followed by "Failed during this command:" which
caused me to have a bit of a double-take.  I reworded "Failed during"
to "Failures during" so it's clearer that it's additional information.
@ghost
Contributor

ghost commented Oct 13, 2013

I have invited the community to help test this branch.

mjegh commented Oct 14, 2013

As it happens I was just thinking of adding a recommends to a module so I thought I'd try it out. My META.yml says:

recommends:
Test::Version 1.002001

I installed your new CPAN and set the configs:

cpan[2]> o conf recommends_policy
recommends_policy [1]

perl 5.18.0.

$ perl -MTest::Version -le '1;'
Can't locate Test/Version.pm in @inc (you may need to install the Test::Version module) (@inc contains: /home/martin/perl5/perlbrew/perls/perl-5.18.0/lib/site_perl/5.18.0/i686-linux /home/martin/perl5/perlbrew/perls/perl-5.18.0/lib/site_perl/5.18.0 /home/martin/perl5/perlbrew/perls/perl-5.18.0/lib/5.18.0/i686-linux /home/martin/perl5/perlbrew/perls/perl-5.18.0/lib/5.18.0 .).
BEGIN failed--compilation aborted.

cd into git master of my module and did a cpan .:

martin@bragi:~/git/DBD-ODBC$ cpan .
You are visiting the local directory
'.'
without lock, take care that concurrent processes do not do likewise.
CPAN: Time::HiRes loaded ok (v1.9725)
CPAN: Storable loaded ok (v2.41)
Reading '/home/martin/.cpan/Metadata'
Database was generated on Mon, 14 Oct 2013 08:09:43 GMT
You are visiting the local directory
'/home/martin/git/DBD-ODBC/.'
without lock, take care that concurrent processes do not do likewise.
/home/martin/git/DBD-ODBC/.
Has already been unwrapped into directory /home/martin/git/DBD-ODBC/.
CPAN: Parse::CPAN::Meta loaded ok (v1.4404)
CPAN: YAML loaded ok (v0.84)
CPAN: CPAN::Meta loaded ok (v2.120921)
CPAN: Module::CoreList loaded ok (v2.90)
Configuring /home/martin/git/DBD-ODBC/. with Makefile.PL


Remember to actually *READ* the README file!
And re-read it if you have any problems.

OSNAME: linux
LANG: en_GB.UTF-8
ODBCHOME:
LD_LIBRARY_PATH:
DBROOT:
WINDIR:
II_SYSTEM:
Perl: 5.018000
ExtUtils::MakeMaker: 6.66
Command line options:
o=s =
w! = undef
x! = undef
u! = undef
g! = 0
e! = undef

Your LANG environment variable is set to "en_GB.UTF-8"
This is known to cause problems in some perl installations - even stopping
this Makefile.PL from running without errors. If you have problems please
try re-running with LANG unset or with the utf part of LANG removed.

I've probably done something wrong but I thought I'd let you know anyway.

Martin

@ghost
Contributor

ghost commented Oct 14, 2013

@mjegh did you commit the change to the configuration variable with o conf commit?

mjegh commented Oct 14, 2013

Yes, that output from o conf was after commit and exiting cpan shell and going back in again. I just double checked.

mjegh commented Oct 14, 2013

BTW, I'm on #toolchain right now and during working hours (UK) most days as mje, or mje_ etc

@ghost
Contributor

ghost commented Oct 14, 2013

For the record: we got it sorted out and working via IRC discussion

@ghost
Contributor

ghost commented Nov 10, 2013

@andk, have you had a chance to review/test this?

David

Owner

andk commented Nov 10, 2013

David Golden notifications@github.com writes:

@andk, have you had a chance to review/test this?

Sorry, not at all yet:(

Could you describe your experiences with your testing strategies in
day-to-day usage? Maybe it helps me to discover a low-risk adaption
process on my smoker. The problem is that a smoker runs the risk of
damaging too much but that only a smoker can really find something.

andreas

@ghost
Contributor

ghost commented Nov 22, 2013

The major issue I'm hitting is that there are many instances of circular dependencies caused by recommends and CPAN.pm doesn't know how to break the cycle.

I think that's correct as written -- recommendations shouldn't be creating circularity -- but it's hard to do widespread testing when it's so common. We need module authors to fix that. And they won't fix it until recommend support is is in widespread release and causing problems.

Owner

andk commented Nov 22, 2013

Circular recommendations should be fine, isn't that the whole point of
recommendations? You don't have to follow them. If you do, then it's
your own fault. What am I missing?

Can you give me an example?

andreas

@ghost
Contributor

ghost commented Nov 22, 2013

ETHER/Moose-2.1005.tar.gz                    : make NO cannot resolve circular dependency
(optional) FLORA/Devel-PartialDump-0.15.tar.gz: make NO cannot resolve circular dependency

Right now, failures from recommends/suggests are allowed (i.e. won't trigger halt on failure), but I don't know how to stop them from being circular. The recursive failure gets set in color_cmd_tmps (which is a function that I've never fully understood) so maybe something there can stop recursing but non fatally for an optional dependency.

Owner

andk commented Dec 10, 2013

I have added three things to the branch: circular dependency detection is now turned off for optional items; execution order is changed to run optionals after mandatories; a test in 31sessions to verify it works as intended;

I have not made friend with commit 89048f2. If we need a rewording, I'd prefer one that adds clarity instead of taking it away, which seems to me is the case. So, maybe let's discuss what the intent was and then find out how to achieve it.

I've cherry-picked the other two commits into the trunk together with my own changes. My testing so far was limited mostly to what the newly written test is doing. I'd welcome if you could review what we have now.

Thanks a lot for the sterling work, this was quite a tough exercise.

@ghost
Contributor

ghost commented Dec 10, 2013

On Tue, Dec 10, 2013 at 7:34 AM, andk notifications@github.com wrote:

I have added three things to the branch: circular dependency detection is
now turned off for optional items; execution order is changed to run
optionals after mandatories; a test in 31sessions to verify it works as
intended;

Thank you. I wasn't sure how best to tackle those. I've managed to get
Mooses circular recommends dependency fixed thanks to @karenetheridge so
hopefully we'll see fewer pathological cases anyway.

I have not made friend with commit 89048f289048f2.
If we need a rewording, I'd prefer one that adds clarity instead of taking
it away, which seems to me is the case. So, maybe let's discuss what the
intent was and then find out how to achieve it.

It seemed clearer to me, but that could be a subtlety of native English
or just my own peculiarity. I think what I was guarding against is a
situation I saw where I got "failed during this command: @BLAH" followed by
a success message.

So I wanted to avoid it being read as a declaration of failure and instead
wanted it read as a report of information.

I.e. "[These] failed during this command:" versus "[Here are] failures
during this command:"

Just brainstorming, perhaps it could be: "Some steps failed during this
command:"

I don't feel very strongly about it, so if it's reverted, I won't cry over
it. :-) Or maybe we just need more hands to paint the bikeshed and see if
a consensus wording emerges.

I've cherry-picked the other two commits into the trunk together with my
own changes. My testing so far was limited mostly to what the newly written
test is doing. I'd welcome if you could review what we have now.

Thanks a lot for the sterling work, this was quite a tough exercise.

You're welcome. Sorry I screwed it up the first time. Now that Moose's
recommendation is no longer circular, I'm going to try to use it more
regularly again for my "install everything against monthly bleadperl
releases" and see if any more issues crop up.

David

David Golden xdg@xdg.me
Take back your inbox!http://www.bunchmail.com/
Twitter/IRC: @xdg

Owner

andk commented Dec 10, 2013

David Golden notifications@github.com writes:

So I wanted to avoid it being read as a declaration of failure and instead
wanted it read as a report of information.

I.e. "[These] failed during this command:" versus "[Here are] failures
during this command:"

Just brainstorming, perhaps it could be: "Some steps failed during this
command:"

I don't feel very strongly about it, so if it's reverted, I won't cry over
it. :-) Or maybe we just need more hands to paint the bikeshed and see if
a consensus wording emerges.

It's not the wording, it's the change. For mandatory installations it
has always been a declaration of failure and should continue to be. I do
not want to change a message that is correct. If the change you propose
is only made for optional modules, that's fine by me. I was alerted by
the fact that a test had to be changed. For a new feature a new wording
is good. I haven't yet made up my mind how to present failures during
optional modules. That's one reason why I wrote "experimental" in the
Changes file.

andreas

@ghost
Contributor

ghost commented Dec 10, 2013

On Tue, Dec 10, 2013 at 11:03 AM, andk notifications@github.com wrote:

It's not the wording, it's the change. For mandatory installations it
has always been a declaration of failure and should continue to be. I do
not want to change a message that is correct.

Because a command can have a list of arguments for the queue, failure isn't
binary. Even before recommends/suggests, one could have partial failures.
One can even have failures in prereqs that don't actually stop the command
queue from completing successfully. (Which is why 'halt_on_failure' got
added for people who wanted an absolute stop on the first error, however
deep.)

So the change in wording was to indicate that "failures occurred during a
queue run" (some of which are now explicitly "optional") distinct from
"this command failed".

As I said, it's subtle and I don't feel strongly enough to argue it further
if you disagree. My bikeshed is green. :-)

David

David Golden xdg@xdg.me
Take back your inbox!http://www.bunchmail.com/
Twitter/IRC: @xdg

Contributor

karenetheridge commented Dec 10, 2013

Some other dists to try are listed here: https://gist.github.com/miyagawa/5516279

Owner

andk commented Dec 11, 2013

David, it seems that CPAN::Reporte is incompatible with this CPAN.pm now. I just wrote a local report with CPAN-Reporter-1.2010 and it contains this section:

Prerequisite modules loaded:

requires:

Module Need Have


! build_requires HASH(0xd324940) n/a
! opt_build_requires HASH(0xd324658) n/a
! opt_requires HASH(0xd3244f0) n/a
! requires HASH(0xd3248b0) n/a

Are you aware of that? Does it ring a bell?

@ghost
Contributor

ghost commented Dec 11, 2013

Makes sense. The return value of prereq_pm probably changed and CPAN::Reporter is tightly coupled to the CPAN::Distribution API.

Pinging @garu to see if he can attempt a quick fix.

Owner

andk commented Dec 28, 2013

David, I'd like to take your argument up you made in #52 (comment)

You said: One can even have failures in prereqs that don't actually stop the command queue from completing successfully.

Maybe you know something I don't. Please bring it to a separate ticket or point me to one if it exists already. My intention is to present the user a concise summary of fails before the next prompt appears. If we don't do that, I want to fix it, so we need a separate ticket.

Thanks!
(And an aside to this ticket: I would like that the recommends/suggests support comes in a way that keeps the tradition of concise end-of-command summaries. I don't know yet if we are there.)

@ghost ghost closed this Mar 13, 2014

@ghost ghost deleted the dagolden:topic/fix-retry-recommend-support branch Mar 13, 2014

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