-
Notifications
You must be signed in to change notification settings - Fork 700
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/linter #465
base: master
Are you sure you want to change the base?
Feature/linter #465
Conversation
this looks good start, we will have to see how we can lint the patches submitted to mailing list. |
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
|
Add @priv-kweihmann to conversation |
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. @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) |
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. |
@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 |
Agreed. We have a different situation in meta-openeembedded than you have in your layers:
My suggestion: |
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.
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)
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 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) 😊
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 :) |
54bfc9f
to
b828422
Compare
@HerrMuellerluedenscheid the lately added rule file is definitely a good way to get on with the discussion here.
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 |
Nothing much to add except:
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...
Maybe same |
@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 Okay, I'll remove the info level rules. |
Opened an issue to discuss ways to cleanup the output of Any input welcome! @here |
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: @rpurdie mentioned that someone is working on patchwork again, I don't know who, maybe the linter could be integrated there as well. |
b9e816d
to
ed8d6ad
Compare
Hey @ALL, just wanted to check back in here. @priv-kweihmann and I worked a little to improve the code or One more proposition is in the pipeline in this issue 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. |
ed8d6ad
to
c76dad7
Compare
697ea63
to
8473061
Compare
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. |
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. |
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. |
@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. 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. |
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. |
We will never know if we don't event take the first step.
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?
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.
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 |
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.
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. |
Changelog: ========= Features --------- Add support for Python 3.11 (openembedded#466) (ff379e3) Allow representing enums with their unqualified symbolic names in headers (openembedded#465) (522b98e) Bug Fixes -------- Major refactoring of Polling, Retry and Timeout logic (openembedded#462) (434253d) Require google-auth >= 2.14.1 (openembedded#463) (7cc329f) Signed-off-by: Wang Mingyu <[email protected]> Signed-off-by: Khem Raj <[email protected]>
Changelog: ========= Features --------- Add support for Python 3.11 (openembedded#466) (ff379e3) Allow representing enums with their unqualified symbolic names in headers (openembedded#465) (522b98e) Bug Fixes -------- Major refactoring of Polling, Retry and Timeout logic (openembedded#462) (434253d) Require google-auth >= 2.14.1 (openembedded#463) (7cc329f) Signed-off-by: Wang Mingyu <[email protected]> Signed-off-by: Khem Raj <[email protected]>
Changelog: ========= Features --------- Add support for Python 3.11 (openembedded#466) (ff379e3) Allow representing enums with their unqualified symbolic names in headers (openembedded#465) (522b98e) Bug Fixes -------- Major refactoring of Polling, Retry and Timeout logic (openembedded#462) (434253d) Require google-auth >= 2.14.1 (openembedded#463) (7cc329f) Signed-off-by: Wang Mingyu <[email protected]> Signed-off-by: Khem Raj <[email protected]>
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