move QA directives into ebuilds and eclasses

[ferringb]

This relates to #762; in that, one must hardcode certain eclasses into pkgcheck. The source being examined, it already knows what it's doing and where it may cause QA issues. It should inform pkgcheck of where it's knowingly doing something naughty, or tell pkgcheck how it needs to be checked.

The rough idea is to move all qa directives- the hardcoded eclass awareness or ebuild awareness- down into the source, and since we can extract it via tree-sitter-bash, do so.

I'm assuming part of the "only run this on gentoo repos" is restricting standards enforcement. Gentoo-x86 has a higher enforcement, specifically. To address this would require a repository level directive, which isn't bash- another file in the repo. Do we already have this?

What I'm proposing for the bash syntax is thus:

ebuilds:

__QA_EBUILD_DIRECTIVE__=( check1:enable, check2:disable, check3:enable:param1=blah:param2:param3 )

Eclass would be __QA_ECLASS_DIRECTIVE__, but the same syntax.

I'm proposing this parse syntax:

check_config: dict[str, dict[str, str]] = {}
for x in array.split():
  check, s = x.split(":", 1)
  settings = {}
  for x in s.split(","):
     if '=' not in x:
        x += "=enable"
     k,v = x.split("=", 1)
     settings[k] = v
  check_configs[check] = settings

As to the mapping of "these settings" to "this check", I expect just using the check name (case sensitive) is best. That feels noisy however, but I think this is safest while also making backwards compatibility of renaming checks trivial to address- it's just an alias of what QA directive to load the name(s) from.

Lack of any setting, for example, just a raw ( check ), is implicitly check:enable=true. Sub settings that lack an assignment are implicity true. check:enable is check:enable=true. These are equivalent: check:requires_bash5.3 == check:requires_bash5.3=enable. IE these are all the same: check1 == check1:enable == check1:enable=true

I specifically propose this as a single var; this reduces possible leaks of QA directives in source whilst also allowing to flag directives that no longer mean anything.

