Re: Perl::Critic Tests

On Sat, Apr 15, 2017 at 4:27 AM, David G <..hidden..> wrote:

Hi Nick,

I think patches would be very desirable at least for master (with the possibility of backporting for 1.5 if they are not too invasive).

While I think we should strive for the highest security possible, thus far I've only been backporting new tests in rare cases -- I did backport some of the fixes, though. But in general, I'd like to keep the change on the release branches minimal. Much more appealing to me would be to release a new version based on 'master' if there's enough interesting new stuff on it; the "new stuff" could be anything from new functionality or nicer UI to improved security.

I would prefer to see them as a single Critic Policy per patch, with perhaps no more than 50 items addressed per patch.
That should limit the impact and make review easier

It should also help with back porting if we choose to do so as many of the patches should apply cleanly

Personally, I'd start with the Policies with the highest count, and work down through the list from there.

I'm with Nick on his approach: lets start by adding the policies with the lowest count (i.e. zero failures): that immediately protects us from making those failures in the future at no more cost than just extending a list of tests in xt/01.1-critic.t.. Working our way up the list (from few to many failures), because we can add new policies at the cost of making only 2 or 3 source code changes for the first policies...

That should allow maximum results for minimum effort. As well as a faster reduction of violations.
eg: Adding a "final return" to subroutines should be (in most cases) as simple as adding a return statement at the end of every effected sub()

Keep in mind that depending on the "running" perl version we may need to add specific policy violation exceptions.

