Add markdown action - #19010
Conversation
|
nosborn/github-action-markdown-cli@508d6ce and tcort/github-action-markdown-link-check@a800ad5 are not allowed to be used in openhab/openhab-addons. Actions in this workflow must be: within a repository owned by openhab, created by GitHub, or matching the following: Ana06/get-changed-files@, ghys/checkstyle-github-action@, stCarolas/setup-maven@*. Who can allow this? |
|
Add-ons maintainers should be able to configure GH repo settings. There you can add exceptions. |
|
Yes, it is. :-) |
|
What am I missing? We fixed the 1800+ markdown issues. Re running the tools shows no progress. Looks like it gets old source code?! |
|
There's one more to merge #19028 |
|
Updated the PR, now it is down to 250. Yes one PR to go. I also wonder what happend to freebox, as that binding shows errors while being removed. |
|
Only freeboxos is left. |
|
Does it need to be rebased? |
Freebox and freeboxos are separate bindings. The first is removed and now no longer (after merge to current head) shows no more warnings. I think somehow missed freeboxos. |
Related to openhab#19010 Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
Thanks for all your efforts on this! 👍 I have created #19062 covering FreeboxOS. Now enjoy your holiday. 😎 |
|
Yes you've certainly earned a nice holiday after fixing all these Markdown issues! 😉 |
wborn
left a comment
wborn
left a comment
There was a problem hiding this comment.
Some docs on how to run this locally would also be nice so contributors can more quickly get feedback on their changes and do not have to wait for CI.
Would be nice if it becomes part of the Spotless config but it doesn't seem to support markdownlint.
Another possibility for being able to easily run it locally would be by using the frontend-maven-plugin to run markdownlint.
Maybe it can become part of the CI workflow so we have only have one badge to check for the overal status?
|
There is room for improvement. I have not found a way to run it locally without additional setup steps. Edit: when the frontend-maven-plugin is added to the root pom, will the check be run by all mvn builds even when someone only has the binding subfolder as project opened? If so this would be very useful. Nevertheless this ci action is still needed as gatekeeper when I’m back I will add some lines to the docs (in a separate PR) |
* Fix Markdown Related to #19010 Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
|
All green, ready to merge |
* Fix Markdown Related to openhab#19010 Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
|
Ping @openhab/add-ons-maintainers i prefer not to wait as new markdown issues will then probably be merged. |
|
@wborn - can you have a look? Unfortunately my knowledge here is limited, so I would only be able to blindly approve and merge it and see what happens. |
|
If you address the review comments, I will help fixing any new issuess and test it locally based on the docs. |
* Fix Markdown Related to openhab#19010 Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk> Signed-off-by: Paul Smedley <paul@smedley.id.au>
|
Made some progress. I added markdownlint to the mvn verify step. It is very challenging as the build proces can be started from two places: the project root or a bundle. I managed to fix these working directory issues except for the last step: loading the markdown config file. Any ideas? |
|
Maybe you can use Line 490 in ce138b5 |
|
Thanks, that was exactly where i was looking for. I have now changed the paths and it is more clean now. Errors are interactive/clickable and point to the problematic file straight away. Nice! Tried to run it from a few different bundles, all works as expected. I have removed the workflow as the markdown check is now part of the verify fase, so it should also be run from CI. I have a full build running now to see if that gives any findings. If not i think this is ready to get merged and/or get aditional feedback. Edit: locally the full build is succesfull it detects some markup issues that i will fix in another PR. Edit: Is there some conflict with jsscripting? As the CI fails due to this error: |
|
This also broke the JS Scripting build and possibly Matter as well when |
Regression from openhab#19010. Signed-off-by: Florian Hotze <dev@florianhotze.com>
Related to openhab#19010 Signed-off-by: Wouter Born <github@maindrain.net>
Signed-off-by: Leo Siepel <leosiepel@gmail.com>
Regression from #19010. Signed-off-by: Florian Hotze <dev@florianhotze.com>
Signed-off-by: Leo Siepel <leosiepel@gmail.com>
Regression from openhab#19010. Signed-off-by: Florian Hotze <dev@florianhotze.com>
|
This causes endless build failures in Eclipse, not to mention slowdown. If the goal is to enable something for GitHub actions, why is it added to the core Maven build script? That script is run every time the JAR itself is built, which seems to have nothing to do with the markdown action. I'm asking because I don't know what the goal is, but I can't suggest a solution until I know. It's very problematic as it is now, it grinds my development work almost to a halt. I just got 174 build errors because of this... the |
|
After reading some more in this PR, it seems like this was considered a "feature", not a bug. But it is a bug IMO, because it means that this will have to run hundreds of times over and over again during normal work that don't include any modifications to the markdown documents at all. It also requires node to be configured and working. It would be much better to include it in a Maven profile, so that it doesn't run unless the profile is activated using |
If something like that can work for all parties, that would be excellent because I'm still tormented by #20538. |
Fundamentally, you have to decide if it should be opt-in or opt-out. I think all the problems related to node (npm Windows is a whole chapter in itself), the plugin used, and the fact that this often fails and takes a lot of time when done across hundreds on bundles, makes it quite obvious that it should be an opt-in. Add to that, that the build process in Eclipse is fully automated, so you can't enable "skip options". It will run the default build, so you better make sure that the default build is just that - the build, nothing else. Eclipse automatically skips the |
I haven’t seen this behavior in VS Code on Windows. Could you elaborate on both issues? I also haven’t seen other reports, so I’m wondering if there’s something specific about your setup. The check was intended to run only once, whether you run the whole repo or the bundle. The cost should be small, while the review effort is reduced. If your experience is different, I hope we can improve that. It took quite a lot of time to get it this far, and I’m currently reluctant to make it opt-in because of the review benefits and the value of spotting issues early. |
You can't compare VS Code with Eclipse. VS Code is a text editor on steroids (via a million plugins), Eclipse is a fully fledged IDE that integrates extremely well with Java and manages the whole thing for you. This has huge advantages, but it requires that the project is set up "sensibly". In Eclipse, you can find references, implementations, super classes, all uses of a variable etc. with ease (not stupid text-matches like VS Code does, it actually "understands" the code and know about types, classes, inheritance etc.). You can easily do refactorings, rename across projects having every reference updated, you can move packages - in short it's fully integrated. In addition, it builds automatically for you, have a debugger read for use at any time, and it even allows you to modify code when OH is running, where it only builds and "hot replaces" the bytecode that has been changed in memory, making the effect "immediate". To do all these things, Eclipse must be tightly integrated in the build process itself. But, it requires that projects "follow the rules" to retain this ability, and don't e.g. attach all kind of unrelated tasks to the build. That will obviously cause issues with this whole system. My previous experience with Eclipse is "fantastic", but I've never used BndTools before, and a BndTools plugin is required to integrate the OSGi setup in OH with Eclipse (Eclipse in itself supports/provides the OSGi framework via Equinox, the reference OSGi implementation, Bnd is used to "configure" OSGi (what bundles to run, their relationship they have etc.). Karaf, being a completely different product by Apache, has made a number of "extensions" to OSGi that are Karaf-only, so they don't do anything for Equinox, which is why Bnd is used for everything "outside" Karaf, like running itests, running OH in Eclipse, etc. The BndTools plugin for Eclipse has a lot of issues, and makes the whole OH/Eclipse integration slow and "challenging", which I suspect is why so many have chosen other solutions. But, this is very far from my experience with Eclipse without the BndTools plugin, so I blame that, not Eclipse. Anyway, once you figure out how to live with BndTools, you still have most of the "Eclipse advantages" remaining, just soured by the BndTools tantrums from time to time. I don't know how you "intended it only to run once", but when you put it in a super POM of all the add-ons, it will apply to each and every add-on. Which means that when Eclipse manages the whole application, it will be triggered again and again and again. Add to that, as I said, that npm isn't even a system that should be required to build OH, and that I'm sure many would be contributors have no idea how to configure and get working, and you have a great reason not to do this in the first place. In addition, if I recall correctly, the "frontend plugin" doesn't support incremental builds, so it will run the whole process every time, regardless of what has changed, and that it often fails. I ususally run Maven in "offline mode" to avoid the constant checking for new artifacts, which is slow and not desirable anyway, I want to run what I have locally, and the plugin seems completely incapable of handling that, and will hang for a while before timing out and causing a build error. It's also a complete waste of time, it probably cost me 30 minutes just to wait for the 174 failures it caused this afternoon. So, the cost isn't small, it's not even remotely small. It will run, in full, for every add-on for the tiniest change, like a changed timestamp on a file (git checkout, rebasing etc), since it doesn't do differential/smart builds.
I'm not talking about making it opt-in on the CI - but why should we be forced to run this a million times locally when we do completely unrelated things? I can't see how that can be a benefit at all, and frankly, it has made add-ons almost unworkable for me. This has long annoyed me, but I didn't know what caused it. Today, when it completely sabotaged everything, I took the trouble to figure out where it came from, and that's why I haven't said anything before. The annoyance has been there since it was introduced. |
|
Just to add an example of a „split“ behaviour: Wrt to Node/npm: frontend-maven-plugin downloads it for you, it doesn’t use your existing or non-existing local node install. |
That probably explains why it fails in offline mode then - and why it's so slow - it probably downloads that once for every add-on, because the file structure isn't what it expects. |
|
@lsiepel Don't you have anything more to say? Moving the current plugin configuration into a profile is very easy, and so is modifying the CI runner to activate the profile. So, I kind of seems that your objection is that you don't want to prevent it from running on local builds? |
|
Time constraints.. :-/ Initially i had a seperate github action that did the markup checks. I was asked by @wborn to integrated it in the build proces, it took me a million hours todo so and i would be a bit frustrated to have it reverted. But if reverting prevents continous frustration by others, it might be better. One of the posts above is explaining how excellent eclipse is, i would never question ones IDE preferences. I can only say that when i use vscode i have about all the features you mention available and i do not experience the issues you mention. I'm not familiair with the way eclipse builds, but it should not run the markdownlint check for each bundle. With the way inheritance is used, it should run the markup check only once per build, no matter if you run the build process from the root, the bundles or the binding folder. And yes it is important to have it run locally before a PR is created. Just like SAT. I'm open to improvements. |
I haven't suggested reverting anything, just moving it into a profile so that it needs an extra parameter to run. It's a minimal change, but the result is that it won't be run by default. I can try to whip up a suggestion as to how it might be done, and you can see what I mean. I use VS Code too, for other languages, because Eclipse is really only good with Java - and relatively helpless with everything else. And I can promise you that the integration in VS Code can't be compared. But, if you're happy with it, that's fine. My point is that this tight integration has a "cost" in that things must be done in such a way that the automatic handling works. I can say that I wouldn't be able to do the things I do with anything close to the speed and precision I do without Eclipse's close integration. Others have their way of working, but to me, it's very hard to understand why people would want to not use Eclipse for Java development. My point is that we shouldn't make it "impossible" to use Eclipse with OH, because it is a very capable tool.
I don't understand how you think this should work. Eclipse builds each bundle/project individually. It doesn't run "build all", if only builds what has changed. Maven itself, or its standard plugins like the compiler plugin, supports only recompiling what has changed (if you don't use "clean", which Eclipse does not do unless you explicitly asks for it). It has some way of knowing which source files have changed and what needs to be recompiled, I don't know the details of how this works, but I know that's what it does. So, asking to build a bundle that hasn't changed is very, very cheap. But, the "frontend" plugin doesn't support this, so it will do the full task every time a bundle is built. If it supported differential builds, so that it only ran if one of the markdown files had changed since it was last run, it would "fit" into this system. In addition, it seems like the plugin doesn't respect the "offline" setting in Maven, so that it tries to download npm once every time a bundle is build, and when in "offline" mode, it looks to me like it waits for a timeout, and then fails. I run Maven "offline" as standard. The reason for this lies in Maven itself, and they way it handles snapshots. Non snapshot versions aren't a problem, it won't try to "refresh" those if they are already cached, because they are assumed not to change. But, snapshot versions change. So, they must be "refreshed". Maven does so when it feels that enough time has elapsed since the last time it tried for that artifact/bundle. I think it's once every 24 hours by standard. When you have hundreds of bundles open, it means that very often some of them are close to this "limit". The next time you build something that depends on a snapshot artifact that Maven considers "expired", it will contact the online repo and compare the timestamps with that of the local cached version. If the online artifact has a more recent snapshot, it will replace the local artifact with a downloaded one. The problem is that when you're working on a branch, your version will typically be the same as "the latest" snapshots online, e.g. 5.2.0-SNAPSHOT these days. But, their content isn't the same - because you might have made local changes needed for the branch you're working on. When Maven "refreshes" some of the bundles with those found online, it will often download versions that are incompatible with what you're working with. This will break everything, and you get all kind of compilation errors because this and that doesn't exist, a method signature has changed, etc. The only way to "fix" this is to build those bundles locally, so that they overwrite what it just downloaded with a newer timestamp. But, you don't always know what bundles were downloaded, and this keeps happening in the background while you work because Eclipse runs the Maven builds automatically. It's quite a hopeless and very frustrating situation when things just keep breaking "by itself", and if you don't know what bundles was replaced, the "fix" is to build core in its entirety. Until next time it happens, with can be at any time. Rinse and repeat. From the Maven command line, you have the So, to me, this is a nightmare, and the only way I'm able to work with add-ons now is to keep reverting this PR locally every time I check something out. I must then exclude the reversal every time I commit, so that it won't be a part of the commit. And, I must revert the reversal before I can switch branch or rebase, or Git thinks I have a "dirty work tree". It's not exactly convenient, and makes me not want to work with add-ons at all.
I usually don't run SAT locally. Sure, it happens that I must run spotless after pushing because I forgot that I changed something that spotless doesn't like. But, I'd much rather do that from time to time, than waste the time to run SAT locally all the time. But, the big, essential difference is that SAT has been set up so that it doesn't run. I don't remember exactly how that is done at this time, but I know that it doesn't run, or spotless would have failed the build constantly when you work. Not adhering to spotless doesn't break Eclipse builds. Perhaps it's possible to do something similar with this, but I thought that the whole plugin had been excluded from running during Eclipse builds (although I can't find any such configuration right now). With the |
|
It took some digging, but I finally found out how the SAT plugin it excluded from the Eclipse build: It's bound to the Is there a particular reason why |
|
There is something strange here, even if I move it to 
I found the reason the plugin insists on running on incremental builds, the plugin author has specifically configured it to do so: The plugin comes with Eclipse configuration that "makes sure" that it runs with Eclipse builds... 😞 |
|
Setting it to It's obvious that the |
* Fix Markdown Related to openhab#19010 Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
Related to openhab#19010 Signed-off-by: Wouter Born <github@maindrain.net>
Signed-off-by: Leo Siepel <leosiepel@gmail.com>
Regression from openhab#19010. Signed-off-by: Florian Hotze <dev@florianhotze.com>
Related to openhab#19010 Signed-off-by: Wouter Born <github@maindrain.net> Signed-off-by: olemr <olemr@olemr.com>
Signed-off-by: Leo Siepel <leosiepel@gmail.com> Signed-off-by: olemr <olemr@olemr.com>
Regression from openhab#19010. Signed-off-by: Florian Hotze <dev@florianhotze.com> Signed-off-by: olemr <olemr@olemr.com>
Changes:
acton Windows to (closely) mimic GHA locallyPlease provide feedback.
Fixes: #13858
Refs: #19011