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.

Sign up for GitHub

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

Jump to bottom

Skip ->dist if $skip_dist #48

Merged
merged 4 commits into from Aug 6, 2023
Merged

Skip ->dist if $skip_dist #48

merged 4 commits into from Aug 6, 2023

Conversation

Tux
Copy link
Contributor

@Tux Tux commented Aug 3, 2023

This includes skipping cpan_version_check, as it depends on running dist

When running release -t on 400 versions of perl, dist checks and mere annoying and certainly not helpful

@briandfoy briandfoy self-assigned this Aug 3, 2023
@briandfoy briandfoy added Type: enhancement improve a feature that already exists Priority: low get to this whenever labels Aug 3, 2023
briandfoy
briandfoy reviewed Aug 3, 2023
View reviewed changes
Copy link
Owner

@briandfoy briandfoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these changes aren't the way to get what you want, and disrupt the intent of the tool. You can turn off the checks that need to make the dist: release -k -V -t. If that's not working for you, show me what's going on.

script/release Outdated
@@ -762,7 +762,7 @@ $release->clean;
$release->touch_all_in_manifest;
$release->build_makefile;
$release->make;
$release->dist;
$release->dist unless $skip_dist;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you are using -t, you should never reach this code.

script/release Outdated
@@ -684,7 +684,7 @@ unless( $opts{T} // $release->config->skip_tests ) {
if( $opts{t} // $release->config->dry_run ) {
$release->_print("============ Cleaning up\n");
$release->_print( "This is a dry run, so stopping. Cleaning up.\n" );
$release->distclean;
eval { $release->distclean };
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if I should change how run works so it doesn't croak. It can return success or failure instead. But I think this issue is unrelated to the issue, and we'd still need to ensure it outputs the error message.

script/release Outdated
@@ -595,7 +595,7 @@ unless( $opts{k} // $release->config->skip_kwalitee ) {
$release->clean;
$release->build_makefile;
$release->make;
$release->dist;
$release->dist unless $skip_dist;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The check_kwalitee needs the local dist file to exist. Are you seeing something different? But, I would think if you are testing against 400 perls, you don't want to test kwalitee 400 times either, so you'd have release -k -t.

script/release Outdated
@@ -557,7 +557,7 @@ $release->_debug( "Will use HARNESS_OPTIONS '$ENV{HARNESS_OPTIONS}' during tests

# # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # #
# Check that this is a version later than the one of CPAN
unless( $opts{V} // $release->config->skip_cpan_version_check ) {
unless( $opts{V} // $skip_dist || $release->config->skip_cpan_version_check ) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Version checking needs to make the dist too, but I don't think you'd want this check for what you are doing. Would release -V -t work?

@briandfoy briandfoy added the Status: needs feedback requestor or community feedback needed to go on label Aug 3, 2023
@briandfoy
Copy link
Owner

briandfoy commented Aug 3, 2023 •
edited

Heh, I've often found that I make mistakes that are only caught when I make the dists. :)

I cherry-picked fde7964 for the Makefile.PL update for bug tracker and have released 2.133 so MetaCPAN can update.

But, the release script is a tool to release dists, and if you aren't doing that, it might be time to make a new program with just the parts you need. That can still be based off Module::Release, and it's something we can put in this repo, but as a stripped down version of release.

@Tux
Copy link
Contributor Author

Tux commented Aug 4, 2023

Yes, I think that makes sense! I'll come up with a cut-down script (which will most likely be my style/layout)

@Tux
Copy link
Contributor Author

Tux commented Aug 4, 2023 •
edited

Maybe pre-release is a better name than release-test

Feedback welcome

Tux added 2 commits August 4, 2023 14:50
@Tux
Skip ->dist if $skip_dist
0918633 
This includes skipping cpan_version_check, as it depends on
running dist

When running "release -t" on 400 versions of perl, dist checks
and mere annoying and certainly not helpful
@Tux
Add script/release-test
40e2521
@Tux
Copy link
Contributor Author

Tux commented Aug 4, 2023

Force pushed the tux branch as a complete rebase on master

briandfoy
briandfoy requested changes Aug 4, 2023
View reviewed changes
Copy link
Owner

@briandfoy briandfoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a good start on release-test, but I think there is still some stuff you can prune. Not a big deal.

If you undo the changes to release, I'll merge this.

script/release-test Outdated
}
}

$release->load_mixin ("Module::Release::Prereq");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't use *::Prereq, so you can delete this.

script/release-test
$release->clean;

foreach my $mod (grep m/\S/ => split m/\s*,\s*/ => $required) {
system "$perl -M$mod -e1 >/dev/null 2>&1";
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check that $mod is a valid package name so there's no code injection

script/release-test Outdated

$release->load_mixin ("Module::Release::Prereq");

my $skip_prereqs = 1;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like you don't use these two variables.

@briandfoy briandfoy added Status: changes requested adjust the pull request as noted in comments and removed Status: needs feedback requestor or community feedback needed to go on labels Aug 4, 2023
@briandfoy briandfoy merged commit dc83a63 into briandfoy:master Aug 6, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@briandfoy briandfoy briandfoy approved these changes

Assignees

@briandfoy briandfoy

Labels
Priority: low get to this whenever Status: changes requested adjust the pull request as noted in comments Type: enhancement improve a feature that already exists
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Tux @briandfoy