Enhancement: Create GitHub CI/CD workflow and/or client side commit hooks

[SirWumpus]

Each push to GitHub should run the Makefile test target (to ensure we only ever push working code) as part of a CI workflow.

Unclear if a CD workflow should be added (never done one) to regenerate manifests, tarballs, snowballs, etc.

[SirWumpus]

Anyway CI testing is only as good as the test suite and its output, which appears you guys have gone to great length to be... verbose. :)

[xexyl]

Anyway CI testing is only as good as the test suite and its output

Of course.

, which appears you guys have gone to great length to be... verbose. :)

Yep .. which is good. It took some time to get it this way but it's pretty extensive and extremely helpful: as is your work! Thanks again!

[SirWumpus]

My work, iocccsize, is a drop in the bucket to what mkiocccentry will do. I don't think I've contribute much beyond some bugs and food for thought.

[xexyl]

My work, iocccsize, is a drop in the bucket to what mkiocccentry will do. I don't think I've contribute much beyond some bugs and food for thought.

Even so it's still a significant importance to the contest, iocccsize!

A minor nitpick on the workflow file. Is there a reason we have C/C++? Wouldn't C suffice? Okay it's not a big deal but just wondering. (Let's just say the name C++ makes me feel a bit off, to make the obvious pun :-) )

Anyway it looks pretty straightforward and when the jparse repo is created I can (it seems) fairly easily install it - perhaps with some modifications.

[xexyl]

Actually, @lcn2, since we're working on this and it does some cloning might it be an idea to test jparse in its own repo? I'm not suggesting this one way or another btw: just that since we're working on this type of thing it might be nice to verify it works okay there too.

Let me know and if so we can coordinate it together. I'm hoping to get back to the spoiler issue soon and then maybe I can reply to the FAQ reorganising thought I had .. though I can tell I'll need a break soon.

[xexyl]

And a question for you, @SirWumpus:

I see in the test.yml:

    steps:
    - uses: actions/checkout@v4

Is the @v4 significant for some reason? Or the entire line even - what does it mean? The other line seem to just be telling it what to do but not sure about this line.

Thanks.

[xexyl]

One more thing (for now). Should the workflow also install the checknr tool? make check_man is run by make prep and it seems to work but is it wise to install clone the tool and install it first, just in case?

It might be: the reason is I added code to exit with proper exit code depending on result of the command. The original one did not. If I am thinking right, it might be something like:

diff --git i/.github/workflows/test.yml w/.github/workflows/test.yml
index 10c0744433cae92643a894cc05e28d134510b7ef..dcdef36b002e53077c1cf35f503228ce06ba97e8 100644
--- i/.github/workflows/test.yml
+++ w/.github/workflows/test.yml
@@ -21,10 +21,14 @@ jobs:
       run: git clone https://github.com/lcn2/seqcexit
     - name: install seqcexit
       run: cd seqcexit && make install
     - name: clone picky
       run: git clone https://github.com/xexyl/picky
+    - name: clone checknr
+      run: git clone https://github.com/lcn2/checknr.git
+    - name: install checknr
+      run: cd checknr && make install
     - name: install picky
       run: cd picky && make && cp -p picky /usr/local/bin
     - name: make
       run: make
     - name: make slow_prep

Do you want me to do that, Landon? Or do you think it's a problem, Anthony?

[SirWumpus]

The C/C++ CI is just the default name taken from the suggested GitHub template. It can be changed in test.yml at the very top. Maybe replace it with All Done!!! All Done!!! -- Jessica Noll, age 2

Similarly

    steps:
    - uses: actions/checkout@v4

Came from the same template I started from. I assume the @4 is a version or tag Not sure what actions/checkout@v4 provides (I've not studied that part). I suspect it loads tools like git or checks out the repo to test instance. Right now its automagic.

[xexyl]

The C/C++ CI is just the default name taken from the suggested GitHub template. It can be changed in test.yml at the very top. Maybe replace it with All Done!!! All Done!!! -- Jessica Noll, age 2

:-) .. though I noticed that GitHub is very picky with yaml. If you think the above change I proposed is wise I'll do the change at the same time (removing the C++ part. But I do have a fun change on the subject of that quote ... for another commit. Pondering it: might be good to have.

Similarly

    steps:
    - uses: actions/checkout@v4

Came from the same template I started from. I assume the @4 is a version or tag Not sure what actions/checkout@v4 provides (I've not studied that part). I suspect it loads tools like git or checks out the repo to test instance. Right now its automagic.

The version or tag was my thinking too .. which is why I was wondering.

[xexyl]

Note for Landon: once you have removed the forced error in the Makefile I have a fun thing for the Makefile I think you'll enjoy. I don't want to commit now in case it causes a conflict. But I don't want to cause any git conflict so I'll hold off. I did just add it to my file but I can stash it and then stash pop and remove the changed line.

[lcn2]

Speaking of such, Landon, did you say this should be done after the fork merge?

We recommend that, yes.

Actually, @lcn2, since we're working on this and it does some cloning might it be an idea to test jparse in its own repo?

We suggest waiting as this would be a distraction from getting IOCCC28 started sooner.

[lcn2]

And a question for you, @SirWumpus:

I see in the test.yml:

    steps:

    - uses: actions/checkout@v4

Is the @v4 significant for some reason? Or the entire line even - what does it mean? The other line seem to just be telling it what to do but not sure about this line.

Thanks.

The use of v3 generated warnings about code that was going to become obsolete.

[lcn2]

One more thing (for now). Should the workflow also install the checknr tool? make check_man is run by make prep and it seems to work but is it wise to install clone the tool and install it first, just in case?

It might be: the reason is I added code to exit with proper exit code depending on result of the command. The original one did not. If I am thinking right, it might be something like:

diff --git i/.github/workflows/test.yml w/.github/workflows/test.yml

index 10c0744433cae92643a894cc05e28d134510b7ef..dcdef36b002e53077c1cf35f503228ce06ba97e8 100644

--- i/.github/workflows/test.yml

+++ w/.github/workflows/test.yml

@@ -21,10 +21,14 @@ jobs:

       run: git clone https://github.com/lcn2/seqcexit

     - name: install seqcexit

       run: cd seqcexit && make install

     - name: clone picky

       run: git clone https://github.com/xexyl/picky

+    - name: clone checknr

+      run: git clone https://github.com/lcn2/checknr.git

+    - name: install checknr

+      run: cd checknr && make install

     - name: install picky

       run: cd picky && make && cp -p picky /usr/local/bin

     - name: make

       run: make

     - name: make slow_prep

Do you want me to do that, Landon? Or do you think it's a problem, Anthony?

Yes please 🙏

[xexyl]

Speaking of such, Landon, did you say this should be done after the fork merge?

We recommend that, yes.

Actually, @lcn2, since we're working on this and it does some cloning might it be an idea to test jparse in its own repo?

We suggest waiting as this would be a distraction from getting IOCCC28 started sooner.

I understand and that very well could be the case indeed. Just wanted to say in case.

[xexyl]

And a question for you, @SirWumpus: I see in the test.yml:

    steps:

    - uses: actions/checkout@v4

Is the @v4 significant for some reason? Or the entire line even - what does it mean? The other line seem to just be telling it what to do but not sure about this line. Thanks.

The use of v3 generated warnings about code that was going to become obsolete.

Ah I see.

[xexyl]

One more thing (for now). Should the workflow also install the checknr tool? make check_man is run by make prep and it seems to work but is it wise to install clone the tool and install it first, just in case? It might be: the reason is I added code to exit with proper exit code depending on result of the command. The original one did not. If I am thinking right, it might be something like:

diff --git i/.github/workflows/test.yml w/.github/workflows/test.yml

index 10c0744433cae92643a894cc05e28d134510b7ef..dcdef36b002e53077c1cf35f503228ce06ba97e8 100644

--- i/.github/workflows/test.yml

+++ w/.github/workflows/test.yml

@@ -21,10 +21,14 @@ jobs:

       run: git clone https://github.com/lcn2/seqcexit

     - name: install seqcexit

       run: cd seqcexit && make install

     - name: clone picky

       run: git clone https://github.com/xexyl/picky

+    - name: clone checknr

+      run: git clone https://github.com/lcn2/checknr.git

+    - name: install checknr

+      run: cd checknr && make install

     - name: install picky

       run: cd picky && make && cp -p picky /usr/local/bin

     - name: make

       run: make

     - name: make slow_prep

Do you want me to do that, Landon? Or do you think it's a problem, Anthony?

Yes please 🙏

Will do now and then I must go afk a while.

[lcn2]

SUMMARY and QUESTION

So the net effect of test.yml is an advisory (not mandatory) check on the result of a commit?

That is, a commit such as the experiment 🧪 where we deliberately introduction of a failure (see commit 73a965d0e8d42b6f20a727524cdd65d417f1ab07) will go through into the given branch, but now it will be flagged as a violation.

BTW: We intend to remove that line in make test once this issue is ready to be closed.

We are not criticizing, please understanding, just trying to understand the situation. We do see the value in flagging a commit as problematic.

Or, did we miss something important in all of the discussion above. If so, @SirWumpus, please 🙏 advise and correct.

[SirWumpus]

So the net effect of test.yml is an advisory (not mandatory) check on the result of a commit?

That is probably the best way to describe in one sentence what I tried to do in several comments.

[xexyl]

So the net effect of test.yml is an advisory (not mandatory) check on the result of a commit?

That is probably the best way to describe in one sentence what I tried to do in several comments.

And if it is advisory and goes through then that's also good because it will help find problems faster but it won't be an impedance on people contributing. I mean it might be that they are asked to fix the problem but it won't be a blocker which can be very frustrating.

But since it's noticed right away it can be fixed sooner. I like that idea.

[lcn2]

With commit 07681d2292fc933de442ceb0fa3b25ee63cf7b56 we complete our test.

As a reminder, commit 73a965d0e8d42b6f20a727524cdd65d417f1ab07 was an deliberate introduction of an error to test the effect of the new test.yml file.

The net effect of test.yml is an advisory (not mandatory) check on the result of a commit.

As a result of these successful tests, we now revert the deliberate error and resume normal testing operations.

Based on @SirWumpus comment-2209385393 we believe we can successfully close this issue with our thanks to @SirWumpus 👍

[xexyl]

With commit 07681d2 we complete our test.

As a reminder, commit 73a965d was an deliberate introduction of an error to test the effect of the new test.yml file.

The net effect of test.yml is an advisory (not mandatory) check on the result of a commit.

As a result of these successful tests, we now revert the deliberate error and resume normal testing operations.

Based on @SirWumpus comment-2209385393 we believe we can successfully close this issue with our thanks to @SirWumpus 👍

.. and with my thanks too!

[xexyl]

Hmm ... I did a pull to sync to this repo, on my fork, and I got an error. Not sure if that's because it was undoing it or if it will be a problem for every commit. I have a fun commit to do and we'll see if it happens.

... but:

https://github.com/xexyl/mkiocccentry/actions/runs/9798737001

and

https://github.com/xexyl/mkiocccentry/actions/runs/9798768017

I will try the fun thing now and hopefully it works okay.

[xexyl]

Just did a commit and I got this:

https://github.com/xexyl/mkiocccentry/actions/runs/9798790413.

Unsure if two issues will happen but maybe this is a problem? What do you think, @SirWumpus ?

[SirWumpus]

Did you check the CI flow failure? There is an issue with install checknr...

https://github.com/xexyl/mkiocccentry/actions/runs/9798737001/job/27057762199

[xexyl]

Did you check the CI flow failure? There is an issue with install checknr...

https://github.com/xexyl/mkiocccentry/actions/runs/9798737001/job/27057762199

I wondered about that but I didn't see anything there. I'll take a look.

[xexyl]

Ah ... maybe there was a typo in the Makefile? That's what it looks like. I'll do a pull request if that's the case.

[SirWumpus]

Read the error which pertains to strncpy its seems...

Run cd checknr && make install
cc -std=gnu11 -O3 -g3 -pedantic -Wall -Wextra -Wno-unused-const-variable  checknr.c -o checknr
In file included from /usr/include/string.h:535,
                 from checknr.c:59:
In function ‘strncpy’,
    inlined from ‘process’ at checknr.c:301:4:
/usr/include/x86_64-linux-gnu/bits/string_fortified.h:95:10: warning: ‘__builtin_strncpy’ output may be truncated copying 4 bytes from a string of length 8192 [-Wstringop-truncation]
   95 |   return __builtin___strncpy_chk (__dest, __src, __len,
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   96 |                                   __glibc_objsize (__dest));
      |                                   ~~~~~~~~~~~~~~~~~~~~~~~~~
install -v -d -m 0775 /usr/local/bin
install: cannot change permissions of ‘/usr/local/bin’: Operation not permitted
make: *** [Makefile:263: install] Error 1
Error: Process completed with exit code 2.

[xexyl]

Read the error which pertains to strncpy its seems...

Run cd checknr && make install
cc -std=gnu11 -O3 -g3 -pedantic -Wall -Wextra -Wno-unused-const-variable  checknr.c -o checknr
In file included from /usr/include/string.h:535,
                 from checknr.c:59:
In function ‘strncpy’,
    inlined from ‘process’ at checknr.c:301:4:
/usr/include/x86_64-linux-gnu/bits/string_fortified.h:95:10: warning: ‘__builtin_strncpy’ output may be truncated copying 4 bytes from a string of length 8192 [-Wstringop-truncation]
   95 |   return __builtin___strncpy_chk (__dest, __src, __len,
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   96 |                                   __glibc_objsize (__dest));
      |                                   ~~~~~~~~~~~~~~~~~~~~~~~~~
install -v -d -m 0775 /usr/local/bin
install: cannot change permissions of ‘/usr/local/bin’: Operation not permitted
make: *** [Makefile:263: install] Error 1
Error: Process completed with exit code 2.

No. That's just a warning. The error is the trying to create the /usr/local/bin. But that's necessary for install if the directory does not exist. I have in the past wondered if we should even use check_man in the test rules.

What do you think, Landon? The checknr Makefile needs that line but for GitHub it does not work okay.

[SirWumpus]

Actually you have two issues (1 warning, 1 error). The CI actions run as a normal user so you need to change...

cd checknr && make install

to

cd checknr && sudo make install

[xexyl]

Actually you have two issues (1 warning, 1 error). The CI actions run as a normal user so you need to change... cd checknr && make install to

cd checknr && sudo make install

Correct. But does it work with sudo in GitHub?

[SirWumpus]

Yes. See earlier in test.tml where I install universal-ctags.

[xexyl]

Another option is to just use what is there in the system and not worry about it failing. One of us will eventually find it when we run make prep manually.

But if sudo will work in the workflow that can be done.

[xexyl]

Yes. See earlier in test.tml where I install universal-ctags.

I missed that at that hour ... thanks! I'll fix.

[xexyl]

Oh I know why. It's because I yanked it from the line for picky. But that doesn't use sudo. Why?

For seqcexit tool (which is obviously what I yanked, not picky).

[xexyl]

Just did a commit. Hopefully that solves it. Added sudo to the others that were missing it.

[SirWumpus]

Well I'm not sure as to why.

It could in checknr its trying to assert that /usr/local/bin is present or not and create it. Its probably an oddity of install(1) that it be run as root. Just guessing. I never looked deeply into the each makefile unless there was an error.

[xexyl]

Got the no jobs run one .. hmm. I'll see if the other one happens too.

[xexyl]

Well I'm not sure as to why.

It could in checknr its trying to assert that /usr/local/bin is present or not and create it. Its probably an oddity of install(1) that it be run as root. Just guessing. I never looked deeply into the each makefile unless there was an error.

That makes sense yes. Wonder why it's happening. I tried a commit and so far no error (but maybe it's not done yet) but the no jobs run did happen: https://github.com/xexyl/mkiocccentry/actions/runs/9798917192.

Okay and that's considered an error too. Hmm ...

[xexyl]

It seems like the other file, dependabot.yml, has a problem. The other issue did not show up this time.

No jobs are running now it would appear so the sudo must have fixed that problem - or I would think so - but I don't see it as a job that ran either.

I added the other file to a repo and it failed too so maybe it does have an issue but what it is I don't know. Hopefully the sudo solved the other problem though.

[SirWumpus]

I'm getting an error in the Ci again where there was not one before...

0s
Run make slow_prep
bash: -c: line 14: syntax error: unexpected end of file
make: *** [Makefile:581: slow_prep] Error 2
Error: Process completed with exit code 2.

[lcn2]

I'm getting an error in the Ci again where there was not one before...

0s
Run make slow_prep
bash: -c: line 14: syntax error: unexpected end of file
make: *** [Makefile:581: slow_prep] Error 2
Error: Process completed with exit code 2.

Same for us. This is an odd issue.

[SirWumpus]

This all was working this morning when I made my PRs, so something changed (which is the point of CI after all).

[lcn2]

Looks like commit c225b96b32c16370d3257880c7e21b257aecbb0d is a fault and we didn't notice it because other things like the lack of a sudo was missing in the workflow. Correcting it now ...

[xexyl]

This all was working this morning when I made my PRs, so something changed (which is the point of CI after all).

Yes. It seems to have been introduced with installing checknr. The rationale was because it doesn't exit anything but 0 which made the test successful no matter what.

But obviously this is a problem. Perhaps it should be removed or the test should no longer run that tool.

Which do you prefer me do Landon? I am happy to do it. It seems like the easiest would be to roll back the install of checknr. Is it necessary? One of us would eventually discover the problem anyway.

[xexyl]

Oh. I know what it is. I must have made a typo with that fun commit.

[xexyl]

I will fix it soon.

[lcn2]

fixing it now

[SirWumpus]

What is the porpoise 🐬 of checknr? Do we really need it when the CI worked without before?

[xexyl]

Okay I won't worry about fixing it then. Thanks for taking care of it.

I should have run all the rules, not just the one. I was in a rush when I made the change: something we all know is not good but something we all do from time to time.

Sorry for that!

[lcn2]

Okay I won't worry about fixing it then. Thanks for taking care of it.

I should have run all the rules, not just the one. I was in a rush when I made the change: something we all know is not good but something we all do from time to time.

Sorry for that!

No worries .. it happens.

We know it is a trivial fix, but we are going their the pre-commit testing anyway .. stay tuned.