Bad Makefile target "gha"

[rainij]

I propose to replace the Makefile target gha by something more straightforward. Main reason: it is complicated and seems to be unreliable (see below), and hard to understand (my opinion). Moreover I think it would be better to run all tests on a pull request and merge (This seems to be not the case right now).

A possible (straightforward) solution might be (essentially) to replace make gha by make test everywhere. In any case one should make sure that it is not prone to silent failures like make gha (see below).

I would further propose to just delete gha. In case this is not desired I would at least propose to rename the target to something else. The current name does not really tell us what it does. Of course the name says "I should be used by GitHub Actions" (to anybody who already knows what GHA is) but in my opinion it should rather say what it does (maybe a comment would help too). If the repo were hosted on other platforms like gitlab the current name would probably not make sense anymore.

Why it is (probably) unreliable: It can silently fail. In a previous PR (https://github.com/exercism/sml/pull/193) I observed and fixed a syntax error in it. The CI pipeline (GitHub Actions) previously did not detect this error. The logs showed that something was odd but the job was marked as a success. So in any case, even if this is not the makefile's fault, at least something needs to be done about that. I never used GitHub Actions so I do not know if the reason for this might also be due to a not optimally configured workflows folder. Following the log for the "successful" job, as printed out before the mentioned PR:

From https://github.com/exercism/sml
 * branch            main       -> FETCH_HEAD
 * [new branch]      main       -> origin/main
Warning: Use of "-z-_" without parentheses is ambiguous at -e line 1.
Backslash found where operator expected at -e line 1, near ")\"
    (Missing operator before \?)
syntax error at -e line 1, near "/exercises\/practice/("
Execution of -e aborted due to compilation errors.
---------------
HEAD: b448531594154db7a0b92704f894ca1c827febed
master:
COMMIT_RANGE: origin/main..b448531594154db7a0b92704f894ca1c827febed
GITHUB_EVENT_NAME: pull_request
GITHUB_SHA: b448531594154db7a0b92704f894ca1c827febed
Modified/Added:
M   CODE_OF_CONDUCT.md
Renamed:
---------------
Nothing to test.

One last remark. I am new to the exercism project, so some things I propose might be bad. For that reason it would be nice if somebody with more exercism/sml-experience could comment on this before any actions are taken.

[kotp]

Thank you for the observations. I wanted to let you know that the issue is seen, and I also wanted to let you know that I appreciate the thoughts on the abbreviation gha as a name. Without it being defined on first use, it can be very confusing to know what that is without prior knowledge. At least using the full name in some way will let someone research it for more information with less troubles.

So I am for the change there, and maybe even Exercism wide.

[rainij]

Thank you @kotp for your feedback.

I wonder if one might drop the gha target entirely here? Or at least not to use it as a replacement to run all tests? Currently the target seems to try to find out which exercises changed and to run only tests for those. I would prefer to just run all tests. I hope there are no resource constraints on github preventing this (note: when I run all tests locally, using make test, it takes less then 5 seconds on my notebook).

[kotp]

If this is "per organization" then collectively it may may sense to optimize for time necessary to keep within limits.

[ErikSchierboom]

I like the suggestion of just dropping the gha target entirely if it is causing issues. It would need to be removed in two places for CI to work correctly:

https://github.com/exercism/sml/blob/main/.github/workflows/ci.yml https://github.com/exercism/sml/blob/main/.github/workflows/ci-pull-request.yml

Note: I'm not opposed to the gha target being fixed, but I don't know how to do that. We could also replace make gha with make test in the workflows, and then leave this issue open for someone to fix the gha target at a later time. That might be the best option actually.

[rainij]

I'm for replacing gha by test in the files mentioned by @ErikSchierboom too.

Concerning deleting vs fixing gha: It would be nice if somebody could tell the intent of the target. I do not mean to ask what it does, I mean who or what benefits from it? If nobody has a reason to keep it I would vote for deleting it. Not sure whom to ask.

[ErikSchierboom]

I think the goal of that target is to only run the tests for any exercises that have changed within the PR.

[rainij]

@ErikSchierboom yes I know (or at least also think) that this is what gha does. My questions goes into the direction "who needs it, and does it more confuse then to help". I would never rely on that target if I did any changes anywhere. I would always try to run all tests to check if something is broken. Single tests can also be run with make test-<name of exercise>, which is necessary too for development.

Of course, that it is better to delete the target gha is only my opinion. I wouldn't object if someday says "we need target gha for this or that reason".

I just want to know what can be done to resolve the issue. I think somebody wanting to open a PR to resolve it would like to know the answers to the following questions (at least):

• Replace gha by test in github actions? Already answered with "yes".
• Delete or leave gha? Still open what to do.

Probably I didn't formulate the issue very well. I try to make it better next time.

[ErikSchierboom]

I'd vote for:

• Replace gha by test
• Remove the gha target (we can always restore it later)

[rainij]

I close this because the issue is resolved by merge of https://github.com/exercism/sml/pull/199.