Drop Ruby as a build dependency #740

ffrank · 2024-02-27T10:12:08Z

We have been using Ruby for two things

a test script to find non-conforming YAML files (which has not worked in a long time and had been disabled)
the make yamlfmt target, which is a very hacky approach to having gofmt but for YAML, which is also an odd proposition, given that whitespace has semantic meaning in YAML

This change adds the Python based yamllint to our build dependencies. It has a much smaller footprint compared to bringing in Ruby, especially for folks who want to build mgmt on their regular work/play machines.

The test script now works and gives actionable error output.

The yamlfmt target essentially does the same thing now, though. No automatic "fixing" of YAML formats anymore, but then we probably also don't expect to write that much YAML any longer.

Linter settings were chose so that few changes are needed in existing YAML. It's another benefit of this approach that we can now curate our linter configuration.

ffrank · 2024-02-27T12:45:41Z

I have no idea why the file-mode.sh test suddenly fails in github CI. Works On My Machine.

frebib · 2024-02-27T19:43:37Z

Makefile

@@ -215,7 +215,7 @@ gofmt:
 find . -maxdepth 9 -type f -name '*.go' -not -path './old/*' -not -path './tmp/*' -not -path './vendor/*' -exec goimports -w {} \;

 yamlfmt:
- find . -maxdepth 3 -type f -name '*.yaml' -not -path './old/*' -not -path './tmp/*' -not -path './omv.yaml' -exec ruby -e "require 'yaml'; x=YAML.load_file('{}').to_yaml.each_line.map(&:rstrip).join(10.chr)+10.chr; File.open('{}', 'w').write x" \;
+ find . -maxdepth 3 -type f -name '*.yaml' -not -path './old/*' -not -path './tmp/*' -not -path './omv.yaml' | xargs yamllint


Apparently there's a tool called yamlfix that might satisfy this.
https://github.com/lyz-code/yamlfix

frebib · 2024-02-27T19:44:20Z

.github/FUNDING.yml

@@ -1,3 +1,4 @@
+---


We could disable the requirement for this in yamllint if we wanted

purpleidea · 2024-02-27T19:50:51Z

I have no idea why the file-mode.sh test suddenly fails in github CI. Works On My Machine.

Race bugs in testing env. I will disable it.

purpleidea · 2024-02-27T19:51:45Z

then we probably also don't expect to write that much YAML any longer.

Quite true. I expect a few incoming config file things, but in general it's not as much of an issue.

ffrank · 2024-02-29T20:24:06Z

In github CI I realized we have two more ruby users: mdl and fpm. fpm is not so painful because it doesn't need to be part of the build, and if we want to run it in CI, github actions can still just run ruby setup actions.

However, mdl is an entire different story. I tried substituting it with pymarkdownlnt, which has a limitation around tables (I can work around), but also has an annoying bug that occurs when indented blocks appear in enumerations or bullet point lists, and TABs are used for indentation. This begs the questions:

Why do we insist on tabs in markdown? It's not best practice and we explicitly disable the MD010 rule that would warn us about this. Linting life would be much easier without tabs. Would spaces be so bad here?

(This mostly happens in docs/language-guide.md.)

purpleidea · 2024-02-29T20:29:14Z

Why do we insist on tabs

I love my tabs. For many reasons unfortunately.

Accessibility
Not having to hammer my space bar to indent
Easy of use for when you want to see more or less indents

...

purpleidea · 2024-02-29T20:29:52Z

(And in theory we should be adding more markdown docs as time goes on, so we definitely want to keep the validator.)

ffrank · 2024-02-29T20:33:13Z

To illustrate the issue in the previous comment:

$ pymarkdownlnt scan docs/language-guide.md 

Unexpected Error(BadTokenizationError): An unhandled error occurred processing the document.

The issue is a failed assertion in this parser. I believe that it's caused by a bug, and I have been looking into it, but debugging someone else's parser/lexer code is...challenging.

purpleidea · 2024-02-29T20:51:03Z

While I love the idea of dropping the use of any ruby, it being there is not personally blocking me atm.

But if you're really keen on this issue, maybe this would help though: https://github.com/gomarkdown/markdown ?

ffrank · 2024-02-29T22:24:15Z

Oh I definitely want to be sure we keep linting the markdown. We should even work towards disabling fewer rules imo.

So yeah I filed jackdewinter/pymarkdown#1015 and will also look at the gomarkdown thingy.

ffrank · 2024-03-02T07:41:50Z

The tip of this now has a shape like I could imagine it. Let me know your thoughts please.

If you all find this an acceptable way forward, I will squash it down.

Note that I went for pipenv over just jamming pip packages into everyone's systems (except pipenv itself...apparently it's best practice to install it through pip). I hope this will be more sustainable (and not more fragile).

purpleidea · 2024-03-03T00:15:20Z

Wow, lots of neat work, thanks Felix!

I've just had a short read through as a diff of the whole thing, rather than each commit separately. There are clearly a bunch of great changes I can see already.

