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

skip annotations for version results #535

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

arthurzam
Copy link
Member

@arthurzam arthurzam commented Jan 29, 2023

Add support for an ebuild to declare a skip annotation for a class of version results. This skip annotation must be declared before any non-comment non-empty line (most of the times it would be before the EAPI declaration). Empty lines between the copyright and the annotation are allowed.
The annotation line must be precise, only one class per line and every space is required.

For example, it would look like:

# Copyright 2023 Gentoo Authors
# Distributed under the terms of the GNU General Public License v2

# pkgcheck skip: PythonCompatUpdate

# Approved by larry 2023-04-31
# pkgcheck skip: VariableScope

EAPI=8

Resolves: #478

More thoughts

  1. Is the format fine?
  2. Before merge to master/before release, we must define a QA policy who and when can add a skip.
    As preliminary idea from me (non QA member!): error none, warning only with QA approval, info/style anyone?
  3. Am I too hard with the syntax and placing?
  4. How and if we should support annotation for package ("dir") results?
  5. Should I support multiple class names in same line, separated with commas?
  6. Should I add special syntax for placing QA member name and date for those that require approval?
  7. Should I post to -dev ML when ready?

TODO:

  • This should be just after EAPI line instead of before
  • Should support comma separated list of classes on the same line
  • Post to ML for comments
  • Open Question for ML: auto approval on ebuild bump?

@arthurzam arthurzam requested a review from thesamesam January 29, 2023 19:59
@codecov
Copy link

codecov bot commented Jan 29, 2023

Codecov Report

Patch coverage: 31.57% and project coverage change: -0.12 ⚠️

Comparison is base (eb6a62d) 80.24% compared to head (b4b42bb) 80.12%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #535      +/-   ##
==========================================
- Coverage   80.24%   80.12%   -0.12%     
==========================================
  Files          56       56              
  Lines        8452     8470      +18     
  Branches     1595     1922     +327     
==========================================
+ Hits         6782     6787       +5     
  Misses       1580     1580              
- Partials       90      103      +13     
Impacted Files Coverage Δ
src/pkgcheck/results.py 85.78% <0.00%> (-0.85%) ⬇️
src/pkgcheck/sources.py 58.14% <35.29%> (-1.70%) ⬇️

... and 7 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@radhermit
Copy link
Contributor

radhermit commented Jan 29, 2023

If this is going to be supported in the discussed format, I'd highly recommend pulling the pragma values during source object iteration into packages... basically wrap actual package objects adding an attribute for the result class skip set (e.g. see how the bash parse tree source works). This change currently operates at a result level which, in theory, can result in a lot of redundant work (it's reading the ebuild file per version result and each ebuild can create lots of version results), not to mention having to special case package types.

In theory, implementing it like that would even make it possible to add the contextual information into the pipeline before the checks get run so packages that ignore all the results a check can return wouldn't even run the check.

@arthurzam arthurzam marked this pull request as draft January 31, 2023 06:09
@arthurzam arthurzam marked this pull request as ready for review May 11, 2023 18:13
@arthurzam
Copy link
Member Author

An update based on benchmarks done by sam, mgorny and me.
The performance impact of the new implementation is negligible when done on full tree.

Add support for an ebuild to declare a skip annotation for a class of
*version* results. This skip annotation must be declared before any
non-comment non-empty line (most of the times it would be before the
EAPI declaration). Empty lines between the copyright and the annotation
are allowed.
The annotation line must be precise, only one class per line and every
space is required.

For example, it would look like:

  # Copyright 2023 Gentoo Authors
  # Distributed under the terms of the GNU General Public License v2

  # pkgcheck skip: PythonCompatUpdate

  EAPI=8

Resolves: pkgcore#478
Signed-off-by: Arthur Zamarin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support annotations to silence warnings in ebuilds
2 participants