-
Notifications
You must be signed in to change notification settings - Fork 602
Add a step to the RMG about podcheck #21000
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
Conversation
|
This looks like it needs a rebase and maybe a squash for the two commits that have the same message. Do you have an example link errors were ignored where they shouldn't be? Perhaps such errors shouldn't be ignorable. |
|
I'll do the squash when finally pushing I do think this is a wise step to prevent problems. The command responds immediately, so if all is well, this adds less than a minute to the process. I am leery to decide certain problems are not ignorable, because I don't think we can know all the possible circumstances ahead of time. |
Can people who have done development or production releases comment on these proposed changes in the release procedure? |
|
I suggest we merge this, and if people complain it is a burden, revert it |
jkeenan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I built a perl at blead, then ran the command(s) you suggested. The command failed in a way that suggests that t/porting/podcheck.t needs to be run with the perl one has just built, rather than your everyday perl.
Running with my everyday perl (5.38.0 built by perlbrew):
[perl] 2049 $ perl t/porting/podcheck.t --counts
t/porting/podcheck.t: Perl lib version (5.39.2) doesn't match executable '/home/jkeenan/perl5/perlbrew/perls/perl-5.38.0/bin/perl' version (5.38.0) at ../lib/Config.pm line 62.
Compilation failed in require at t/porting/podcheck.t line 18.
BEGIN failed--compilation aborted at t/porting/podcheck.t line 18.
Re-running with the just built perl:
$ ./perl -Ilib t/porting/podcheck.t --counts
# 3 ? Should you be using F<...> or maybe L<...> instead of
# 6 ? Should you be using L<...> instead of
# 44 Verbatim line length including indents exceeds 78 by
# -----
# 53 known potential issues
#
# Files that have all messages of at least one type suppressed:
# ext/pod-html/corpus/perlvar-copy.pod, lib/config.pod, pod/perldebguts.pod, pod/perldtrace.pod, pod/perlperf.pod, pod/perlsolaris.pod, porting/epigraphs.pod
Porting/release_managers_guide.pod
Outdated
| perl t/porting/podcheck.t --show-all | ||
|
|
||
| and grep for those problems. (It can take a minute or so to run.) | ||
|
|
||
| If you decide any should be fixed, after that happens, run | ||
|
|
||
| perl t/porting/podcheck.t |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As @jkeenan points out, this doesn't work.
Now assuming I do:
$ ./perl t/porting/podcheck.t --counts
# 3 ? Should you be using F<...> or maybe L<...> instead of
# 6 ? Should you be using L<...> instead of
# 44 Verbatim line length including indents exceeds 78 by
# -----
# 53 known potential issues
#
# Files that have all messages of at least one type suppressed:
# ext/pod-html/corpus/perlvar-copy.pod, lib/config.pod, pod/perldebguts.pod, pod/perldtrace.pod, pod/perlperf.pod, pod/perlsolaris.pod, porting/epigraphs.pod
instead, how much of that is relevant to the current release?
Are we expecting a release manager to inspect each change each time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to specify the newly built perl
Squashing the commits makes clear that the new text says don't bother with any warning which begins with a ? nor the Verbatim warnings. So the example you ran would have nothing else for the release manager to do. This step under those circumstances would add less than a minute to the release process. And this is the likely case.
If there are other issues, it would be up to the release manager's judgment as to if the other types of warning should hold up a release or not. My guess is that only a very few possible things might be deemed to require fixing before proceeding with a development release, but more with a stable release
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have an example of a problem you think should not have been ignored? When such a problem perhaps could have been ignored?
I can see deciding this to involve a level of judgement that might be outside of what we expect from a release manager. Release managers have a fair amount of work to do and while it requires some technical perl knowledge, it's fairly mechanical.
Adding the requirement of perhaps a core developer's level of judgement for this might be too much.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The example is included in the text. A broken link was shipped in a devel release. The podcheck db had been regenerated to ignore the problem. Text could be added that if you don't understand the problem, get help from someone who does. If no such person is immediately available, ignore the issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In thinking about it more, these errors are just about pod, and I think all our release managers would be familiar with pod. I don't know all the possible errors that can get generated, but my suspicion is that all the ones not listed as in the text here are likely to need to be fixed.
I'm reluctant to make things non-ignorable, as circumstances could arise that I haven't foreseen, such as it being wrong about the link being broken, and it's best to be humble about that possibility.
@tonycoz did you really mean to approve this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tonycoz did you really mean to approve this?
Yes
It's quick to skim the podcheck summary of errors, and some things, like broken links should really be fixed before the release.
It's quick to skim the podcheck summary of errors, and some things, like
broken links should really be fixed before the release.
This replaces #20975