There are a few unknowns I'd need to dig into first. My first question is that if we're moving from ruby to python, is this a major improvement? It definitely seems to be from a packaging point of view. I'd like to rule out gomarkdown though first, at least for the mdl alternative.

Lastly, how did you want to squash? (I do not recommend squashing all of it into a single commit.) If there are a few stand-alone / easy commits to squash and/or merge early, maybe I can make this easier to review by merging those first?

I have a very large feature branch that I'll be merging shortly, and my goal is to merge this right after that. I don't think there are would be any major conflicts, but will do it in that order just to be safe.

Cheers!

ffrank · 2024-03-05T17:17:01Z

There are a few unknowns I'd need to dig into first. My first question is that if we're moving from ruby to python, is this a major improvement? It definitely seems to be from a packaging point of view. I'd like to rule out gomarkdown though first, at least for the mdl alternative.

Python is already a dependency, and no modern Linux box can function without python. It's a lot less noise on contributor's machines in this way. Ruby is rather obscure. Even Puppet users don't install it anymore, since Puppet vendors its own Ruby (go figure).

I will try replacing pymarkdown with gomarkdown before I proceed.

Lastly, how did you want to squash? (I do not recommend squashing all of it into a single commit.) If there are a few stand-alone / easy commits to squash and/or merge early, maybe I can make this easier to review by merging those first?

Mainly I want to get rid of some intermediary steps like ruby-yaml-fix -> yamllint -> yamlfix. Also if I go for gomarkdown, any mention of pymarkdown needs not be in our history.

But yes, otherwise I'm no fan of single-commit branches either. Some history can be helpful in archaeology down the line.

ffrank · 2024-03-05T18:39:17Z

So gomarkdown does not come with linting functionality. There is gomarkdown/mdfmt, but it's unmaintained and go build fails.

A slightly better maintained version is at https://github.com/shurcooL/markdownfmt/ and it does build, but it's not really customizable, and at a glance, it seems to do the opposite of what we want, e.g. joining our nice and carefully crafted 80 character line paragraphs into single long lines.

purpleidea · 2024-03-05T18:41:51Z

haha, okay! I'm okay with the python route for now if you think that's preferable. I'm a bit concerned about the open issue and wouldn't mind waiting until that's fixed before we swap it though.

Would you be able to rebase/squash your commits into a few separate ones based on logical functionality that you think makes sense and I'll review and merge those?

purpleidea · 2024-03-05T18:42:21Z

Or I could pick off the easy ones first if you'd like. LMK

ffrank · 2024-03-05T19:03:49Z

I will clean up and ping you.

ffrank · 2024-03-06T15:05:40Z

Okay, go.

ffrank · 2024-03-10T18:10:11Z

@purpleidea this is ready for review.

When error messages are written to stdout, they will be considered as output in case we want to fail from inside $( ) or backticks, and then the error does not end up on the terminal.

We pull in pipenv and install pip packages for yamlfix and pymarkdownlnt into the virtualenv. This allows us to drop mdl and later replace the ruby hack in Makefile that currently does our yamlfmt.

At the same time, introduce yamlfix for a proper "make yamlfmt" workflow that does not rely on the old ruby based hack. Also change existing YAML to reflect the result.

Add a fold in github actions output around the ragel build. Run the commit-message test locally, so that error can be detected before pushing to CI. We also now accept two-letter topics. Some minor improvement in the testing scripts.

Also now shows the title line of offensive commit message.

ffrank · 2024-04-01T20:08:09Z

Update, I'm in communication with @jackdewinter in jackdewinter/pymarkdown#1015 in order to solve the limitation around tab characters. Once the fix works, it will allow us to remove the workaround from test-markdownlint.sh.

But I feel that we can merge as is now, and remove the workaround later, so this needs not be blocked.

purpleidea · 2024-04-03T01:11:55Z

test/test-commit-message.sh

@@ -10,11 +10,12 @@ echo running "$0"
 ROOT=$(dirname "${BASH_SOURCE}")/..
 cd "${ROOT}" || exit 1

-commit_title_regex='^\([a-z0-9]\(\(, \)\|[a-z0-9]\)\+[a-z0-9]: \)\+[A-Z0-9][^:]\+[^:.]$'
+commit_title_regex='^\([a-z0-9]\(\(, \)\|[a-z0-9]\)*[a-z0-9]: \)\+[A-Z0-9][^:]\+[^:.]$'


What does this do?

It drops the random requirement that the "topic:" prefix has at least three characters in the topic.

purpleidea · 2024-04-03T01:13:11Z

test/test-commit-message.sh

@@ -183,5 +184,13 @@ then
 test_commit_message_common_bugs $commit
 done
 fi
+else # assume local branch


purpleidea · 2024-04-03T01:20:18Z

test/test-commit-message.sh

