search

priv-kweihmann / oelint-adv

Advanced oelint
BSD 2-Clause "Simplified" License
56 stars 28 forks source link

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

Closed kergoth closed 2 months ago

kergoth commented 3 months ago

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 commented 3 months ago

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

kergoth commented 3 months ago

.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 commented 3 months ago

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)

kergoth commented 3 months ago

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 commented 3 months ago

@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

kergoth commented 2 months ago

@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!