Finish GH Actions migration #399

JordanMartinez · 2022-03-03T22:49:59Z

#398 wasn't triggering CI because the PR was coming from outside of the repo, so it got merged. However, that PR introduced another .yml file that wasn't needed. This removes it.

JordanMartinez · 2022-03-04T00:11:39Z

Ok, now we just need to get tests to pass again.

JordanMartinez · 2022-03-04T00:38:08Z

Seems like we may have a flaky test. Running locally, the handles consecutive delimiters for --include test failed. Re-running it succeeded.

JordanMartinez · 2022-03-04T00:42:38Z

Hey! CI passes now 😄 🎉

JordanMartinez · 2022-03-04T00:47:21Z

Now the question is how to add the Windows build back in...

JordanMartinez · 2022-03-04T13:27:43Z

Windows CI now builds. This PR should be ready for a review. The last few commits were just cleaning up the ci.yml file a bit.

JordanMartinez · 2022-03-04T15:48:38Z

Actually, one other thing we'll need to do here is verify that pulp works across multiple PS versions (i.e. it works on v0.14.x and the upcoming v0.15.x versions).

JordanMartinez · 2022-03-04T19:51:28Z

CI builds. This PR is ready for review. If we need to test a new PS version, we can add it pretty easily via the matrix.

- purs was updated to v0.14.5 - only more recent versions are being tested - psc-package is installed via NPM

thomashoneyman · 2022-03-04T21:31:24Z

.github/workflows/ci.yml

-        os: [ubuntu-latest, windows-latest]
-        PURS_TEST_VERSION: [v0.12.3, v0.12.5]
-    runs-on: ${{ matrix.os }}
+        purs_test_version: [v0.14.4, v0.14.5]


I wonder whether we could get away with only testing against a single PureScript version? In that case we could use setup-purescript to install the Windows, Linux, and macOS versions of the compiler. For example, you can see that all three are used in the integration test:

https://github.com/purescript-contrib/setup-purescript/blob/main/.github/workflows/integration.yml

That might make these installations a little less involved. Or is there a reason we need to test against multiple versions, or have the distinction between BUILD_VERSION and TEST_VERSION that I'm missing?

That might make these installations a little less involved. Or is there a reason we need to test against multiple versions, or have the distinction between BUILD_VERSION and TEST_VERSION that I'm missing?

How will we verify that pulp run works when in one case we're using v0.15.0 and in another we're using v0.14.5? We need to verify that both versions will work with the same version of pulp.

The distinction exists because the Pulp source code can generally only be compiled by one PureScript compiler version at a time, but the compiled code needs to be tested against multiple compiler versions - the user won’t necessarily be using the same version as the one we used to build Pulp. I think testing against multiple compiler versions is necessary, since pulp is expected to work with multiple compiler versions, and especially since different compiler versions cause different code paths to be taken in pulp. Testing against multiple compiler versions has saved my skin a number of times in the past.

It looks like we are no longer testing against 0.12 and 0.13 now, but I’d strongly recommend continuing to test against at least one compiler version from every version series that is still supported by pulp, and in particular to test on the very earliest version that’s still supported. It’s very easy to accidentally depend on a compiler flag that doesn’t exist in all of the supported versions. Dropping support for old versions is also fine but I think it’s important that people who try to use those old versions with the new pulp get reasonable error messages (via the mechanism in Pulp.Validate).

I agree that we should continue to support some older compiler versions than what this PR was currently testing. I've added back v0.12.4 and v0.12.5, which this PR had removed. I've also added v0.13.0 and replaced v0.14.4 with v0.14.0 to see if it triggers any such failures.

I haven't added back in v0.11.0, but that was because I git pushed before remembering to add it. I think it's likely save to drop support for that, but I think we should continue to support if its tests still pass.

Something weird is going on here - this doesn’t seem like something that should be happening as pulp doesn’t directly impact the JS the compiler produces. Are the tests perhaps trying to bundle some compiled PureScript code with a different version of purs than the one that compiled that code somehow?

Are the tests perhaps trying to bundle some compiled PureScript code with a different version of purs than the one that compiled that code somehow?

I believe so. The error itself says
purs bundle "output/*/*.js" --output unit-tests.js --module Test.Main --main Test.Main && node unit-tests.js
so it's bundling output/*/*.js, which was compiled by the build version.

Okay, can we have that purs bundle command use the build version of purs rather than the test version?

Yeah, I split the test:unit part from the test:integration part, and then had to update the dependencies installed when pulp init runs.

v0.11.0 still doesn't build for two reasons. First, there's not psc-package for that release apparently. Second, the code to run uses Effect, not Eff. So, I think we should drop it because that will be 4 major releases behind current once v0.15.0 is out.

