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

Feature request: explicitly check more than just .bb/.bbappend #607

Closed
kergoth opened this issue Aug 12, 2024 · 6 comments
Closed

Feature request: explicitly check more than just .bb/.bbappend #607

kergoth opened this issue Aug 12, 2024 · 6 comments
Assignees
Labels
enhancement New feature or request

Comments

@kergoth
Copy link

kergoth commented Aug 12, 2024

The current behavior, as far as I can tell, is to accept any number of files on the cmdline, but then it only actually checks the .bb/.bbappend files and the files inherited/included by those (GetRecipes, etc on the Stash). This is not intuitive, in my opinion. Normal behavior for a cmdline tool given a path on the cmdline is to operate on that path, or error out. In the case of a linting tool, I'd expect it to check the specified files.

I specifically want to use rules which check layer.conf files, distros, machines, etc, but that can't be done by the default cli tool. Admittedly, it works just fine to use it in library mode and copy and modify the cli functions to check the other files also, so I've done so, but I feel like the scope of this tool given its README and the name of the command both imply that it's capable of more than just recipes as the entry point, so I'd really like to see this expanded if at all possible. I'm willing to do the work myself and submit a PR if that's preferred and you're open to this enhancement.

Thanks for your time. You've done good work on this. The tool is nice, and while I can't say I agree with the default rules, it's easy enough to adjust that to personal preference.

@priv-kweihmann
Copy link
Owner

So we are talking mainly about .conf files, right?
If so a patch could easily inject them into each of the determined run groups (*.bb with appends + *.bbappend without a matching recipe).

But if we are talking about arbitrary files, I guess the current grouping algorithm is ill prepared for that and I can't imagine a way to do it without the knowledge of a full bitbake build, so I tend to be in favor of adding .conf files to the mix, but nothing beyond that (leaving other cases to the already mentioned library mode).

Please confirm that .conf files are enough, then I can spin up a patch later that week

@priv-kweihmann priv-kweihmann added the enhancement New feature or request label Aug 13, 2024
@kergoth
Copy link
Author

kergoth commented Aug 13, 2024

.conf would certainly be enough for now. I doubt anyone cares to lint a .inc that isn't included, for example. I could, however, picture a layer that provides optional bbclasses to be added by the user to INHERIT in local.conf, so it's not explicitly inherited by any recipes out of the box. I think you'd want to be able to lint arbitrary classes as well to cover that case.

@priv-kweihmann
Copy link
Owner

I started working on the issue, but it looks like it's more complicated than I initially thought.
Anyway, likely I can make the requested feature happen.

Only limitation so far (that I can see) is that there won't be support for local.conf and site.conf (iirc this concept has been abandoned anyway), as those files are not layer specific, hence requiring a full build to get the required information, such as location of the layers to inherit classes from there.

@kergoth btw do for happen to have an example check that you want to see in the linter, that explicitly only should be applied to .conf files?
Would be great to add support right out of the box (also makes testing easier)

@priv-kweihmann priv-kweihmann self-assigned this Aug 14, 2024
@kergoth
Copy link
Author

kergoth commented Aug 14, 2024

That makes sense regarding local.conf and site.conf. Regarding default rules, the ones I'm using here are specific to how we're doing things (and are actually against an isar-based layer, not oe), but I can think of a number of possibilities regarding ensuring layer.conf, machine .conf, and distro .conf are well behaved. For example, one could check that the boundaries between machine, distro, and image axes are properly maintained. That is, no machine should be setting DISTRO_FEATURES, or vice versa, for example. That's off the top of my head, but there are probably others regarding well behaved configs. A layer.conf probably shouldn't be mucking about with image variables, or distro, or machine. We could check to verify that the layer.conf adds to BBFILE_COLLECTIONS and defines the expected layer variables too.

priv-kweihmann added a commit that referenced this issue Aug 23, 2024
to the run groups in the way bitbake
does it as well.
The order of conf files is the following

- layer.conf
- machine configs
- distro configs

Closes #607

Signed-off-by: Konrad Weihmann <[email protected]>
priv-kweihmann added a commit that referenced this issue Aug 23, 2024
to check on discouraged variables in
distro configurations

Relates to #607

Signed-off-by: Konrad Weihmann <[email protected]>
priv-kweihmann added a commit that referenced this issue Aug 23, 2024
to check that only layer.conf specific variables
are set in a layer.conf

Relates to #607

Signed-off-by: Konrad Weihmann <[email protected]>
priv-kweihmann added a commit that referenced this issue Aug 23, 2024
to check for discouraged variables in a machine conf

Relates to #607

Signed-off-by: Konrad Weihmann <[email protected]>
@priv-kweihmann
Copy link
Owner

@kergoth I started to slowly push the requested changes.
Will try to add the missing tests throughout the next week.
But if you like you can already have a look at https://github.com/priv-kweihmann/oelint-adv/tree/feat/conf-support and let me know if that fits what you asked for

priv-kweihmann added a commit that referenced this issue Aug 27, 2024
to enable even more throughout checks,
build permutations of

- inline branch expansion
- (opt) layer.conf files
- (opt) machine.conf files
- (opt) distro.conf files
- each calculated file group

to enable a better coverage.
The use permutation is appended to the findings's
message automatically.

Relates to #607

Signed-off-by: Konrad Weihmann <[email protected]>
@kergoth
Copy link
Author

kergoth commented Aug 28, 2024

@kergoth I started to slowly push the requested changes. Will try to add the missing tests throughout the next week. But if you like you can already have a look at feat/conf-support and let me know if that fits what you asked for

Looking good, thanks for your work on this!

priv-kweihmann added a commit that referenced this issue Sep 3, 2024
to check on discouraged variables in
distro configurations

Relates to #607

Signed-off-by: Konrad Weihmann <[email protected]>
priv-kweihmann added a commit that referenced this issue Sep 3, 2024
to check that only layer.conf specific variables
are set in a layer.conf

Relates to #607

Signed-off-by: Konrad Weihmann <[email protected]>
priv-kweihmann added a commit that referenced this issue Sep 3, 2024
to check for discouraged variables in a machine conf

Relates to #607

Signed-off-by: Konrad Weihmann <[email protected]>
priv-kweihmann added a commit that referenced this issue Sep 3, 2024
to enable even more throughout checks,
build permutations of

- inline branch expansion
- (opt) layer.conf files
- (opt) machine.conf files
- (opt) distro.conf files
- each calculated file group

to enable a better coverage.
The use permutation is appended to the findings's
message automatically.

Relates to #607

Signed-off-by: Konrad Weihmann <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants