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

Fails if Plack version == 1.0040 and CGI::Deurl::XS is not installed #3

Closed
andk opened this issue Apr 9, 2016 · 9 comments
Closed

Comments

@andk
Copy link

andk commented Apr 9, 2016

As per subject.

Sample fail report:

http://www.cpantesters.org/cpan/report/67327196

I spotted the connection by chance on my smokers. Since you're running through a maze of optional dependencies I'd recommend the trick I'm using in some of my own Makefile.PLs: look up which optional dependenies are installed and declare those they are as prerequisites. This will not disturb the end user but will enrich the report and then analysis.cpantesters.org will automatically spot the connection that I expressed in the subject line

HTH&&Thanks,

@dboehmer
Copy link
Owner

dboehmer commented Apr 9, 2016

Thank you @andk, I already received your smoker test reports and looked into the issue. I think it could be an issue with Dancer2 as it uses a private method of Plack::Request that's gone with 1.0040. See PerlDancer/Dancer2#1154

As for your workaround: I am using Dist::Zilla to generate the distribution. I need to look for a solution that will work with DZ.

@andk
Copy link
Author

andk commented Apr 9, 2016

On Sat, 09 Apr 2016 06:10:57 -0700, Daniel Böhmer [email protected] said:

  > As for your workaround: I am using Dist::Zilla to generate all the
  > distribution. I need to look for a solution that will work with DZ.

Yupp, sorry, I hadn't spotted that. For dzil this is quite similar in
effect, albeit hardly explored and not yet battle-tested:
https://metacpan.org/source/ANDK/Encode-MAB2-0.09/t/00module_versions_report.t

You would (1) NOT declare a dependency on Module::Versions::Report; (2)
add a test with exactly this name 00module_versions_report.t, the name
turns on the attention of analysis.cpantesters.org; (3) copy the test
from mine and replace lines 17 and 18 with just enough own code to
trigger all of your modules that might load optional dependencies;

The idea is that the end user will not get noise unless they have
Module::Versions::Report installed which is rarely used; the end user
will not be forced to install a single bit more than before; cpantesters
and analysis will see the inventory of "interesting modules". And you
have full control over the code that loads them.

Disadvantage of this is that you have to include it before the problem
happens that you just encountered. Oh well, we're working on that too.

andreas

@dboehmer
Copy link
Owner

dboehmer commented Apr 9, 2016

I don't mind having a file with a 00 prefix to make it execute first.

I looked at your template and much of it, especially the comments, seem to include some amount of magic:-) Is the following the correct adaption of your code?

# -*- mode: cperl -*-

use strict;
BEGIN {
    my $exit_message = "";
    unless (eval { require Module::Versions::Report; 1 }) {
        $exit_message = "Module::Versions::Report not found";
    }
    if ($exit_message) {
        $|=1;
        print "1..0 # SKIP $exit_message\n";
        eval "require POSIX; 1" and POSIX::_exit(0);
    }
}

use Test::More tests => 1;
use Plack::Request; # <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< only changed lines
use CGI::Deurl::XS; # <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
diag(Module::Versions::Report->report);
pass;


# Local Variables:
# mode: cperl
# cperl-indent-level: 4
# End:

Do you know of any other module using this mechanism? Would you appreciate having a Dist::Zilla plugin providing such a test file? I'd rather write such a plugin than duplicating so much magic code and maintaining it at several places.

@andk
Copy link
Author

andk commented Apr 10, 2016

On Sat, 09 Apr 2016 13:16:29 -0700, Daniel Böhmer [email protected] said:

I don't mind having a file with a 00 prefix to make it execute first.
I looked at your template and much of it, especially the comments,
seem to include some amount of magic:-)