I don't propose this as a metadata variable, although that would optimize things. Until some day we have layered metadata caches (this for emerge usage, that for QA), it's not worth the cost. If this comes to pass, then something like ``QA_DIRECTIVE`- eclass and ebuilds- makes sense, since we can treat it as stacked, and just render it L -> R to get the final directives.

The origin of this idea is that I'm actually in the process of moving all snakeoil and pkgcore python QA enforcement over to something similar. That code however self-annotates; I'm proposing we do the same to bash.

[ferringb]

/CC @thesamesam , @ulm , @arthurzam

[thesamesam]

I like the idea. I don't really care how it looks (whatever is fine with me).

A related idea came up in #478 (I think you could debate if it's the same thing or not). Arthur has an impl in #535 but we never merged it, I'm not sure why.

[ferringb]

My bad; I didn't check pre-existing tickets, but I should have. I'm fine w/ whatever, as long as it's tightly defined akin to what I'm suggesting.

To wit- one reason to use a bash variable here is that we can enforce it via tree-sitter (including detecting in walking the tree if someone tries to make this dynamically set), and the basic idea, if it's exposed as bash, may be something we can wire down into EBD. If it makes sense, something we could standardize into PMS and allow the PM to progressively enforce.

Doing so- as long as it's treated as optional- would allow annotating ebuilds in a way that would constrain to support future EAPIs. I don't know how to say this better, but if we know XYZ we're going to disallow but that ebuild is EAPI10- not yet able to go to EAPI11- we want that ebuild to be able to state "treat me stricter". If the PM supports it, cool, if not, whatever.

My only concern with that proposal is portage historically embraced/extended this into something that became a defacto standard everything else had to emulate irregardless of specs. I don't have a solution to that problem, but if it's resolvable, and this has value, it's an idea.

@arthurzam I find your version easier to read, but it doesn't have the possibility of what I mentioned above.

What are folks views here?

[ulm]

Not sure if I understand this correctly. Where would __QA_ECLASS_DIRECTIVE__ be defined? Not as a variable in the eclass, I hope? (Because starting with __, it is a name reserved for the package manager.)

[ferringb]

Not sure if I understand this correctly. Where would __QA_ECLASS_DIRECTIVE__ be defined? Not as a variable in the eclass, I hope? (Because starting with __, it is a name reserved for the package manager.)

I explicitly intended it as a variable per the reasons I explained. Doing so also would allow some PM enforcement of the format during regen (a very basic "it must be this format" which can be formalized into EAPI standards).

What would you propose as a variable name?

[ulm]

I explicitly intended it as a variable per the reasons I explained. Doing so also would allow some PM enforcement of the format during regen (a very basic "it must be this format" which can be formalized into EAPI standards).

What would you propose as a variable name?

Just without the leading (and trailing) underscores? We have precedent with variables like QA_PREBUILT and QA_CONFIG_IMPL_DECL_SKIP which have somewhat similar purpose and affect the package manager or QA tools.

[ferringb]

I can live with that, although __.*__ I tend to lean towards just to keep ebuilds and eclasses out of intersecting the inline metadata.

So: bikeshedding. is QA_DIRECTIVES acceptable?

Also: if __.*__ is strictly reserved for the PM, this in turn implies the PM must keep it's bash bits with that naming, right? I ask because portage doesn't which isn't much of a surprise.

I'll audit pkgcore to ensure there was never a violation of this, but pkgcore should be clean on this.

[radhermit]

In case inspiration from another implementation is helpful for pkgcheck work, pkgcruft has supported ignore directives since pkgcraft/pkgcraft@1cebfa1 that can be set at the repo, category, or package level via .pkgcruft-ignore files (see examples in the test data repo at https://github.com/pkgcraft/pkgcraft-testdata/tree/main/repos/valid/qa-primary) or in the initial ebuild comment lines (e.g. https://github.com/pkgcraft/pkgcraft-testdata/blob/main/repos/valid/qa-primary/Ignore/IgnoreUnused/IgnoreUnused-0.ebuild#L1). Note that eclasses aren't supported yet (pkgcraft/pkgcraft#256).

The directives are layered on top of each other based on their scope and allow nested scopes to override higher ones, e.g. a set of report variants can be disabled for an entire repo and then re-enabled on a package or version basis. As a scanning run occurs the directives are parsed and cached as necessary in parallel. In addition, the pkgcruft ignore command can be used to show all the ignore directives for a given target scope.

[radhermit]

Also just to note, personally I prefer a simpler comment-based notation (or regular lines for external files). The examples above seem overly complex compared to what it seems like would be useful which is also because they use bash notation. I took inspiration for gitignore files and ruff directives (or similar linters) for pkgcruft directives assuming most devs should be familiar with one of those formats.

Rolling comment-based directives from eclasses across an inheritance structure internally shouldn't be that hard because that's what eclassdoc parsing already provides for other pkgcheck support.

In any case, feel free to ignore my opinions since I moved on from pkgcore/pkgcheck/etc a long time ago and having a semi-similar directive format isn't that valuable since it's highly unlikely they could ever be directly translated.

[ferringb]

Inline comments are superior in fidelity and 'better'. They however forces a bash parse for all QA checks even if the check suite is strictly metadata scope and higher, which is why I just reached for the bash variable 'hammer'.

Enumerating what I think the clear design requirements are:

• opt-in and out of results. Meaning disable and enable. If it's fine grained- meaning the parse tree has to be tracked for directives- then there is no reason to not allow a child scope to re-enable a check a parent scope disabled.
• Suppression of groups of results (or class of results), meaning suppression of a check. Tim- this is akin to your @Header syntax, I assume.
• Some way to address what I mentioned at the beginning; not requiring the full bash parse just to know what metadata (or higher) scope of checks are disabled. If one requires that sort of directive be in the first few lines of the ebuild (or eclass if inheriting), that can do it. I just hate that sort of design.
• QA directives have to be human understandable: not #noqa: F123, #noqa: result/whatever.
• The dev must be able to atach the directive to the scope of a thing; the function, the pipeline, the case/if/for, etc.

These are the things that aren't as clear, but should be thought through:

• How should eclass directives be treated for the ebuild? There are eclass specific checks (rust checks keyed to if cargo is in pkg.inherited). Eclasses should be able to turn on specific QA checks for their consumers. This will complicate the initial pkgcheck pipeline graph, but let's assume it's doable.
• How can these directives allow non pkgcheck settings? Pkgcruft for example. There is a namespacing problem here- we must do QA checks against the QA directives (report if there are unrecognized checks, for example). Something like pkgcruft:BrianPrefersRust seperates it so pkgcheck doesn't warn on an unknown directive. No 'namespace' defaults to 'pkgcheck', or whatever the current "this is the QA tool of gentoo ecosystem" becomes.
• Repositories themselves need to be able to define their QA profile. pkgcheck has the suite and policy of gentoo-x86 hardcoded into it. Thought should be given to how other repo's would want to define their QA suite. A very simple and stupid example here is the gentoo copyright is only enforced on gentoo-x86, whilst gentoo overlays probably want that also. Or chromiumos wanting their particular copyright enforced.

The copyright check is an example of where it's preferable a single check be able to be told how it should be configured, but that is very much a V2. Assume that they'll have their own tooling or checks that they run that may not be part of pkgcheck (or eventually, not living in pkgcheck but ran by it).

One thing I hate in all QA directive syntaxs I've seen, is that the 'why' is never attached to the disable. You're lucky if you get a comment next to the disable, but that's rare. If the syntax were to allow putting the reason in- alongside directive- that can solve that complaint. Knowing why something was disabled makes it simpler to know if you can rip it out.

What are peoples thoughts? Anything I missed or you think should be added, please speak up. I suspect this may require a GLEP, so we might as well get some consensus here, first.

Tim- anything you specifically would want considered that pkgcruft can do, but pkgcheck can't, would be good to hear if it has any relevance to any of this. I wrote the above as basically a first pass of the stuff that seemed 'obvious' in how folks are talking about this.

[ferringb]

Also: if folks think it's better to just do # pkgcheck-QA: blah blah blah, that's an option. My concern is that some rewrites i'm doing on pkgcheck are going to allow 3rd party checks as a side effect of the main fix. So that QA check of the directives problem will exist either way, whether N tools share the same scheme, or if it's just pkgcheck.

[radhermit]

Suppression of groups of results (or class of results), meaning suppression of a check. Tim- this is akin to your @Header syntax, I assume.

Yes, although it feels more cohesive in pkgcruft compared to what was implemented in pkgcheck, but pkgcruft's documentation is nonexistent so it's not that apparent right now. Meaning the @ReportSet syntax can be used in the same fashion for all relevant options/arguments while in pkgcheck it is more of a patchwork of semi-standard aliases across a handful of options depending on context if it hasn't changed.

Tim- anything you specifically would want considered that pkgcruft can do, but pkgcheck can't, would be good to hear if it has any relevance to any of this. I wrote the above as basically a first pass of the stuff that seemed 'obvious' in how folks are talking about this.

Mostly I think extending directive support across different scopes like I mentioned previously could be nice instead of only supporting it at the ebuild file level. Beyond that, much of what pkgcruft can do that pkgcheck can't as easily in this situation (e.g. lazy concurrent ignore cache support) generally relates to rust vs python (or just the overall design) and isn't relevant.

How can these directives allow non pkgcheck settings?

Personally I don't think pkgcheck should care about this at all unless you want to GLEP-ify the process and make the directive format more standardized. I mostly added directive support to pkgcruft on a whim (which extends to most of pkgcruft generally speaking) to see how lazy concurrency works for supported data structures in the current threadpool parallelization design. In general, I don't see pkgcruft having much relevance for Gentoo development at large any time soon, if ever (unless someone wants to drive server-side git hook scanning via pkgcruft-git or other related initiatives), so it's not worth the coordination effort.

The dev must be able to atach the directive to the scope of a thing; the function, the pipeline, the case/if/for, etc.

This reaches beyond the scope of ebuild directives into bash-level linter directives which is fine if you want to implement that, but I don't think the current design of pkgcheck provides that level of contextual awareness and trying to hack it via tree-sitter's very LSP-related parser API is bound to be painful at any meaningful level of complexity.

One thing I hate in all QA directive syntaxs I've seen, is that the 'why' is never attached to the disable.

This is a good point, but I'd argue that it's better to use regular code comments for documenting them (which could have basic checks to verify they exist if you really want that), but those could be in a semi-standardized format like the package.mask entry headers if you'd like actually parsing them to be more sane.