Feature/linter

[HerrMuellerluedenscheid]

I propose to add a linter. This will decrease review time and make all receipes more homogeneous in terms of style, thus easier to maintain.

This linter is pretty configurable. I think it's starting pretty strict (which I think is good). But in some cases a more flexible tuning rather with info/warning rather than error can be discussed.

For demonstration purposes I touched the python3-django_2... recipe. Have a look at the linter output: https://github.com/openembedded/meta-openembedded/runs/4036977361?check_suite_focus=true

[kraj]

this looks good start, we will have to see how we can lint the patches submitted to mailing list.

[schnitzeltony]

On Thu, Oct 28, 2021 at 6:01 PM Marius Kriegerowski ***@***.***> wrote: I propose to add a linter. This will increase review time and make all receipes more homogeneous in terms of style, thus easier to maintain. This linter is pretty configurable in terms of configurability. I think it's starting pretty strict (which I think is good). But in some cases a more flexible tuning rather with info/warning rather than error can be discussed.

Hi Marius, Last time I sent a PR to Konrad (1-2 weeks ago), this linter was terribly broken because it was not updated to new override syntax. I looked into and it seems huge efforts to fix it. Have you tested it? Andreas

[schnitzeltony]

Add @priv-kweihmann to conversation

[priv-kweihmann]

Hey guys, if you would point out where it might cause issues with the new override syntax I'd be happy to fix that, because I think (and I use it on the new syntax quite heavily) it should be working pretty okayish.
And some of them reported issue should be rather up for discussion (for instance all those suggestedvar items)... (as already mentioned this tool is pretty much configurable according to the project's needs)

@HerrMuellerluedenscheid now I finally see what you meant with that "false positive" about dependsordered... and I have to admit that this is a valid finding, still I think I should work around it.

Not sure how much use the tool has in openembedded space, but I'd be happy to help if my help is needed.

Anyways, I think a very very trimmed down config of the linter (like something of: only errors) could be applied automatically as part of CI (I myself do that for instance at meta-rubygems with quite a good success rate)

[priv-kweihmann]

this looks good start, we will have to see how we can lint the patches submitted to mailing list.

This is actually the most pressing point, as the GH action just works for GH PRs - in my opinion instead of invoking it here we have to do it on the build that actually also covers things contributed via mail.
There are ways to do that, but all of them come at a cost and I'm not quite sure if meta-openembedded is a good starting point for that (due to the size and complexity of this layer)

[schnitzeltony]

@priv-kweihmann Maybe I interpreted results wrongly but [1] looked to me as if linter is not doing the right thing. And that you applied my PR - although build was red - made me convinced me about that 😁

[1] https://github.com/priv-kweihmann/meta-rubygems/runs/3919649988?check_suite_focus=true

[priv-kweihmann]

@priv-kweihmann Maybe I interpreted results wrongly but [1] looked to me as if linter is not doing the right thing. And that you applied my PR - although build was red - made me convinced me about that 😁

[1] https://github.com/priv-kweihmann/meta-rubygems/runs/3919649988?check_suite_focus=true

Nah, that happened just because I knew it was okay, although the linter warning - but sure that won't happen again😛

For the record: with plain configuration this linter might be more a burden than helpful, that's why I proposed to trim down the config to a bare minimum first

[schnitzeltony]

For the record: with plain configuration this linter might be more a burden than helpful, that's why I proposed to trim down the config to a bare minimum first

Agreed. We have a different situation in meta-openeembedded than you have in your layers:

• meta-oe is sitting on gazillions of un-lintered recipes. E.g complaining about a simple upgrade might annoy contributors
• My opinion: I don't like styleguide issues for many reasons but topmost: They don't enhance the build output and that's my only focus. Get feature-rich and working images. As said: Just my personal opinion.
• In your layers situation is different: You are more or less the only contributor and it seems you started by doing things right from the beginning - and yes your CI machinery really impressed me.

My suggestion:
Error out on errors - warn for styleguide

[HerrMuellerluedenscheid]

I think that we should define a common set of linting rules first. Once they are ready they can be tapped into every place where patches come from downstream.

I disagree that this should kick in on build process. Linters should (and luckily can) be the very first check upon submitting (no matter via email or github). If linting is done later in the process the responsibility of fixing a broken linting result is hard to be communicated back to the person who submitted the patch.

meta-oe is sitting on gazillions of un-lintered recipes. E.g complaining about a simple upgrade might annoy contributors

Yeah, but someone has to get started with that. And I think that the workload is best shared if it happens now slowly over time with every recipe that is being touched. This should be communicated a la: "We have a new linter in place. You may need to fix things which are not your fault but we are very thankful if you take care of that to ensure higher code quality. We really appreciate your effort" (Maybe when opening a new issue)

My opinion: I don't like styleguide issues for many reasons but topmost: They don't enhance the build output and that's my only focus. Get feature-rich and working images. As said: Just my personal opinion.

True for experienced developers. However from my point of view the burdon of getting started in this rather (very) complex eco-system will be lowered if at least all recipes have the very exact same ordering, style, etc. as it reduces confusion. I started about one and a half years ago with yocto and remember thinking: 🤔 does the order matter? Will my recipe still work if SRC_URI is defined after SRCREV?? etc...

I (as a newcomer) am suuuper glad if at least there is a strict linter that tells me how my recipes should look like which in turn gives me confidence that I'm doing the right thing. These are things which I can also ask the community but why should I waste other people's time on that if there is a linter which does the same thing 🤷 ? It also lowers the burdon of submitting patches. People are shy about that in the beginning. If the linter waves with a green flag I at least know that I'm not too far from a proper contribution. That's my personal view on linters (in general) 😊

Error out on errors - warn for styleguide

Sounds good to me. I will throw in the linter configuration so everybody can have a look at what is an error, warning or info.

BTW: I never reaaally understood why people would prefer submitting/accepting patches via email instead of github/gitlab/gitea/whatever merge requests. This process here would be so much easier if only git would be used.

---

After re-reading: I may have sounded harsher than I meant to be in some places 😄 sorry about that :)