@@ -155,42 +157,30 @@ test_commit_message_body() {
 if [[ -n "$TRAVIS_PULL_REQUEST_SHA" ]]
 then
 commits=$(git log --format=%H origin/${TRAVIS_BRANCH}..${TRAVIS_PULL_REQUEST_SHA})
- [[ -n "$commits" ]]


Why are we nuking everything from here to the end?

We just avoid copy/paste by doing the for commit in $commits work at the bottom of the script.

purpleidea · 2024-04-03T01:20:28Z

test/test-commit-message.sh

 then
 echo "FAIL: Commit message should match the following regex: '$commit_title_regex'"
+ echo "('$title' does not match)"


purpleidea · 2024-04-03T01:33:11Z

I've cherry-picked a few of these commits. I think we should hold off on the markdown/yaml thing for the moment. I need to have a deeper look into it all. Thanks for the fun commits!

ffrank force-pushed the noruby branch from 5dd0522 to 468cceb Compare February 27, 2024 12:13

ffrank force-pushed the noruby branch from eb1b3f9 to f4bfca5 Compare February 27, 2024 12:49

frebib approved these changes Feb 27, 2024

View reviewed changes

ffrank force-pushed the noruby branch 2 times, most recently from 9034c4c to 8570028 Compare March 1, 2024 23:44

ffrank marked this pull request as draft March 5, 2024 17:17

ffrank force-pushed the noruby branch from 8570028 to a885fa0 Compare March 6, 2024 14:18

ffrank marked this pull request as ready for review March 6, 2024 15:05

ffrank added 4 commits March 23, 2024 19:40

test: Send error messages to stderr, where they belong

5ba2df8

When error messages are written to stdout, they will be considered as output in case we want to fail from inside $( ) or backticks, and then the error does not end up on the terminal.

test: markdownlint: Replace mdl with pymarkdownlnt

6191bf6

We pull in pipenv and install pip packages for yamlfix and pymarkdownlnt into the virtualenv. This allows us to drop mdl and later replace the ruby hack in Makefile that currently does our yamlfmt.

test: Use yamllint over ruby for testing YAML formatting

7f673f3

At the same time, introduce yamlfix for a proper "make yamlfmt" workflow that does not rely on the old ruby based hack. Also change existing YAML to reflect the result.

scripts: Some quality of life improvements

a6e5ad6

Add a fold in github actions output around the ragel build. Run the commit-message test locally, so that error can be detected before pushing to CI. We also now accept two-letter topics. Some minor improvement in the testing scripts.

ffrank added 2 commits March 23, 2024 19:40

test: commitmessage: Simplify, actually run without PR

da4ff16

Also now shows the title line of offensive commit message.

test: mardownlint: WiP removal of the workaround of leading tasbs issues

837014a

ffrank mentioned this pull request Apr 1, 2024

Improve commit message test #751

Open

purpleidea reviewed Apr 3, 2024

View reviewed changes

ffrank force-pushed the noruby branch from a885fa0 to 837014a Compare May 10, 2024 21:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drop Ruby as a build dependency #740

Drop Ruby as a build dependency #740

ffrank commented Feb 27, 2024

ffrank commented Feb 27, 2024

frebib Feb 27, 2024

frebib Feb 27, 2024

purpleidea commented Feb 27, 2024

purpleidea commented Feb 27, 2024

ffrank commented Feb 29, 2024

purpleidea commented Feb 29, 2024

purpleidea commented Feb 29, 2024

ffrank commented Feb 29, 2024

purpleidea commented Feb 29, 2024

ffrank commented Feb 29, 2024

ffrank commented Mar 2, 2024 •

edited

purpleidea commented Mar 3, 2024

ffrank commented Mar 5, 2024

ffrank commented Mar 5, 2024

purpleidea commented Mar 5, 2024

purpleidea commented Mar 5, 2024

ffrank commented Mar 5, 2024

ffrank commented Mar 6, 2024

ffrank commented Mar 10, 2024

ffrank commented Apr 1, 2024 •

edited

purpleidea Apr 3, 2024

ffrank Apr 3, 2024

purpleidea Apr 3, 2024

purpleidea Apr 3, 2024

ffrank Apr 3, 2024

purpleidea Apr 3, 2024

purpleidea commented Apr 3, 2024

Drop Ruby as a build dependency #740

Are you sure you want to change the base?

Drop Ruby as a build dependency #740

Conversation

ffrank commented Feb 27, 2024

ffrank commented Feb 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

purpleidea commented Feb 27, 2024

purpleidea commented Feb 27, 2024

ffrank commented Feb 29, 2024

purpleidea commented Feb 29, 2024

purpleidea commented Feb 29, 2024

ffrank commented Feb 29, 2024

purpleidea commented Feb 29, 2024

ffrank commented Feb 29, 2024

ffrank commented Mar 2, 2024 • edited

purpleidea commented Mar 3, 2024

ffrank commented Mar 5, 2024

ffrank commented Mar 5, 2024

purpleidea commented Mar 5, 2024

purpleidea commented Mar 5, 2024

ffrank commented Mar 5, 2024

ffrank commented Mar 6, 2024

ffrank commented Mar 10, 2024

ffrank commented Apr 1, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

purpleidea commented Apr 3, 2024

ffrank commented Mar 2, 2024 •

edited

ffrank commented Apr 1, 2024 •

edited