From 36b4039d0aa758ad666c58158305cdb835334589 Mon Sep 17 00:00:00 2001 From: "Jose D. Gomez R" Date: Thu, 26 Oct 2023 18:39:48 +0200 Subject: [PATCH] Centralize perl linters MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Mixed all linter configurations from os-autoinst, openQA, os-autoinst-distri-opensuse. Perlcritic RC & Perltidy RC files are centralized now here. For convenience, the most laxed rules were picked, to minimize the changeset size on downstream repositories. - Improved perltidy & perlcritic wrappers Downstream repositories have a divergent copy of a tidyall wrapper that validates before that perltidy is in the correct version before running. The wrapper does *quite a lot* of text processing to assert the correct version of perltidy. The new and improved version will get the detected perltidy version directly from perl in a clean manner, and will invoke tidyall with all arguments passed to it. Additionally, perlcritic has a wrapper in Makefiles to inject a custom Perl critic policy that it's now provided in this repository. Both scripts are intended to be symlinked by downstream repositories that receive this repository via the subrepo flow. So rules can be enforced uniformly across repositories, even if repositories are not using the default settings provided here. - `Perl::Critic::Policy::HashKeyQuotes` is now enforced automatically via .perlcriticrc. Added `|` symbol as an exception to `Perl::Critic::Policy::HashKeyQuotes`. - All CI Perl-related checks are driven by GitHub Actions. This repo offers a sample for Prove, Perlcritic & Tidyall. Co-authored-by: Tina Müller (tinita) Co-authored-by: Oliver Kurz --- .github/workflows/perl-lint-checks.yml | 26 ++++++ .github/workflows/perl-prove.yml | 18 +++++ .gitignore | 6 ++ .perlcriticrc | 22 +++++ .perltidyrc | 14 ++++ .proverc | 3 + .tidyallrc | 3 + README.md | 81 +++++++++++++++---- cpanfile | 10 +++ lib/OpenQA/Test/PatchDeparse.pm | 9 ++- lib/OpenQA/Test/TimeLimit.pm | 8 +- .../Perl/Critic/Policy/HashKeyQuotes.pm | 35 ++++++++ tools/perlcritic | 31 +++++++ tools/tidyall | 56 +++++++++++++ tools/update-deps | 9 ++- xt/01-make-update-deps.t | 11 ++- 16 files changed, 314 insertions(+), 28 deletions(-) create mode 100644 .github/workflows/perl-lint-checks.yml create mode 100644 .github/workflows/perl-prove.yml create mode 100644 .gitignore create mode 100644 .perlcriticrc create mode 100644 .perltidyrc create mode 100644 .proverc create mode 100644 .tidyallrc create mode 100644 cpanfile create mode 100644 lib/perlcritic/Perl/Critic/Policy/HashKeyQuotes.pm create mode 100755 tools/perlcritic create mode 100755 tools/tidyall diff --git a/.github/workflows/perl-lint-checks.yml b/.github/workflows/perl-lint-checks.yml new file mode 100644 index 0000000..88842bc --- /dev/null +++ b/.github/workflows/perl-lint-checks.yml @@ -0,0 +1,26 @@ +--- +name: 'Perl Lint Checks' + +on: + pull_request: + push: + branches: + - 'master' + +jobs: + perl-lint-checks: + runs-on: ubuntu-latest + name: "Perltidy" + container: + image: perldocker/perl-tester:5.26 + steps: + - uses: actions/checkout@v4 + - run: GITHUB_ACTIONS=1 ./tools/tidyall --check-only --all --quiet + perl-critic-checks: + runs-on: ubuntu-latest + name: "Perlcritic" + container: + image: perldocker/perl-tester:5.26 + steps: + - uses: actions/checkout@v4 + - run: ./tools/perlcritic --quiet . diff --git a/.github/workflows/perl-prove.yml b/.github/workflows/perl-prove.yml new file mode 100644 index 0000000..a0d15f6 --- /dev/null +++ b/.github/workflows/perl-prove.yml @@ -0,0 +1,18 @@ +--- +name: 'Perl Testing Checks' + +on: + pull_request: + push: + branches: + - 'master' + +jobs: + perl-prove: + runs-on: ubuntu-latest + name: "Prove" + container: + image: perldocker/perl-tester:5.26 + steps: + - uses: actions/checkout@v4 + - run: prove . diff --git a/.gitignore b/.gitignore new file mode 100644 index 0000000..13eda00 --- /dev/null +++ b/.gitignore @@ -0,0 +1,6 @@ +.perltidyrc +*.tdy +.*.swp +*~ +__pycache__/ +.tidyall.d/ diff --git a/.perlcriticrc b/.perlcriticrc new file mode 100644 index 0000000..5a32a67 --- /dev/null +++ b/.perlcriticrc @@ -0,0 +1,22 @@ +theme = community +severity = 4 +include = strict Perl::Critic::Policy::HashKeyQuotes +verbose = ::warning file=%f,line=%l,col=%c,title=%m - severity %s::[%p] %e\n + +# Policies built-in in perlcritic +[ControlStructures::ProhibitDeepNests] +severity = 4 +add_themes = community +max_nests = 4 + +# Policies from Community +[Community::DiscouragedModules] +severity = 3 + +# Test modules have no package declaration +[Community::PackageMatchesFilename] +severity = 1 + +# Custom Policies +[Perl::Critic::Policy::HashKeyQuotes] +severity = 5 diff --git a/.perltidyrc b/.perltidyrc new file mode 100644 index 0000000..60eb7ef --- /dev/null +++ b/.perltidyrc @@ -0,0 +1,14 @@ +# Workaround needed for handling non-ASCII in files. +# # See . +--character-encoding=none +--no-valign +-l=160 +-fbl # don't change blank lines +-fnl # don't remove new lines +-nsfs # no spaces before semicolons +-baao # space after operators +-bbao # space before operators +-pt=2 # no spaces around () +-bt=2 # no spaces around [] +-sbt=2 # no spaces around {} +-sct # stack closing tokens )} diff --git a/.proverc b/.proverc new file mode 100644 index 0000000..4c07390 --- /dev/null +++ b/.proverc @@ -0,0 +1,3 @@ +--formatter TAP::Formatter::File +--lib +xt/ diff --git a/.tidyallrc b/.tidyallrc new file mode 100644 index 0000000..d4c7a45 --- /dev/null +++ b/.tidyallrc @@ -0,0 +1,3 @@ +[PerlTidy] +select = **/*.{pl,pm,t} tools/tidyall tools/perlcritic tools/update-deps +argv = --profile=$ROOT/.perltidyrc diff --git a/README.md b/README.md index 537ea4b..722dd96 100644 --- a/README.md +++ b/README.md @@ -1,21 +1,61 @@ # Common files for os-autoinst/os-autoinst and os-autoinst/openQA -This repository is to be used as a -[git-subrepo](https://github.com/ingydotnet/git-subrepo). +This repository is to be used as a[git-subrepo] +(https://github.com/ingydotnet/git-subrepo). See the instructions below in the +[Git Subrepo Usage](#git-subrepo-usage) section +## Tooling offered + +This repo offers: + +* Linter configuration for perl projects: `.perltidyrc`, `.perlcriticrc`, + `.proverc` & `.tidyallrc`. +* Perlcritic policies that `os-autoinst/os-autoinst` and `os-autoinst/openQA` + share. +* Useful tools for linting & testing: + * A `perlcritic` wrapper that will automatically add Perlcritic rules + defined under `lib/perlcritic` and + `external/os-autoinst-common/lib/perlcritic`. + * A `tidyall` wrapper that will validate the Perltidy version against the + `cpanfile` definition. + Because `Perl::Tidy` defaults may vary between versions this tool ensures + the version of perltidy matches the required version specified in the + `cpanfile` for non-`cpanm` based installs. + +* Example Github Actions for linting and unit testing for Perl. + +All files can be either copied or symlinked for any downstream repository +consuming these tools. + +## Running tools and tests from scratch + +```bash +# Run a container with the project mounted in it. +podman run -it -v $(pwd):/host/project --workdir /host/project opensuse/leap:latest bash + +# Inside the container +zypper in -y perl-App-cpanminus make gcc +cpanm --installdeps . --with-develop + +prove . +./tools/tidyall --check-only --all --quiet +./tools/perlcritic --quiet . +``` + +## Git Subrepo Usage `git-subrepo` is available in the following repositories: [![Packaging status](https://repology.org/badge/vertical-allrepos/git-subrepo.svg)](https://repology.org/project/git-subrepo/versions) -## Usage - ### Clone To use it in your repository, you would usually do something like this: - % cd your-repo - % git subrepo clone git@github.com:os-autoinst/os-autoinst-common.git ext/os-autoinst-common +```bash +cd your-repo +git subrepo clone git@github.com:os-autoinst/os-autoinst-common.git external/os-autoinst-common +``` This will automatically create a commit with information on what command was used. @@ -27,42 +67,51 @@ The cloned repository files will be part of your actual repository, so anyone cloning this repo will have the files automatically without needing to use `git-subrepo` themselves. -`ext` is just a convention, you can clone it into any directory. +`external` is just a convention, you can clone it into any directory. It's also possible to clone a branch (or a specific tag or sha): - % git subrepo clone git@github.com:os-autoinst/os-autoinst-common.git \ - -b branchname ext/os-autoinst-common +```bash +git subrepo clone git@github.com:os-autoinst/os-autoinst-common.git \ + -b branchname external/os-autoinst-common +``` -After cloning, you should see a file `ext/os-autoinst-common/.gitrepo` with +After cloning, you should see a file `external/os-autoinst-common/.gitrepo` with information about the cloned commit. ### Pull To get the latest changes, you can pull: - % git subrepo pull ext/os-autoinst-common +```bash +git subrepo pull external/os-autoinst-common +``` If that doesn't work for whatever reason, you can also simply reclone it like that: - % git subrepo clone --force git@github.com:os-autoinst/os-autoinst-common.git ext/os-autoinst-common +```bash +git subrepo clone --force git@github.com:os-autoinst/os-autoinst-common.git \ + external/os-autoinst-common +``` ### Making changes -If you make changes in the subrepo inside of your top repo, you can simply commit -them and then do: +If you make changes in the subrepo inside of your top repo, you can simply +commit them and then do: - % git subrepo push ext/os-autoinst-common +```bash +git subrepo push external/os-autoinst-common +``` ## git-subrepo You can find more information here: + * [Repository and usage](https://github.com/ingydotnet/git-subrepo) * [A good comparison between subrepo, submodule and subtree](https://github.com/ingydotnet/git-subrepo/blob/master/Intro.pod) - ## License This project is licensed under the MIT license, see LICENSE file for details. diff --git a/cpanfile b/cpanfile new file mode 100644 index 0000000..3794cb6 --- /dev/null +++ b/cpanfile @@ -0,0 +1,10 @@ +# Force an up-to-date version so TidyAll dependencies can be installed and +# tested. Older versions of Storable do not handle Regexes. +requires 'Storable', '>= 3.06'; + +on 'develop' => sub { + requires 'Perl::Tidy', '== 20230912'; + requires 'Code::TidyAll'; + requires 'Perl::Critic'; + requires 'Perl::Critic::Community'; +}; diff --git a/lib/OpenQA/Test/PatchDeparse.pm b/lib/OpenQA/Test/PatchDeparse.pm index 429c27f..9f7a17d 100644 --- a/lib/OpenQA/Test/PatchDeparse.pm +++ b/lib/OpenQA/Test/PatchDeparse.pm @@ -1,4 +1,6 @@ package OpenQA::Test::PatchDeparse; +use strict; +use warnings 'all'; use Test::Most; # Monkeypatch B::Deparse @@ -15,7 +17,8 @@ if ( ) { -#<<< do not let perltidy touch this +#<<< do not let perltidy nor perlcritic touch this +## no critic (TestingAndDebugging::ProhibitNoStrict) # This is not our code, and formatting should stay the same for # better comparison with new versions of B::Deparse # <---- PATCH @@ -60,7 +63,5 @@ elsif ($B::Deparse::VERSION) { diag "Using B::Deparse v$B::Deparse::VERSION. If you see 'uninitialized' warnings, update patch in t/lib/OpenQA/Test/PatchDeparse.pm"; } - +## use critic 1; - - diff --git a/lib/OpenQA/Test/TimeLimit.pm b/lib/OpenQA/Test/TimeLimit.pm index 4aa3c5e..57cda67 100644 --- a/lib/OpenQA/Test/TimeLimit.pm +++ b/lib/OpenQA/Test/TimeLimit.pm @@ -1,7 +1,9 @@ -# Copyright 2020-2021 SUSE LLC +# Copyright SUSE LLC # SPDX-License-Identifier: GPL-2.0-or-later package OpenQA::Test::TimeLimit; +use strict; +use warnings 'all'; use Test::Most; my $SCALE_FACTOR = $ENV{OPENQA_TEST_TIMEOUT_SCALE_FACTOR} // 1; @@ -12,8 +14,8 @@ sub import { # disable timeout if requested by ENV variable or running within debugger return if ($ENV{OPENQA_TEST_TIMEOUT_DISABLE} or $INC{'perl5db.pl'}); $SCALE_FACTOR *= $ENV{OPENQA_TEST_TIMEOUT_SCALE_COVER} // 3 if Devel::Cover->can('report'); - $SCALE_FACTOR *= $ENV{OPENQA_TEST_TIMEOUT_SCALE_CI} // 2 if $ENV{CI}; - $limit *= $SCALE_FACTOR; + $SCALE_FACTOR *= $ENV{OPENQA_TEST_TIMEOUT_SCALE_CI} // 2 if $ENV{CI}; + $limit *= $SCALE_FACTOR; $SIG{ALRM} = sub { BAIL_OUT "test '$0' exceeds runtime limit of '$limit' seconds\n" }; alarm $limit; } diff --git a/lib/perlcritic/Perl/Critic/Policy/HashKeyQuotes.pm b/lib/perlcritic/Perl/Critic/Policy/HashKeyQuotes.pm new file mode 100644 index 0000000..309e72e --- /dev/null +++ b/lib/perlcritic/Perl/Critic/Policy/HashKeyQuotes.pm @@ -0,0 +1,35 @@ +# Copyright SUSE LLC +# SPDX-License-Identifier: GPL-2.0-or-later + +package Perl::Critic::Policy::HashKeyQuotes; + +use strict; +use warnings; +use base 'Perl::Critic::Policy'; + +use Perl::Critic::Utils qw( :severities :classification :ppi ); + +our $VERSION = '0.0.1'; + +sub default_severity { return $SEVERITY_HIGH } +sub default_themes { return qw(openqa) } +sub applies_to { return qw(PPI::Token::Quote::Single PPI::Token::Quote::Double) } + +# check that hashes are not overly using quotes +# (os-autoinst coding style) +sub violates { + my ($self, $elem) = @_; + + #we only want the check hash keys + return if !is_hash_key($elem); + + my $c = $elem->content; + # special characters + return if $c =~ m/[- \/<>.=_:\\\$\|]/; + + my $desc = q{Hash key with quotes}; + my $expl = q{Avoid useless quotes}; + return $self->violation($desc, $expl, $elem); +} + +1; diff --git a/tools/perlcritic b/tools/perlcritic new file mode 100755 index 0000000..52c116d --- /dev/null +++ b/tools/perlcritic @@ -0,0 +1,31 @@ +#!/usr/bin/env perl +# Copyright SUSE LLC +# SPDX-License-Identifier: GPL-2.0-or-later +# +# perlcritic with auto-injection of custom perlcritic rules. +use strict; +use warnings 'all'; +use Cwd qw(abs_path getcwd); +use File::Basename 'dirname'; +use File::Spec::Functions 'catfile'; + +sub extra_include_paths { + my @extra_paths = @_; + # using abs_path without arguments so symlinks are not resolved. + # dirname twice to go from `$prj_root/tools/tidyall` to: `$proj_root` + my $prj_root = dirname dirname catfile(abs_path(), __FILE__); + + my @paths = (); + + foreach my $path (@extra_paths) { + push @paths, catfile($prj_root, $path); + push @paths, catfile($prj_root, "external/os-autoinst-common", $path); + } + + # Remove non existing paths + return grep { -e $_ } @paths; +} + +$ENV{PERL5LIB} = join(':', (extra_include_paths('lib/perlcritic'), $ENV{PERL5LIB} // '')); + +exec 'perlcritic', @ARGV; diff --git a/tools/tidyall b/tools/tidyall new file mode 100755 index 0000000..08e379b --- /dev/null +++ b/tools/tidyall @@ -0,0 +1,56 @@ +#!/usr/bin/env perl +# Copyright SUSE LLC +# SPDX-License-Identifier: GPL-2.0-or-later +# +# Tidyall command with perltidy version constraint. +use strict; +use warnings 'all'; +use Perl::Tidy; +use Module::CPANfile; + +=item perltidy_version() + +Grabs the perltidy version from cpanfile using Module::CPANfile. + +=cut + +sub perltidy_version { + $_ = Module::CPANfile->load('cpanfile') + ->prereq_for_module('Perl::Tidy') + ->requirement + ->version; + # Version requirements may contain qualifiers >=, ==, <, etc. The convention + # is to separate the qualifier from the actual version with a space. + # + # It's safe enough to assume that the last item is really the version. + @_ = split(' ', $_); + return pop(); +} + +sub is_force_flag { $_ eq '--force' } + +my $required_version = perltidy_version(); +my $detected_version = $Perl::Tidy::VERSION; +my @tidyall_argv = @ARGV; + +unless ($detected_version eq $required_version) { + print STDERR "Incorrect version of perltidy.\n"; + printf STDERR "- Detected: %s\n+ Required: %s\n\n", $detected_version, $required_version; + + my $force_run = grep { is_force_flag } @ARGV; + + unless ($force_run) { + printf STDERR "Please install the appropriate version of perltidy.\n"; + printf STDERR "If you want to proceed anyways, re run with --force flag.\n"; + exit 1; + } + + # tidyall does not know about the --force flag. + @tidyall_argv = grep { !is_force_flag } @tidyall_argv; + + print STDERR "Proceeding to run with incorrect version of perltidy. "; + print STDERR "Results might not be consistent.\n"; + print STDERR "==================\n"; +} + +exec 'tidyall', @tidyall_argv; diff --git a/tools/update-deps b/tools/update-deps index 85b5465..56086c5 100755 --- a/tools/update-deps +++ b/tools/update-deps @@ -1,9 +1,10 @@ #!/usr/bin/env perl -# Copyright 2020 SUSE LLC -# SPDX-License-Identifier: MIT +# Copyright SUSE LLC +# SPDX-License-Identifier: GPL-2.0-or-later + use strict; -use warnings; -use 5.010; +use warnings 'all'; +use 5.26; use YAML::PP; use Data::Dumper; diff --git a/xt/01-make-update-deps.t b/xt/01-make-update-deps.t index 2d40642..9fde25a 100755 --- a/xt/01-make-update-deps.t +++ b/xt/01-make-update-deps.t @@ -1,7 +1,10 @@ #!/usr/bin/env perl -# Copyright 2022 SUSE LLC +# Copyright SUSE LLC # SPDX-License-Identifier: GPL-2.0-or-later +use strict; +use warnings 'all'; + use Test::Most; use Test::Warnings; use FindBin '$Bin'; @@ -12,6 +15,12 @@ if (not -e "$Bin/../.git") { exit; } +if (not -e "$Bin/../Makefile") { + pass("Skipping all tests, no Makefile"); + done_testing; + exit; +} + my $build_dir = $ENV{OS_AUTOINST_BUILD_DIRECTORY} || "$Bin/.."; my $make_tool = $ENV{OS_AUTOINST_MAKE_TOOL} || 'make'; my $make_cmd = "$make_tool update-deps";