[priv-kweihmann]

@HerrMuellerluedenscheid the lately added rule file is definitely a good way to get on with the discussion here.
First of all I would actually remove all info level items

• as they are nice to have but will cause a lot of frustration
• these rules are pretty opinionated about how things should be (and I know a lot of the oe contributors don't necessarily share my views on them)
• those items should be left for local checking IMHO

BTW I got a script at https://github.com/priv-kweihmann/meta-sca-ci-utils/blob/main/scripts/oelint, which also comments back to the PR so ppl actually would know what went wrong (here's the corresponding GH action https://github.com/priv-kweihmann/meta-sca-ci-utils/blob/main/oelint/action.yaml)... maybe that has to be slightly adapter, so feel free to fork that one

[schnitzeltony]

Nothing much to add except:

BTW: I never reaaally understood why people would prefer submitting/accepting patches via email instead of github/gitlab/gitea/whatever merge requests. This process here would be so much easier if only git would be used.

I think the answer is: It has always been like that but this is more @kraj decision. I share your opinion: Github actions have so much to offer and YOCTO is the only project I still send patches by email. At least every time gmail considers git-send-email as unsafe I am asking myself...

After re-reading: I may have sounded harsher than I meant to be in some places 😄 sorry about that :)

Maybe same

[HerrMuellerluedenscheid]

@priv-kweihmann, I actually like opinionated linters as they just safe time that would be wasted on discussion. And given the strict default rules of oelint I'm actually surprised that you dislike black for python 🤣

Okay, I'll remove the info level rules.

[HerrMuellerluedenscheid]

Opened an issue to discuss ways to cleanup the output of oelint and to enhance user experience: priv-kweihmann/oelint-adv#259

Any input welcome! @here

[shr-project]

There used to be automated review script integrated somehow with patchwork and was reporting various issues for patches submitted to oe-core ML.

http://git.yoctoproject.org/cgit/cgit.cgi/patchtest

Here is an random example how the replies from patchtest looked:
https://yhbt.net/lore/all/20181102030313.13842.45534@do.openembedded.org/

@rpurdie mentioned that someone is working on patchwork again, I don't know who, maybe the linter could be integrated there as well.

[HerrMuellerluedenscheid]

Hey @ALL, just wanted to check back in here. @priv-kweihmann and I worked a little to improve the code or oelint during the last week. E.g. now there is an option --relpaths which only shows relative paths in the output which removes a lot of unnecessary text. Checkout the example of this associated MR: https://github.com/openembedded/meta-openembedded/runs/4106189722?check_suite_focus=true

One more proposition is in the pipeline in this issue
Your feedback and input is highy appreciated!

I think it's good to push this issue a little because a proper linter can improve the learning curve a lot when starting to work in a new domain.

[HerrMuellerluedenscheid]

Hey,

From my point of view the linter can be merged. It's a starting point, thus there may be some tweaking required in the future. But I think it's good start to get used to it.
Cheers

[rpurdie]

It probably falls to me to highlight an issue here. Developing linters is great but doing it outside the established development method of the project potentially creates some problems. I mentioned this work to others and they're unaware of it as it isn't on the mailing lists where people expect this kind of development to be done. As such, many developers haven't an opportunity to comment on it or give feedback. I appreciate some people dislike the mailing lists and want to use github only. There are other developers with the opposite view too. Allowing pull requests against meta-openembedded was an experiment which Khem as the maintainer was wanting to undertake but this pull request does move things into trickier territory. If such a change were merged, it could create a lot of friction as patches are meant to be reviewed on the mailing list and feedback from the linter wouldn't make it there. This would mean that either, often it would be ignored, or patches would end up blocked for unknown reasons and neither outcome would be good. If it effectively mandates github pull requests, that changes the development model and that isn't something which should happen by stealth. I'm well aware of the two sides to all of this and as a maintainer of OE-Core, I know all too well the issues involved so in many ways I don't want to get involved here but I do feel I need to mention it.

