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

>= 4.28_01 declares testing dependencies as runtime dependencies #207

Closed
kentfredric opened this issue Jul 19, 2016 · 15 comments
Closed

>= 4.28_01 declares testing dependencies as runtime dependencies #207

kentfredric opened this issue Jul 19, 2016 · 15 comments

Comments

@kentfredric
Copy link
Contributor

https://metacpan.org/diff/file?target=LEEJO/CGI-4.28_01/&source=LEEJO/CGI-4.28/

Of particular note is the growth of runtime.requires in META.json and the removal of TEST_REQUIRES in Makefile.PL

This is a regression for end users who use CPAN Clients that support --no-testing, as it means they can no longer elide test dependencies.

The preferred solution here is to tweak the authoring stage to inject the contents of the old TEST_REQUIRES hash into META.json as test.requires ( Which is presently not declared in your META.json: https://metacpan.org/source/LEEJO/CGI-4.31/META.json#L23-56 )

@karenetheridge
Copy link

karenetheridge commented Jul 19, 2016

The preferred solution here is to tweak the authoring stage to inject the contents of the old TEST_REQUIRES hash into META.json as test.requires

That would undo the intent of the commit that is responsible for the change: cc18e3a#diff-5d3ba18294715d9415e9e732852bfec6

The premise behind this commit is flawed. The correct action is to revert this commit, and either ignore the invalid cpantesters reports, or to pursue the issue with the smoker and fix it there. However this sounds like it could be the same issue recently explored on the cpantesters.discuss mailing list.

@kentfredric
Copy link
Contributor Author

Part of the problem is that META.* does NOT declare the testing dependencies at 4.28

https://metacpan.org/source/LEEJO/CGI-4.28/META.json

But it aught to.

I agree TEST_REQUIRES should be used, and that the problem commit should be reverted.

But the test dependency should also be visible in test.requires, but it is not.

@karenetheridge
Copy link

Of course it doesn't, because the directive to do so was removed in the commit that I linked. A new release with that commit reverted will solve the issue.

@kentfredric
Copy link
Contributor Author

4.28 happened before that commit. That commit breaks 4.28_01

But 4.28_01 lacks test.requires

https://metacpan.org/source/LEEJO/CGI-4.28/Makefile.PL#L31 <- TEST_REQUIRES
https://metacpan.org/source/LEEJO/CGI-4.28/META.json # Test dependencies missing
https://metacpan.org/source/LEEJO/CGI-4.28/META.yml # Test dependencies missing

@karenetheridge
Copy link

Look at the commit topology -- it's not linear.

: [ether@jaeger git/CGI]$; git tag --contains cc18e3a292a6c06b0fc0bbf4b10e9efedf713224
v4.29
v4.30
v4.31

Unfortunately there are no v4.28 or v4.28_01 tags so it's not immediately obvious at what commit the 4.28_01 commit was made, but it's possible to guess within a few commits.

@kentfredric
Copy link
Contributor Author

kentfredric commented Jul 19, 2016

Commit dc3abc5 introduced

META_ADD => {
   build_requires => {},
   configure_requires => {},
}

And this logic is still there, breaking dependencies on some EUMM versions.

Subsequently .... Problem replication:

  1. Be on Vanilla Perl 5.22.1 ( Has EUMM 7.0401 which shipped 4.28 )

  2. cpanm --look CGI@4.28

  3. rm META.*

  4. perl Makefile.PL

  5. make distdir

  6. cat CGI-4.28/META.json

    "prereqs" : {
        "build" : {
           "requires" : {}  #<-- BAD
        },
        "configure" : {
           "requires" : {}  # <-- BAD
        },
        "runtime" : {
           "requires" : {
              "Carp" : "0",
              "Config" : "0",
              "Encode" : "0",
              "Exporter" : "0",
              "File::Spec" : "0.82",
              "File::Temp" : "0",
              "HTML::Entities" : "3.69",
              "base" : "0",
              "if" : "0",
              "overload" : "0",
              "parent" : "0.225",
              "perl" : "5.008001",
              "strict" : "0",
              "utf8" : "0",
              "warnings" : "0"
           }
        }
    },
  7. ^D

    1. cpanm --look CGI@4.28
  8. rm META.*

  9. Edit Makefile.PL and remove META_ADD

  10. perl Makefile.PL

  11. make distdir

  12. cat CGI-4.28/META.json

    "prereqs" : {
        "build" : {
           "requires" : {
              "Cwd" : "0",
              "ExtUtils::MakeMaker" : "0",
              "File::Find" : "0",
              "IO::File" : "0",
              "IO::Handle" : "0",
              "POSIX" : "0",
              "Test::Deep" : "0.11",
              "Test::More" : "0.98",
              "Test::Warn" : "0.3"
           }
        },
        "configure" : {
           "requires" : {
              "ExtUtils::MakeMaker" : "0"
           }
        },
        "runtime" : {
           "requires" : {
              "Carp" : "0",
              "Config" : "0",
              "Encode" : "0",
              "Exporter" : "0",
              "File::Spec" : "0.82",
              "File::Temp" : "0",
              "HTML::Entities" : "3.69",
              "base" : "0",
              "if" : "0",
              "overload" : "0",
              "parent" : "0.225",
              "perl" : "5.008001",
              "strict" : "0",
              "utf8" : "0",
              "warnings" : "0"
           }
        }

@leejo
Copy link
Owner

leejo commented Jul 19, 2016

The premise behind this commit is flawed. The correct action is to revert this commit, and either ignore the invalid cpantesters reports, or to pursue the issue with the smoker and fix it there. However this sounds like it could be the same issue recently explored on the cpantesters.discuss mailing list.

Could you point me to the discussion on the mailing list? I'll probably revert the commit and ignore the failures.

@kentfredric
Copy link
Contributor Author

I believe she's referring to this: http://www.nntp.perl.org/group/perl.cpan.testers.discuss/2016/07/msg3850.html

But given I can reproduce the problem with dependencies not being exported, you probably want to revert that code and nuke the "wipe config with META_ADD" logic.

@karenetheridge
Copy link

The thread I was thinking of is http://www.nntp.perl.org/group/perl.cpan.testers.discuss/2016/07/msg3850.html.

What sort of errors have you been receiving?

@leejo
Copy link
Owner

leejo commented Jul 19, 2016

What sort of errors have you been receiving?

Here's an example: http://www.cpantesters.org/cpan/report/82e58cd4-e75d-11e5-bc4f-33d50d52efe3

@kentfredric
Copy link
Contributor Author

kentfredric commented Jul 19, 2016

git revert cc18e3a
perl -MExtUtils::MakeMaker\ 9999
# ExtUtils::MakeMaker version 9999 required--this is only version 7.1001.
# BEGIN failed--compilation aborted.
git clean -ndX; perl Makefile.PL; make distdir; cp CGI-4.31/META.json  /tmp/META_01.json
gvim Makefile.PL
git diff
diff --git a/Makefile.PL b/Makefile.PL
index 9d9184d..d9ce36c 100644
--- a/Makefile.PL
+++ b/Makefile.PL
@@ -60,10 +60,6 @@ WriteMakefile(
                 },
                 no_index => { directory => [qw/t/] },
             },
-            META_ADD => {
-                build_requires     => {},
-                configure_requires => {}
-            },
         )
     ),
 );