Really? I wasn't aware of that. I really can't imagine *security* policies which have this problem (now, if you're talking coding *style* then, yes, but lets try to stay away from that for now).

Running the critic tests on perl 5.22 may throw a violation that we can't resolve if we wish to retain 5.10 compatibility.

I'd suggest that we probably need to do a comparison between the critic results when running on both the earliest perl version we support (5.10 from memory) and the latest available perl version (5.24) to see how much impact that's likely to have.

As for the RequireVersionVar policy, I agree that we should have version numbers in ALL of our modules, but exactly how to handle this gets somewhat complicated.
Some info on this can be found at....

http://stackoverflow.com/questions/1454202/how-can-i-automatically-update-perl-modules-version-with-git

http://search.cpan.org/dist/PPI-PowerToys/lib/PPI/PowerToys.pm

http://search.cpan.org/~thaljef/Perl-Critic/lib/Perl/Critic/Policy/Modules/RequireVersionVar.pm

I think it *only* makes sense to require that there's a version variable in the sources *unless* the modules are required with a version too.

Coming to think of it: that would probably be a great way to solve this ugly bit in our sources trying to validate that we have the correct version of our modules installed:

https://github.com/ledgersmb/LedgerSMB/blob/master/tools/starman.psgi#L28-L31

if we could simply replace that with

use LedgerSMB '1.5.0';

that would be terrific!

That last link states
$VERSION should be defined in the form....

    our $VERSION = 1.0611;
    $MyPackage::VERSION = 1.061;
    use vars qw($VERSION);
    use version; our $VERSION = qv(1.0611);

but NOT as....

    my $VERSION = 1.0611;

Actually, due to the way CPAN interprets "1.9" (as 1.900 - which is bigger than 1.10 [1.100]) we want to be using version triplets instead of pairs.

Further it suggests a common practice of....
    our ($VERSION) = '$Revision$' =~ m{ \$Revision: \s+ (\S+) }x;
However a quick search didn't turn up any docs on "$Revision$"

That's because $Revision$ isn't anything Perlish: It's a remnant from RCS times (the predecessor of CVS, the predecessor of SVN, the predecessor of GIT, HG, ... :-) )

I'd STRONGLY recommend a separate discussion about VERSION so we can work out a Project Level policy before making any changes to the files.

Yes. We need to find a good way to handle this indeed. Managing this (automatically, preferably) isn't straight forward...

Regards
David G

On 15/04/17 02:02, Nick Prater wrote:

As discussed in chat, I've been following up on ehuelsmann's suggestion 
to test our code against the Perl::Critic policies recommended by CERT. 
See:

https://gist.github.com/briandfoy/4525877
https://www.securecoding.cert.org/confluence/display/perl/SEI+CERT+Perl+Coding+Standard
https://matrix.to/#/!qyoLumPqusaXqFJNyK:matrix.org/$14917685211662024izFqM:matrix.org
https://matrix.to/#/!qyoLumPqusaXqFJNyK:matrix.org/$1491849623220407yWMci:matrix.org

Enabling the CERT recommended policies on our 'new' code results in 1050 
policy violations:

'Perl::Critic::Policy::Subroutines::RequireFinalReturn' => 293,
'Perl::Critic::Policy::ValuesAndExpressions::ProhibitMagicNumbers' => 
172,
'Perl::Critic::Policy::Modules::RequireVersionVar' => 170,
'Perl::Critic::Policy::ErrorHandling::RequireCarping' => 122,
'Perl::Critic::Policy::Subroutines::ProhibitUnusedPrivateSubroutines' => 
34,
'Perl::Critic::Policy::InputOutput::RequireCheckedSyscalls' => 31,
'Perl::Critic::Policy::Variables::RequireInitializationForLocalVars' => 
26,
'Perl::Critic::Policy::Subroutines::ProhibitBuiltinHomonyms' => 24
'Perl::Critic::Policy::ValuesAndExpressions::ProhibitMixedBooleanOperators' 
=> 23,
'Perl::Critic::Policy::Subroutines::ProhibitExplicitReturnUndef' => 21,
'Perl::Critic::Policy::RegularExpressions::ProhibitCaptureWithoutTest' 
=> 14,
'Perl::Critic::Policy::InputOutput::RequireCheckedOpen' => 11,
'Perl::Critic::Policy::Objects::ProhibitIndirectSyntax' => 11,
'Perl::Critic::Policy::Variables::ProhibitUnusedVariables' => 10,
'Perl::Critic::Policy::TestingAndDebugging::ProhibitNoWarnings' => 10,
'Perl::Critic::Policy::Variables::RequireLocalizedPunctuationVars' => 
10,
'Perl::Critic::Policy::InputOutput::ProhibitBarewordFileHandles' => 10,
'Perl::Critic::Policy::ValuesAndExpressions::ProhibitMismatchedOperators' 
=> 9,
'Perl::Critic::Policy::Subroutines::ProtectPrivateSubs' => 8,
'Perl::Critic::Policy::InputOutput::RequireCheckedClose' => 8,
'Perl::Critic::Policy::BuiltinFunctions::ProhibitBooleanGrep' => 5,
'Perl::Critic::Policy::Variables::RequireLexicalLoopIterators' => 5,
'Perl::Critic::Policy::ValuesAndExpressions::ProhibitCommaSeparatedStatements' 
=> 4,
'Perl::Critic::Policy::BuiltinFunctions::ProhibitStringySplit' => 4,
'Perl::Critic::Policy::ControlStructures::ProhibitUnreachableCode' => 3,
'Perl::Critic::Policy::Variables::ProhibitConditionalDeclarations' => 3,
'Perl::Critic::Policy::TestingAndDebugging::ProhibitNoStrict' => 3,
'Perl::Critic::Policy::BuiltinFunctions::ProhibitUniversalIsa' => 3,
'Perl::Critic::Policy::InputOutput::RequireEncodingWithUTF8Layer' => 2,
'Perl::Critic::Policy::BuiltinFunctions::ProhibitStringyEval' => 1,


I've not tested 'old' code, as I view 'new' code as the first priority.

Our current coding guidelines are set out here:
https://ledgersmb.org/community-guide/community-guide/development/coding-guidelines/perl-coding-guidelines

Moving forward, Do we want to make these Perl::Critic policies part of 
our coding standard?

If yes, I'm happy to start preparing patches to fix these violations and 
add policies to the tests in xt/01.1-critic.t.

Happy Easter!

Nick

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Ledger-smb-devel mailing list
Ledger-smb-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ledger-smb-devel

Bye,

Erik.

http://efficito.com -- Hosted accounting and ERP.

Robust and Flexible. No vendor lock-in.