[priv-kweihmann]

I honestly don't see the issue here - the proposed change is working only on the changed files in a guthub PR, giving almost immediate feedback - so I don't see where a mailing list has to involved, as it simply doesn't affect ML users at all.
And to be totally frank patchwork isn't up to the task either - so for me it boils down to the question if the project does want just a feature or not? (currently I feel the answer is rather not)

[HerrMuellerluedenscheid]

@rpurdie I understand your point. But I also don't think this is a real issue. If contributors prefer to send patches to the mailling list to get pure manual reviews they can still to that. If instead they prefer direct and immediate feedback to be sure that they adhere to a proper structure and did not forget any critical parts, they can open a merge request which is still followed by a manual review with the advantage for the reviewer that he/she saves time as basic linting has already been taken care of.
I personnally share the oppinion that immediate feedback is better, saves time and leads to less frustration as productivity is increased.

Alternatively, if a github action is the wrong place for this linter, could this somehow be integrated into the ML workflow? It's out of my hands then but I think this could be an alternative.

[rpurdie]

I honestly don't see the issue here - the proposed change is working only on the changed files in a guthub PR, giving almost immediate feedback - so I don't see where a mailing list has to involved, as it simply doesn't affect ML users at all. And to be totally frank patchwork isn't up to the task either - so for me it boils down to the question if the project does want just a feature or not? (currently I feel the answer is rather not)

It really depends where this ends up going. If for example you or others "clean up" a recipe to pass the linter checks and some changes then introduce a regression from the linter perspective, is that going to be a problem? If not and it is just extra validation for patches that might be ok but it isn't clear here whether this has been thought about or not.

Also, I know oelint has a reputation of being a bit too pedantic and people don't agree with everything it reports. That leads to a risk that changes here to pass the linter might not be changes the wider community wants.

By discussing this in a github pull request we're already creating a split community where key changes are no longer being discussed in a definitive place where people can watch for and expect them and that in itself is a big issue.

[HerrMuellerluedenscheid]

It really depends where this ends up going.

We will never know if we don't event take the first step.

If for example you or others "clean up" a recipe to pass the linter checks and some changes then introduce a regression from the linter perspective, is that going to be a problem? If not and it is just extra validation for patches that might be ok but it isn't clear here whether this has been thought about or not.

I'm not sure I can follow here. Can you elaborate a little? There is no auto-fixing happening. Did you take a look at what the linter does?

Also, I know oelint has a reputation of being a bit too pedantic and people don't agree with everything it reports. That leads to a risk that changes here to pass the linter might not be changes the wider community wants.

It may have that reputation because people didn't configure the linter. By default it's oppinionated and strict. Those aspects have been discussed and addressed throughout the thread. What is an error or a warning is highly customizable, as are the format and style of the output. Thus, I invite you to give it a try and provide specific feedback on linter settings to help finding a reasonable configuration. I think your input would be a tremendous help.

By discussing this in a github pull request we're already creating a split community where key changes are no longer being discussed in a definitive place where people can watch for and expect them and that in itself is a big issue.

I'll post to the mailing list to draw other peoples' attention to this thread.

EDIT: So, I finally managed to dispatch a message to the automated-testing list which I felt addressed this topic the best. Let me know which other mailing list I should involve.

[rpurdie]

If for example you or others "clean up" a recipe to pass the linter checks and some changes then introduce a regression from the linter perspective, is that going to be a problem? If not and it is just extra validation for patches that might be ok but it isn't clear here whether this has been thought about or not.

I'm not sure I can follow here. Can you elaborate a little? There is no auto-fixing happening. Did you take a look at what the linter does?

I'm assuming the idea behind having a linter run on commits is that the commits would be adjusted to make the linter happier. My worry was someone working through a different workflow regressing changes and adding issues back. I can imagine some scenarios where that could upset the people improving the linter warnings. I also know that some people don't like linter only changes to the code and I don't have a clear idea how the linter is planned to be used here so that was also partly what I was referring to. Would someone be going recipe by recipe to lint meta-oe to pass the linter or is this only on newly added lines? I have used oelint before, I haven't looked at exactly how it is integrated here into github.

It may have that reputation because people didn't configure the linter. By default it's oppinionated and strict. Those aspects have been discussed and addressed throughout the thread.

Keep in mind that you've discussed it with a limited number of people in a place most people don't know to look.

I'm not against the idea of the linter, my biggest worry here is that we're now effectively creating two different workflows for developing features and merging patches. The patch on mailing list vs pull request issue has been discussed before and I'm very worried about fragmenting the project and it's developers. As the OE-Core maintainer, I know an inevitable question will be "when do we enable this for core?" and I also know how that will cause friction elsewhere. I appreciate I'm looking into the future but for better or worse I do need to do that and I do have some idea of what can happen from experience. It does make me look rather negative unfortunately which isn't my intention. Equally, if I just ignore this until it becomes a bigger issue, that isn't fair to you (the authors of the change), the maintainers or the wider community either so I can't win.