Yeah, I think dropping support for 0.11 is totally reasonable.

JordanMartinez · 2022-03-04T21:49:56Z

Last few commits are to clean up the imports and warnings so future PRs have less noise going on.

JordanMartinez · 2022-03-08T14:33:18Z

CI now builds on v0.12.0, v0.12.4, v0.12.5, v0.13.0, v0.14.0, and v0.14.5

JordanMartinez · 2022-03-08T14:33:36Z

Do I need to add an entry to the changelog in this PR? Or will we do that in the release?

src/Pulp/Run.purs

src/Pulp/Validate.purs

hdgarrood · 2022-03-08T14:39:55Z

.github/workflows/ci.yml

-        os: [ubuntu-latest, windows-latest]
-        PURS_TEST_VERSION: [v0.12.3, v0.12.5]
-    runs-on: ${{ matrix.os }}
+        purs_test_version: [v0.12.0, v0.12.4, v0.12.5, v0.13.0, v0.14.0, v0.14.5]


It'd be nice to pull this out so that it can be reused by both the windowsBuild and the nonWindowsBuild, so that the test versions can't get out of sync between the windows and non-windows builds.

I would, but it's not clear to me how to do that. I could use one build and then use a lot of if: runner.os != 'Windows' conditions, but I didn't like that. So, I split it into two.

🤷‍♂️ I merged them back into one build because I did make the mistake of only updating one of those arrays in #401.

Drop trigger.yml

8eb8609

JordanMartinez added 9 commits March 4, 2022 15:21

Update deps to versions listed in latest package set

3400ca1

Update package-lock.json from v1 to v2

41bcde1

Ignore additional files

e071477

Fix Version arg; apply suggestions; code compiles

469b248

Remove unused values/fns

c2461b9

Drop unused noPsa (handled by shouldUsePsa)

831bad5

Install psc-package via npm

bbfa686

Run CI on ubuntu and windows

92b01d8

- purs was updated to v0.14.5 - only more recent versions are being tested - psc-package is installed via NPM

Ignore unused arg

3862469

JordanMartinez force-pushed the finish-gh-migrations-move branch from c76a8cb to 3862469 Compare March 4, 2022 21:30

thomashoneyman reviewed Mar 4, 2022

View reviewed changes

JordanMartinez added 4 commits March 4, 2022 15:43

Sort imports

eef4720

Drop unused import

2abfe7c

Fix prelude import warning

c82b118

Fix more compiler warnings

770a3a3

This was referenced Mar 4, 2022

Support upcoming v0.15.0 compiler #401

Merged

Migrate FFI to ES modules purescript/purescript-prelude#287

Merged

JordanMartinez added 4 commits March 7, 2022 15:51

Test more PS versions

f2651c5

Run all builds to completion

534a023

Add v0.11.0 back

fdc6cb9

Fix indentation

40794bd

JordanMartinez force-pushed the finish-gh-migrations-move branch from 7e5c235 to 3eaf9f2 Compare March 7, 2022 22:45

Run unit and integration tests separately

eb1ee70

JordanMartinez force-pushed the finish-gh-migrations-move branch from 2259d15 to eb1ee70 Compare March 8, 2022 01:49

JordanMartinez added 4 commits March 7, 2022 19:56

Non-standard scripts require 'run' command

5fe1d6b

Use compiler-version-specific deps on init

5fb2e5b

Drop support for PureScript v0.11.0

6b79581

Bump min psa version to v0.7.0

90841e1

hdgarrood approved these changes Mar 8, 2022

View reviewed changes

JordanMartinez added 3 commits March 8, 2022 08:57

Add changelog entry

dfccf2a

Delete commented out old code

eaf0e58

Merge windows build into ubuntu build

cf1469e

JordanMartinez merged commit 0523cd3 into master Mar 8, 2022

JordanMartinez deleted the finish-gh-migrations-move branch March 8, 2022 15:30

Finish GH Actions migration #399

Finish GH Actions migration #399

Uh oh!

Conversation

JordanMartinez commented Mar 3, 2022

Uh oh!

JordanMartinez commented Mar 4, 2022

Uh oh!

JordanMartinez commented Mar 4, 2022

Uh oh!

JordanMartinez commented Mar 4, 2022

Uh oh!

JordanMartinez commented Mar 4, 2022

Uh oh!

JordanMartinez commented Mar 4, 2022

Uh oh!

JordanMartinez commented Mar 4, 2022

Uh oh!

JordanMartinez commented Mar 4, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JordanMartinez commented Mar 4, 2022

Uh oh!

JordanMartinez commented Mar 8, 2022

Uh oh!

JordanMartinez commented Mar 8, 2022

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!