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

CI: add oelint-adv based linter for our own recipes #224

Open
wants to merge 32 commits into
base: scarthgap
Choose a base branch
from

Conversation

hnez
Copy link
Member

@hnez hnez commented Dec 9, 2024

oelint-adv is a linter for bitbake recipes.

Right now there are still a lot of warnings that need to fixed or configured out (if appropriate).

@hnez hnez force-pushed the oelint branch 3 times, most recently from 06c6614 to 38354bd Compare December 20, 2024 13:33
@hnez
Copy link
Member Author

hnez commented Dec 20, 2024

It may have taken me a day, but now we are down to only two oelint warnings:

…/meta-lxatac-software/recipes-core/images/lxatac-core-image-base.bb:8:error:oelint.vars.specific:'EXTRA_IMAGECMD' is set specific to ['ext4'], but isn't known from PACKAGES, MACHINE, DISTRO or resources [branch:false]
…/meta-lxatac-software/recipes-core/images/lxatac-core-image-base.bb:14:error:oelint.vars.specific:'EXTRA_IMAGECMD' is set specific to ['ext4'], but isn't known from PACKAGES, MACHINE, DISTRO or resources [branch:false]

I do not want to disable the error but I also do not know what it is trying to tell me, so I am hoping @jluebbe or @ejoerns may be able to give me some advise on those.

All in all I found the suggestions I got from oelint quite helpful. And If one does not have to fix a huge list of them all at once they should also not get in the way too much.

I would vote in favor of using it here and in other projects as well.

@hnez hnez marked this pull request as ready for review December 20, 2024 13:36
@hnez hnez requested a review from ejoerns December 20, 2024 13:36
@hnez
Copy link
Member Author

hnez commented Jan 2, 2025

I've had a second look at the oelint.vars.specific:'EXTRA_IMAGECMD' error and now think that it can be safely disabled.
The poky/meta/classes-recipe/image_types.bbclass also include a reference to EXTRA_IMAGECMD:ext4 (along with other filesystem specific variants).

I've added an exception to lxatac-core-image-base.bb.

Now the oelint job runs without errors (there is still room for improvement though as the .oelint.cfg disables some checks).