Sorry for that:(

Is the following the correct
adaption of your code?

I don't think so. But maybe I'm missing something. Let me suggest to
take the discussion elsewhere, we have moved away too far from the
original ticket. I hope we can talk a bit about it soon.

andreas

@dboehmer
Copy link
Owner

dboehmer commented May 2, 2016

Dancer2 folks seem to be fixing this. See PerlDancer/Dancer2#1154 (comment)

Also I looked a for a Dist::Zilla based module that will report versions for the modules in question and found . Could you please inspect release 0.003_001?

I don't remember which output formats analysis.cpantesters.org supported. I'd like to that a look into that code if you were as kind as pointing me to where that site's code is or (even better) where the modules versions are parsed.

@andk
Copy link
Author

andk commented May 3, 2016

The link I gave above in the third entry on this page, shows my admitteldy clumsy attempt in Encode::MAB2.

Since then I've written and released a substantially reduced approach: https://metacpan.org/source/ANDK/CPAN-Testers-ParseReport-0.3.0/t/00-load.t

An early brainstorming discussion about the idea took place at dagolden/Test-DiagINC#1

The point is, we want everything that is influences the tests, not more, not less. Having only first level dependencies does not help if the culprit is in a second level dependency. Having only declared prerequisites does not help when the culprit is not declared. Having more than the actually involved modules may have the effect of sand in the eyes. We need the same amount of data for passing and failing reports because we have to focus on the differences. This issue here is a good example: we want a single test that reports both Plack's version and CGI::Deurl::XS's by educated sample code. Often, but not always, loading all of the modules in a "META.yaml/provides" section will do.

If all this does not help, can you rephrase your question, maybe I misparse it (what did you find and what should I inspect?)

@dboehmer
Copy link
Owner

dboehmer commented May 4, 2016

I was trying to ask you to run the tests for version 0.003_001 and look into the test output if you can find the desired module versions in a parseable format.

For example have a look at http://www.cpantesters.org/cpan/report/68361185. The modules listed in the issue's title are now listed in the test report. I guess only the output format is not yet supported as I don't see the module versions listed as a "regression" on analysis.cpantesters.org.

From our conversion in Berlin I learned that only I as the module maintainer can compile the list of "interesting modules". We have that list and their versions now. I think the next step would be to support the format of Dist::Zilla::Plugin::Report::Versions or use a different format. Can you please post a link to supported formats or the code supporting them?

@andk
Copy link
Author

andk commented May 4, 2016

On Wed, 04 May 2016 09:34:08 -0700, Daniel Böhmer [email protected] said:

I was trying to ask you to run the tests for version 0.003_001 and
look into the test output if you can find the desired module versions
in a parseable format.

Sorry, didn't understand the question, but now do. Answer below.

For example have a look at
http://www.cpantesters.org/cpan/report/68361185. The modules listed in
the issue's title are now listed in the test report. I guess only the
output format is not yet supported as I don't see the module versions
listed as a "regression" on analysis.cpantesters.org.

No output format supported besides Module::Versions::Report. M:V:R
should not be a prereq, it's sufficient that some smokers have it.

From our conversion in Berlin I learned that only I as the module
maintainer can compile the list of "interesting modules". We have that
list and their versions now. I think the next step would be to support
the format of Dist::Zilla::Plugin::Report::Versions or use a different
format. Can you please post a link to supported formats or the code
supporting them?

The two links on this page that show examples, with line numbers:

https://metacpan.org/source/ANDK/Encode-MAB2-0.09/t/00module_versions_report.t#L7

https://metacpan.org/source/ANDK/CPAN-Testers-ParseReport-0.3.0/t/00-load.t#L8

Especially the second one doesn't look so difficult to understand, no?.
May I state the goals once again? What I believe we need, are solutions
that

  • work (no gotchas)
  • are easy to understand (not confusing the developer base)
  • are not limited to a subset of the cpan devs (eg. Dist::Zilla)
  • do not disturb the end user (no extra prerequisites)

Thanks,

andreas

@dboehmer
Copy link
Owner

I used Dist::Zilla::Plugin::Test::ReportPrereqs and won't change that any time soon.

  • work (no gotchas)

It works.

  • are easy to understand (not confusing the developer base)

I added a comment to the entry in the dist.ini. It's only 3 lines there and the generated test file contains comments explaining the source of the file.

  • are not limited to a subset of the cpan devs (eg. Dist::Zilla)

The generated test file can be run by everyone and I feel perfectly comfortable with handling this with Dist::Zilla.

  • do not disturb the end user (no extra prerequisites)

I think that is applicable. I ran a test and it reports the version or the absence of modules.

dboehmer added a commit that referenced this issue Oct 27, 2016
dboehmer added a commit that referenced this issue Oct 31, 2016
dboehmer added a commit that referenced this issue Nov 10, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants