libwww-perl / URI

The Perl URI module
https://metacpan.org/pod/URI
Other
41 stars 48 forks source link

Enhancement for issue#101 -- "5.11 breaks CPAN::Reporter" #104

Closed Perlbotics closed 2 years ago

Perlbotics commented 2 years ago

A simple pre-check is performed to ensure that the current version of the URI module is tested.

This does not fix issue#101 but could detect problems during build- and test-phases.

See: https://github.com/libwww-perl/URI/issues/101

haarg commented 2 years ago

The inode checks definitely won't work. In the normal install process, the files will be copied to blib/lib and loaded from there during testing.

Perlbotics commented 2 years ago

Oh, I was too focused on the source part. Thanks. dzil test also complained. The new version should work better unless the build process modifies the target in the build directory.

Perlbotics commented 2 years ago

I don't understand what's wrong with the 'Changes' file...? https://github.com/libwww-perl/URI/runs/7242032219?check_suite_focus=true#step:4:243

simbabque commented 2 years ago

I don't understand what's wrong with the 'Changes' file...? https://github.com/libwww-perl/URI/runs/7242032219?check_suite_focus=true#step:4:243

The Changes file needs to have an entry. One of our dzil plugins requires this so each PR actually contributes the summary of what it has changed. It saves us work, and is common to all libwww-perl dists. To fix it, you'd have to add something to the Changes file under the {{$NEXT}}.

Perlbotics commented 2 years ago

Thanks, but I am confused. The test case said Failed test 'Changes has content for 5.12' and the Changes file has an entry of 5.12. I had a look at the dzil module. It checks for the given number, extracts all lines until the next entry, discards the first line and all empty lines and then checks if the remaining lines are more than one. Sounds okay for me.

haarg commented 2 years ago

Overall, I don't like this PR. Needing to test via prove -l or make test or dzil test is standard. This test provides no value for a user. And generally, if you forget to use -l the test will fail anyway and remind you to run the correct command.

Perlbotics commented 2 years ago

Sure, but we don't know what the OP did. Especially not, if he ran the build manually or using the tools you mentioned. In case of the latter, this might have caught the problem. What could have been the cause? IDK, PERL5LIB setting? Disk full? Permissions? As I said before, if the maintainers don't like the PL, skip it. No harm done. But I would like to see the bug tag removed from this issue since ATM it looks like an isolated build incident. Update: CPAN testers:

PASS: 453 Number of tested configurations: 127

haarg commented 2 years ago

I don't see how this actually helps in that regard. The initial report already showed that they had a broken installation. The URI tests either already passed or were ignored. The issue happened after that or with the environment.

simbabque commented 2 years ago

Thanks, but I am confused. The test case said _Failed test 'Changes has content for 5.12'_ and the Changes file has an entry of 5.12.

In one your commit 7fc3a82ca385928ffa0bfedfcb1a2087d11e4f56 you inserted a heading for 5.12 into Changes. That has messed with what dzil expects. When you submit a PR, you can't know when that will be merged or with what version it will be released, so the tool expects to stick that line under the {{$NEXT}}. This way it will stay in the right place even if your PR goes in much later than you thought it would, avoiding a conflicting version number. Dzil works by looking at the $VERSION in lib/URI.pm, which is 5.11, so will bump it to 5.12 when you dzil test or dzil build, changing the {{$NEXT}} to that new version. That's why it clashed and had two 5.12 entries. It didn't even look at what was already in the file before its marker.

Perlbotics commented 2 years ago

Thanks Julien (@simbabque), so it's a template and I used it wrong. Updated Changes here and in #103.

Perlbotics commented 2 years ago

Hi Julien (@simbabque), thank you and the others for the time and effort invested. I have no problem with a rejected PR if the reason for rejection is clearly explained (here: it is). If the lines to justify the PR are magnitudes more than the lines of code, there is probably something going wrong ;-)

I guess, the process is to set the status of this PR to closed/rejected or something. If there is a way to formally cancel the PR, please let me know (I couldn't find something in one of the github menus). Ah, never mind, found a Close with comment button... click

oalders commented 2 years ago

Thanks for your efforts @Perlbotics! Yes, the close button is the standard way to handle this. 😄