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.

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

Centralize perl lint checks #30

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions .github/workflows/perl-lint-checks.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
---
name: 'Perl static checks'

on:
pull_request:
push:
branches:
- 'master'

jobs:
perl-lint-checks:
runs-on: ubuntu-latest
name: "Perltidy"
container:
image: perldocker/perl-tester
steps:
- uses: actions/checkout@v4
- run: GITHUB_ACTIONS=1 ./tools/tidyall --check-only --all --quiet
kalikiana marked this conversation as resolved.
Show resolved Hide resolved
perl-critic-checks:
runs-on: ubuntu-latest
name: "Perlcritic"
container:
image: perldocker/perl-tester
steps:
- uses: actions/checkout@v4
- run: ./tools/perlcritic --quiet .
18 changes: 18 additions & 0 deletions .github/workflows/perl-prove.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
---
name: 'Perl tests'

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 .
5 changes: 5 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
*.tdy
.*.swp
*~
__pycache__/
.tidyall.d/
49 changes: 49 additions & 0 deletions .perlcriticrc
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
theme = community + openqa
severity = 4
include = strict ValuesAndExpressions::ProhibitInterpolationOfLiterals

verbose = ::warning file=%f,line=%l,col=%c,title=%m - severity %s::[%p] %e\n

# == Perlcritic Policies
# -- Test::Most brings in strict & warnings
[TestingAndDebugging::RequireUseStrict]
equivalent_modules = Test::Most

[TestingAndDebugging::RequireUseWarnings]
equivalent_modules = Test::Most

# -- Avoid double quotes unless there's interpolation or a single quote.
[ValuesAndExpressions::ProhibitInterpolationOfLiterals]
allow_if_string_contains_single_quote = 1
severity = 3

# -- Prohibit deep nesting
[ControlStructures::ProhibitDeepNests]
severity = 4
add_themes = community
max_nests = 4

# == Community Policies
# -- Test::Most brings in strict & warnings
[Freenode::StrictWarnings]
extra_importers = Test::Most

# -- Test::Most brings in strict & warnings
[Community::StrictWarnings]
extra_importers = Test::Most

[Community::DiscouragedModules]
severity = 3

# Test modules have no package declaration
[Community::PackageMatchesFilename]
severity = 1

# == Custom Policies
# -- Useless quotes on hashes
[HashKeyQuotes]
severity = 5

