fix ci run command

[mpizenberg]

Let's hope GitHub runs it in the PR now :)

[mpizenberg]

So the CI is now passing, in roughly less than 30s, where most of the time is passed doing haskell stuff I don't understand. The question now is are you ok using elm-tooling to install elm and elm-format? and do you want to keep the package.json?

I opted for elm-tooling since it's more efficient for CI but the package.json has one advantage which is that you can run the ./tests/make.sh && (cd tests && ./test.sh) && elm-format ... script directly with npm test when doing local dev. It would thus be doing the same thing locally and on CI, but it also merges (and multiplies unnecessarily) calls to elm-format for the different platforms, unless we split test and formatting in package.json.

[mpizenberg]

PS, is the haskell stuff needed since the else branch in test.sh just emits a warning and a versions.dat is versioned in git?

[harrysarson]

Yeah skipping haskell is completely fine.

Just thinking about the duplication between the code we run in package.json and in the ci config; would be nice to avoid having to keep the two in sync.

[mpizenberg]

would be nice to avoid having to keep the two in sync.

There are two things duplicated here, the installation and the running scripts. I can see a few ways to help with both, with trade offs for each.

1. Remove entirely the elm-tooling.json. We rely on package.json to install the dependencies, both locally and on CI. We should split the scripts in the package.json between test and format so that we do not run elm-format on each platform. Then we can add another npm script calling both on them for convenience in local dev.
2. Remove entirely the package.json. Just make a small helper bash script (or a couple of these) to run all tests and check formatting locally (this can be called in CI since bash is available on all plateforms there).
3. Keep both, but remove the dependencies section of the package.json. It only serves as a script helper that we can call both locally and in CI since npm is available on CI anyway even if we theoretically don't need it.

Let me know what you think of those options, and if you have other ideas.

[harrysarson]

Keep both, but remove the dependencies section of the package.json. It only serves as a script helper that we can call both locally and in CI since npm is available on CI anyway even if we theoretically don't need it.

Remind me how folk would install elm tools locallywith this approach? Otherwise this sounds best to me.

[mpizenberg]

With the approach where we keep both, folks would install locally elm and elm-format by running

# install elm and elm-format
# this add links to the binaries in node_modules/.bin
elm-tooling install

Then they can call them via npx just like when installed with npm. And the npm scripts can access them just with their name since it adds node_modules/.bin to the path automatically.

[mpizenberg]

There is something not working on windows and I'm not understanding why since I'm not a windows user. I think the problem when running the npm script is that it's run via powershell and something is wrong but I don't know what.

[mpizenberg]

By using "test": "bash tests/test.sh" instead of "test": "tests/test.sh" the tests are now running even in windows. However now it does not find rsync. So I suppose it does not have access to the same path of binaries than when running directly the scripts in bash from the CI yaml spec.

[mpizenberg]

Sorry for all the pushed commits to run the CI.

I'm not figuring out what PATH issue there is preventing the rsync call. Or at least it's getting too annoying to try to fix now for me. I'll see later if I can figure it out. For now, it seems that running directly with run: tests/test.sh in the action works but doesn't via bash npm.

Also, I'm saying "seems" to work, because it terminates without an error but it also does not print anything at all while the script should print stuff, so maybe it does not work. I'd have to check with failing tests to make sure. Leaving that for another time too.

[harrysarson]

I think just not running on windows would be fine. The bash script just does some hacks to run elm-test with custom kernel code. I think this should be reliably cross platform.

[mpizenberg]

I think just not running on windows would be fine

That's an option too! probably the easiest. I'll update to cleanup and remove windows from CI then.

[mpizenberg]

So for the latest version of this PR we are doing the following:

• Install is handled by elm-tooling.json. Locally you call elm-tooling install, and on CI we use elm-tooling-action. Then elm and elm-format can be called with npx elm and npx elm-format just like when installed with npm.
• Scripts are handled by package.json, which do not contains a dependency section anymore. The test is split in 2, a pre-test that calls tests/make.sh and a test that calls /tests/test.sh. pre-test is automatically called by npm when running npm test. And to check formatting, we can use npm run format.

I think this is now clearer than before and there is no dedup anymore between local and CI.

[mpizenberg]

1s

[mpizenberg]

I had forgotten an echo $PATH in the test script that I used when trying to debug. All clear now

[harrysarson]

Thanks @mpizenberg is great to have CI back!