On Sat, Apr 15, 2017 at 4:27 AM, David G <lsmb...@sbts.com.au> 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://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
>
> <http://search.cpan.org/%7Ethaljef/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/4525877https://www.securecoding.cert.org/confluence/display/perl/SEI+CERT+Perl+Coding+Standardhttps://matrix.to/#/!qyoLumPqusaXqFJNyK:matrix.org/$14917685211662024izFqM:matrix.orghttps://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.
------------------------------------------------------------------------------
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