# -- Superfluous use strict/warning.
[RedundantStrictWarning]
equivalent_modules = Test::Most
14 changes: 14 additions & 0 deletions .perltidyrc
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
# Workaround needed for handling non-ASCII in files.
josegomezr marked this conversation as resolved.
Show resolved Hide resolved
# # See <https://github.com/houseabsolute/perl-code-tidyall/issues/84>.
--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 )}
3 changes: 3 additions & 0 deletions .proverc
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
--formatter TAP::Formatter::File
--lib
xt/
3 changes: 3 additions & 0 deletions .tidyallrc
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
[PerlTidy]
select = **/*.{pl,pm,t} tools/tidyall tools/perlcritic tools/update-deps
argv = --profile=$ROOT/.perltidyrc
85 changes: 70 additions & 15 deletions README.md
Original file line number Diff line number Diff line change
@@ -1,21 +1,67 @@
# 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

All dependencies in this project are sourced from what's available in openSUSE
Tumbleweed RPM repositories.

For developers not using RPM the tumbleweed repositories, a `cpanfile` is
provided and maintained in a best-effort basis.

## Tooling offered

This repo offers:

* Static check 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
`ext/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 .
Comment on lines +46 to +48
Copy link
Member

Choose a reason for hiding this comment

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

I strongly prefer a simple Makefile that just offers to call all that with a simple make test. If you call the Makefile in GHA or use the low-level commands I consider less important although my personal preference is that CI sticks as much as possible to what we expect to work for users

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not against it. But not right now.

for two reasons:

  1. the examples of make test that I've found in os-autoinst are impossible to understand what's happening in just a quick look. This is not to say they're wrong, but they're quite complex.
  2. make test can change dramatically across downstream repos. os-autoinst & openQA 's make test have completely different structures and sub targets and ways to catalog tests into partitions, organization, etc. etc. etc.

I plan to begin with the most generic tooling, and then integrate more complex steps.

Copy link
Member

Choose a reason for hiding this comment

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

You'll probably want to call the exact same tools in all projects? I can see that you don't want to try and unify everything.

How about you just add a new target that does whatever perl-lint-checks is supposed to do. Don't even try to change anything else.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I was also only thinking of a Makefile that does conduct the tests in that repo. Makefile might act as an example for other repos but does not need to.

```

## 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 [email protected]:os-autoinst/os-autoinst-common.git ext/os-autoinst-common
```bash
cd your-repo
git subrepo clone [email protected]:os-autoinst/os-autoinst-common.git ext/os-autoinst-common
```

This will automatically create a commit with information on what command
was used.
Expand All @@ -27,12 +73,14 @@ 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.
josegomezr marked this conversation as resolved.
Show resolved Hide resolved

It's also possible to clone a branch (or a specific tag or sha):

% git subrepo clone [email protected]:os-autoinst/os-autoinst-common.git \
-b branchname ext/os-autoinst-common
```bash
git subrepo clone [email protected]:os-autoinst/os-autoinst-common.git \
-b branchname ext/os-autoinst-common
```

After cloning, you should see a file `ext/os-autoinst-common/.gitrepo` with
information about the cloned commit.
Expand All @@ -41,28 +89,35 @@ information about the cloned commit.

To get the latest changes, you can pull:

% git subrepo pull ext/os-autoinst-common
```bash
git subrepo pull ext/os-autoinst-common
```

If that doesn't work for whatever reason, you can also simply reclone it like
that:

% git subrepo clone --force [email protected]:os-autoinst/os-autoinst-common.git ext/os-autoinst-common
```bash
git subrepo clone --force [email protected]:os-autoinst/os-autoinst-common.git \
ext/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 ext/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.
19 changes: 19 additions & 0 deletions cpanfile
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
##################################################
# WARNING
# This file is autogenerated by tools/update-deps
# from dependencies.yaml
##################################################

# Needed until preaction/Log-Any#105 is solved.
requires 'Storable', '>= 3.06';
josegomezr marked this conversation as resolved.
Show resolved Hide resolved
josegomezr marked this conversation as resolved.
Show resolved Hide resolved

feature 'cover' => sub {
requires 'Devel::Cover';
requires 'Devel::Cover::Report::Codecov';
};
on 'develop' => sub {
requires 'Code::TidyAll';
requires 'Perl::Critic';
requires 'Perl::Critic::Community';
requires 'Perl::Tidy', '== 20230912';
};
26 changes: 26 additions & 0 deletions dependencies.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
---
# % is placeholder for section.
# e.g.:
# % => develop
# %_requires => develop_requires
targets:
# List all %_requires into a cpanfile
cpanfile: [main, develop, cover]
cpanfile-targets:
# save %_require into cpanfile section
develop: develop
cover: cover

main_requires:
# Needed until preaction/Log-Any#105 is solved.
perl(Storable): '>= 3.06'

develop_requires:
perl(Perl::Tidy): '== 20230912'
perl(Code::TidyAll):
perl(Perl::Critic):
perl(Perl::Critic::Community):

cover_requires:
perl(Devel::Cover):
perl(Devel::Cover::Report::Codecov):
13 changes: 6 additions & 7 deletions lib/OpenQA/Test/PatchDeparse.pm
Original file line number Diff line number Diff line change
Expand Up @@ -15,20 +15,21 @@ 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
package B::Deparse;
no warnings 'redefine';
no strict 'refs';

*{"B::Deparse::walk_lineseq"} = sub {
*{'B::Deparse::walk_lineseq'} = sub {

my ($self, $op, $kids, $callback) = @_;
my @kids = @$kids;
for (my $i = 0; $i < @kids; $i++) {
my $expr = "";
my $expr = '';
if (is_state $kids[$i]) {
# Patch for:
# Use of uninitialized value $expr in concatenation (.) or string at /usr/lib/perl5/5.26.1/B/Deparse.pm line 1794.
Expand All @@ -40,7 +41,7 @@ no strict 'refs';
}
if (is_for_loop($kids[$i])) {
$callback->($expr . $self->for_loop($kids[$i], 0),
$i += $kids[$i]->sibling->name eq "unstack" ? 2 : 1);
$i += $kids[$i]->sibling->name eq 'unstack' ? 2 : 1);
next;
}
my $expr2 = $self->deparse($kids[$i], (@kids != 1)/2) // ''; # prevent undef $expr2
Expand All @@ -60,7 +61,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;


14 changes: 7 additions & 7 deletions lib/OpenQA/Test/TimeLimit.pm
Original file line number Diff line number Diff line change
@@ -1,25 +1,25 @@
# Copyright 2020-2021 SUSE LLC
# Copyright SUSE LLC
# SPDX-License-Identifier: GPL-2.0-or-later

package OpenQA::Test::TimeLimit;
use Test::Most;
use experimental 'signatures';

my $SCALE_FACTOR = $ENV{OPENQA_TEST_TIMEOUT_SCALE_FACTOR} // 1;

sub import {
my ($package, $limit) = @_;
sub import ($package, $limit = undef) {
die "$package: Need argument on import, e.g. use: use OpenQA::Test::TimeLimit '42';" unless $limit;
# 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;
}

sub scale_timeout {
return $_[0] * $SCALE_FACTOR;
sub scale_timeout ($time) {
return $time * $SCALE_FACTOR;
josegomezr marked this conversation as resolved.
Show resolved Hide resolved
}

1;
Expand Down
Loading