hnez added 22 commits January 3, 2025 07:03
The grpc based labgrid version introduced in e3b9c26 ("meta-labgrid:
update to a version that adds grpc support") no longer uses the
`LG_CROSSBAR` environment variable.

Instead `LG_COORDINATOR` has to be set.

Fixes: e3b9c26 ("meta-labgrid: update to a version that adds grpc support")
Signed-off-by: Leonard Göhrs <[email protected]>
Using underscores there was a how it was done in previous yocto versions
but since then `RDEPENDS:${PN}` is the way to go an `RDEPENDS_${PN}`
just creates some new variable that no one reads.

This was found by oelint oelint.vars.mispell[1].

[1]: https://github.com/priv-kweihmann/oelint-adv/blob/master/docs/wiki/oelint.vars.mispell.md

Signed-off-by: Leonard Göhrs <[email protected]>
This disables some checks (not because they are necessarily wrong,
but because fixing them is sometimes more work than I am willing to
invest right now) and adds some variable names that we know are not
typos.

Signed-off-by: Leonard Göhrs <[email protected]>
Add a GitHub workflow that runs oelint-adv[1] on our custom
bitbake files.

[1]: https://github.com/priv-kweihmann/oelint-adv

Signed-off-by: Leonard Göhrs <[email protected]>
This fixes oelint.vars.summary80chars[1] warnings for summaries that were
in fact descriptions and oelint.var.mandatoryvar.SUMMARY[2] errors for
recipes without a summary.

Demoting some of the DESCRIPTIONs to SUMMARYs would usually trigger
oelint.var.mandatoryvar.DESCRIPTION[2] errors, but those are currently
disabled.

[1]: https://github.com/priv-kweihmann/oelint-adv/blob/master/docs/wiki/oelint.vars.summary80chars.md
[2]: https://github.com/priv-kweihmann/oelint-adv/blob/master/docs/wiki/oelint.var.mandatoryvar.md

Signed-off-by: Leonard Göhrs <[email protected]>
This solves a oelint.vars.duplicate[1] oelint warning because sysconfdir
is already included in FILES by default.

[1]: https://github.com/priv-kweihmann/oelint-adv/blob/master/docs/wiki/oelint.vars.duplicate.md

Signed-off-by: Leonard Göhrs <[email protected]>
… .inc

This fixes oelint.var.override[1] oelint errors because both the .inc file
and the .bb that includes set the same variable.

[1]: https://github.com/priv-kweihmann/oelint-adv/blob/master/docs/wiki/oelint.var.override.md

Signed-off-by: Leonard Göhrs <[email protected]>
….conf

This silences a oelint.file.requirenotfound[1] oelint warning about the
file not being found. The files does however exist in
`poky/meta/conf/image-uefi.conf` and is actually found and required while
building.

[1]: https://github.com/priv-kweihmann/oelint-adv/blob/master/docs/wiki/oelint.file.requirenotfound.md

Signed-off-by: Leonard Göhrs <[email protected]>
Instead of the whole util-linux suite.

Signed-off-by: Leonard Göhrs <[email protected]>
Oelint warns about a possible oelint.vars.downloadfilename[1] problem
because the download URL does not include `${PV}`.
The download does however include the version via `${LINUX_VERSION}`,
so we we are fine here.

https://github.com/priv-kweihmann/oelint-adv/blob/master/docs/wiki/oelint.vars.downloadfilename.md

Signed-off-by: Leonard Göhrs <[email protected]>
hnez added 10 commits January 3, 2025 08:01
…elint

This ignores a valid oelint warning about `do_compile[network] = "1"`
in the form of oelint.task.network[1].

This is sadly a common problem with Go code.

[1]: https://github.com/priv-kweihmann/oelint-adv/blob/master/docs/wiki/oelint.task.network.md

Signed-off-by: Leonard Göhrs <[email protected]>
This ignores a valid oelint warning about `do_compile[network] = "1"`
in the form of oelint.task.network[1].

This is sadly a common problem with Go code.

[1]: https://github.com/priv-kweihmann/oelint-adv/blob/master/docs/wiki/oelint.task.network.md

Signed-off-by: Leonard Göhrs <[email protected]>
This fixes an oelint.bbclass.underscores[1] error:

> Bitbake tries to create shell and/or python functions out of the base
> name of the class when using EXPORT_FUNCTIONS in this particular class.
> `-` is unfortunately not a valid symbol in neither shell nor python.

[1]: https://github.com/priv-kweihmann/oelint-adv/blob/master/docs/wiki/oelint.bbclass.underscores.md

Signed-off-by: Leonard Göhrs <[email protected]>
…patch

The patch turns off a type of compiler warning that should rather be fixed
in the software intself, hence why it is inappropraite for upstreaming.

This fixes a oelint.file.upstreamstatus[1] oelint info message.

[1]: https://github.com/priv-kweihmann/oelint-adv/blob/master/docs/wiki/oelint.file.upstreamstatus.md

Signed-off-by: Leonard Göhrs <[email protected]>
This fixes a oelint.file.requirenotfound[1] oelint warning for the
non-existing tacd-${PV}.inc file and a oelint.file.requireinclude[2]
warning for the tacd.inc file.

[1]: https://github.com/priv-kweihmann/oelint-adv/blob/master/docs/wiki/oelint.file.requirenotfound.md
[2]: https://github.com/priv-kweihmann/oelint-adv/blob/master/docs/wiki/oelint.file.requireinclude.md

Signed-off-by: Leonard Göhrs <[email protected]>
This solves a oelint.vars.duplicate[1] oelint warning because bindir
is already included in FILES by default.

[1]: https://github.com/priv-kweihmann/oelint-adv/blob/master/docs/wiki/oelint.vars.duplicate.md

Signed-off-by: Leonard Göhrs <[email protected]>
…order

This gives the variables a canonical order.

Signed-off-by: Leonard Göhrs <[email protected]>
This disables the following oelint error message:

 $ oelint-adv --mode=all --quiet .../images/lxatac-core-image-base.bb
 .../images/lxatac-core-image-base.bb:14:error:oelint.vars.specific:
 'EXTRA_IMAGECMD' is set specific to ['ext4'], but isn't known from
 PACKAGES, MACHINE, DISTRO or resources [branch:false]

The EXTRA_IMAGECMD:ext4 variable is however also used in meta-oe
and other filesystem-specific variants are also used elsewhere.
It should thus be okay to use it and disable the error message.

Signed-off-by: Leonard Göhrs <[email protected]>
@@ -111,14 +111,14 @@ SRC_URI[windows_x86_64_gnu-0.52.6.sha256sum] = "147a5c80aabfbf0c7d901cb5895d1de3
SRC_URI[windows_x86_64_gnullvm-0.52.6.sha256sum] = "24d5b23dc417412679681396f2b49f3de8c1473deb516bd34410872eff51ed0d"
SRC_URI[windows_x86_64_msvc-0.52.6.sha256sum] = "589f6da84c646204747d1270a2a5661ea66ed1cced2631d546fdfb155959f9ec"

LIC_FILES_CHKSUM = " \
LIC_FILES_CHKSUM = "\
Copy link
Collaborator

Choose a reason for hiding this comment

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

This better fits the next commit.

@@ -1,4 +1,4 @@
DESCRIPTION = "Set up LG_CROSSBAR environment variable from labgrid env"
SUMMARY = "Set up LG_COORDINATOR environment variable from labgrid env"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Change from LG_CROSSBAR to LG_COORDINATOR better fits the commit that actually changes it.

@@ -4,12 +4,12 @@
# umpf-topic: v2.9/customers/lxa/tac
# umpf-hashinfo: e9c0f04902a5616a0af04fc9490edd9f1273ab7c
# umpf-topic-range: d3e71ead6ea5bc3555ac90a446efec84ef6c6122..927790a02af8f782da3c11fc599017767d175de8
SRC_URI += "\
SRC_URI:append = " \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesn't make sense since this file is auto-generated by umpf.

I'm also not fully convinced that ignoring += is the way to go.

An argument in https://github.com/priv-kweihmann/oelint-adv/blob/8096d573cce0c093179957d5b274b895a63bb352/docs/wiki/oelint.vars.srcuriappend.md is the potential issue with SRC_URI ?=, but I can't find any oe-core or meta-oe recipe that actually uses a default assignment here.

priv-kweihmann/oelint-adv#566 also touches this topic.

@@ -17,7 +17,7 @@ inherit kernel-arch deploy cml1 pkgconfig
# be enabled within the defconfig file so depend on it here to be more user
# friendly (do not abort the build). The actual tools are provided by the
# barebox-tools package.
DEPENDS = "bison-native flex-native libusb1"
DEPENDS:append = " bison-native flex-native libusb1"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd find the solution to set DEPENDS before inherit more simple here.
This is how almost all oe-core recipes do it.

(Will be done in a later commit anyway)

@@ -21,6 +21,7 @@ RDEPENDS:${PN}:remove = "nodejs"

WEBUI_INSTALL_DIR = "${NPM_BUILD}/lib/node_modules/tacd-web"

npm_run_build[doc] = "Run the build script that transpiles the source typescript to javascript"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Using a [postfunc] is probably smarter here anyway?

@@ -7,6 +7,7 @@ SECTION = "kernel"
LICENSE = "GPL-2.0-only"
LIC_FILES_CHKSUM = "file://COPYING;md5=6bc538ed5bd9a7fc9398086aedcd7e46"

# nooelint: oelint.vars.downloadfilename
Copy link
Collaborator

Choose a reason for hiding this comment

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

In the corresponding commit message:

- so we we are fine here.
+ so we are fine here.

@@ -1,4 +1,4 @@
inherit panel-mipi-dbi
inherit panel_mipi_dbi
Copy link
Collaborator

Choose a reason for hiding this comment

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

I just wonder how this works with all the oe-core classes containing dashes, like kernel-yocto.bbclass and what the actual problem this causes is.
But haven't investigated further.

@@ -5,12 +5,14 @@ IMAGE_FEATURES = "ssh-server-openssh empty-root-password tools-debug lic-pkgs"
IMAGE_FSTYPES += "ext4"

# use a fixed directory hash seed to reduce the image delta size
# nooelint: oelint.vars.specific
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes. Actually, these are not really overrides but use the same syntax here.

For IMAGE_CMD this is quite clear, but for EXTRA_IMAGECMD I am not really sure how this works.

@@ -2,4 +2,4 @@

# export labgrid environment to be used with labgrid-client on the lxatac
source /etc/labgrid/environment
export LG_CROSSBAR="ws://${LABGRID_COORDINATOR_IP:?}:${LABGRID_COORDINATOR_PORT:?}/ws"
export LG_COORDINATOR="${LABGRID_COORDINATOR_IP:?}:${LABGRID_COORDINATOR_PORT:?}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe move this change to a different PR?

It is not really oelint-adv-related.


DEPENDS = "dtc-native"
DEPENDS += "dtc-native"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I can't see how this variable is overridden in the .bb file. Unrelated?

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.

2 participants