git clean -ndX; perl Makefile.PL; make distdir; cp CGI-4.31/META.json  /tmp/META_02.json
diff -Naur /tmp/META_01.json /tmp/META_02.json
--- /tmp/META_01.json   2016-07-19 18:23:15.757242283 +1200
+++ /tmp/META_02.json   2016-07-19 18:23:41.166985210 +1200
@@ -22,10 +22,22 @@
    },
    "prereqs" : {
       "build" : {
-         "requires" : {}
+         "requires" : {
+            "Cwd" : "0",
+            "ExtUtils::MakeMaker" : "0",
+            "File::Find" : "0",
+            "IO::File" : "0",
+            "IO::Handle" : "0",
+            "POSIX" : "0",
+            "Test::Deep" : "0.11",
+            "Test::More" : "0.98",
+            "Test::Warn" : "0.3"
+         }
       },
       "configure" : {
-         "requires" : {}
+         "requires" : {
+            "ExtUtils::MakeMaker" : "0"
+         }
       },
       "runtime" : {
          "requires" : {

leejo added a commit that referenced this issue Jul 19, 2016
This reverts commit cc18e3a.

This is a regression for end users who use CPAN Clients that support
--no-testing, as it means they can no longer elide test dependencies.

An example of a failing testers can be seen at:

	http://www.cpantesters.org/cpan/report/82e58cd4-e75d-11e5-bc4f-33d50d52efe3

The correct action is to either ignore the invalid cpantesters reports,
or to pursue the issue with the smoker and fix it there.
@leejo
Copy link
Owner

leejo commented Jul 19, 2016

Does the above resolve this issue entirely or is there more to do?

@kentfredric
Copy link
Contributor Author

I'll file a PR with other fixes because you can demonstrate yourself, build the dist, check the generated META.*, and you will find an absense of the test dependencies as-is.

@leejo
Copy link
Owner

leejo commented Jul 19, 2016

OK, thanks!

@leejo
Copy link
Owner

leejo commented Jul 19, 2016

OK, v4.32 will be on its way to CPAN shortly. Thanks both!

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

3 participants