fix: bail if purs exits non-ok #1285

cakekindel · 2024-09-16T17:22:19Z

Description of the change

TODO

Checklist:

Added the change to the "Unreleased" section of the changelog
Added some example of the new feature to the README
Added a test for the contribution (if applicable)

P.S.: the above checks are not compulsory to get a change merged, so you may skip them. However, taking care of them will result in less work for the maintainers and will be much appreciated 😊

cakekindel · 2024-09-16T17:50:20Z

root@3a9a77b24532:/app# node ../spago/bin/bundle.js build
Reading Spago workspace configuration...

✓ Selecting 14 packages to build:
  ...
  
Downloading dependencies...
Building...
‼ Failed to decode PsaResult at index '0': Unexpected end of JSON input
Json was: 

✘ purs exited with non-ok status code: BySignal "SIGKILL"

cakekindel · 2024-09-16T17:51:55Z

hmm maybe dying isn't the right thing to do?

f-f · 2024-09-16T21:52:27Z

Uh, it looks like this uncovered a funny bug - CI is failing on this test, which should have never passed in the first place because we are supposed to fail in that condition

f-f · 2024-09-16T21:58:42Z

src/Spago/Psa.purs

    logSuccess "Build succeeded."
    pure true
+  else if Array.all identity arrErrorsIsEmpty && not (Cmd.exitedOk result) then do


Having this code structured as a series of if/else makes it extra confusing, let's move it to a case statement, e.g.

case Array.all identity arrErrorsIsEmpty, Cmd.exitedOk result of -- No errors, and 0 exit code true, true -> ... -- No errors, but non-zero exit code, so something's up true, false -> ... -- Compilation errors, print them out false, _ -> ...

Let's also try to catch the JSON decoding error earlier: the scenario we are trying to prevent here has err being an empty string. So let's try to not print the warning if that's the case, hopefully it will reduce the log noise.

ok no problem, ended up changing the output traversal to collect into an array of eithers to defer that logging and changed to a case expression

f-f

Code looks good - could we add a test for this? The best way I can think of is to try to pass a nonexistent flag to purs.

cakekindel · 2024-10-16T20:30:57Z

Sorry forgot about this. Will do tomorrow!

f-f · 2024-10-31T13:00:09Z

@cakekindel did you get around to finish this or shall I take care of the last bits? I'd like to get this merged ASAP before it gets out of date

cakekindel · 2024-10-31T16:10:36Z

@cakekindel did you get around to finish this or shall I take care of the last bits? I'd like to get this merged ASAP before it gets out of date

Sorry I forgot again, updated & added test!

Chose to have the test override the purs binary so that we can directly control its behavior rather than trying to provoke a specific behavior from purs
Failure message ("IF YOURE SEEING THIS ...") is searchable. I can only imagine how awful it would be to identify this test as the root cause of failures if someone tried to run them in parallel for example 🥴
Test fails on master e7a4e7e

test/Spago/Build.purs

f-f

This is good, thanks! 👏

Let's see if I can merge with just applying the suggestion or if we need to fight CI any further

test/Spago/Build.purs

cakekindel · 2024-11-02T19:04:04Z

Is it still CRLF? No diff in log output 😕

add contrarian newlines to the long list of problems bill gates is responsible for cursing us with on this godforsaken hellscape we call Earth.

f-f · 2024-11-03T17:53:53Z

@cakekindel could I ask you to not force push? It makes it harder to follow the changeset over the history of the PR.
We already have a way to deal with Windows newlines, and that that's through the sanitize hook; I pushed something that should approximate a fix in this commit. We use it in a similar way across all the tests, and I'd like to keep that consistent (we do use separate fixtures for windows in other places, but it's best to avoid those if possible, as they are annoying to regenerate)

cakekindel · 2024-11-03T21:05:05Z

@cakekindel could I ask you to not force push? It makes it harder to follow the changeset over the history of the PR.

Understood! I can definitely respect this, but also ask in return for patience / understanding as my instinct is not to prioritize a linear history on working branches. I may rebase onto upstream rather than merge by mistake.

I usually don't force push amendments, not sure why i did that here

We already have a way to deal with Windows newlines, and that that's through the sanitize hook; I pushed something that should approximate a fix in this commit. We use it in a similar way across all the tests, and I'd like to keep that consistent (we do use separate fixtures for windows in other places, but it's best to avoid those if possible, as they are annoying to regenerate)

I saw this after I realized I was out of date but the test still failed for some reason.

I'll revert my commits on top of yours and try to get your solution passing on windows, sorry 😔

f-f · 2024-11-04T10:20:44Z

Understood! I can definitely respect this, but also ask in return for patience / understanding as my instinct is not to prioritize a linear history on working branches. I may rebase onto upstream rather than merge by mistake.

Oh yeah no worries! It's not a hard rule, but using merge commits instead of rebasing makes it harder to lose content if I'm pushing to the same branch when trying to get things merged in 🙂
If you wouldn't like me to mess with your branch at all you can disable that - there should be a checkbox in the PR settings "allow edits from maintainers".

…ows." This reverts commit 9561f44.

cakekindel · 2024-11-04T16:25:08Z

test/Prelude.purs

+  let
+    renderNonPrinting =
+      String.replaceAll (String.Pattern "\r") (String.Replacement "␍")
+      >>> String.replaceAll (String.Pattern "\t") (String.Replacement "␉-->")
+  in


If this is contentious i can undo, just thought it would be a good unobtrusive addition to catch failures early in CI

Ah yeah, I like it! (for now, let's see how it fares over time 😂)

f-f

OMG CI passes 🎉 thank you again @cakekindel for getting this over the finish line! ❤️

cakekindel changed the title ~~draft: fix: bail if purs exits non-ok~~ fix: bail if purs exits non-ok Sep 16, 2024

f-f reviewed Sep 16, 2024

View reviewed changes

f-f mentioned this pull request Sep 16, 2024

Fix detection of some flags passed to purs #1286

Merged

f-f reviewed Sep 21, 2024

View reviewed changes

cakekindel added 7 commits October 31, 2024 10:21

fix: bail if purs exits non-ok

641641f

oops :)

2ce7e9f

refactor

51615fb

rm comment

87383a3

re-add comment

1ed3560

the last branch could be point-free as well

127d3e3

format

59b746d

cakekindel force-pushed the master branch from 69476be to 59b746d Compare October 31, 2024 15:22

test: add test for bad purs exit

7b47e88

f-f reviewed Nov 1, 2024

View reviewed changes

test/Spago/Build.purs Outdated Show resolved Hide resolved

windows makes me sad :(

4f16b32

f-f approved these changes Nov 1, 2024

View reviewed changes

test/Spago/Build.purs Outdated Show resolved Hide resolved

f-f added 2 commits November 2, 2024 00:24

Update test/Spago/Build.purs

cbb29d2

Merge branch 'master' into master

7ba30d7

f-f reviewed Nov 1, 2024

View reviewed changes

test/Spago/Build.purs Outdated Show resolved Hide resolved

I love you windows

bab00b5

cakekindel mentioned this pull request Nov 2, 2024

Spago fails to build on Windows and Node v 21.7 #1219

Open

This comment was marked as resolved.

Sign in to view

fix: test accounts for stderr being separated by crlf on windows.

9561f44

add contrarian newlines to the long list of problems bill gates is responsible for cursing us with on this godforsaken hellscape we call Earth.

cakekindel force-pushed the master branch from 38fc8b2 to 9561f44 Compare November 2, 2024 21:58

cakekindel added 2 commits November 4, 2024 09:38

revert "fix: test accounts for stderr being separated by crlf on wind…

7b5a704

…ows." This reverts commit 9561f44.

fix: replaceAll, render non-printing when strings not equal

bf117b8

cakekindel commented Nov 4, 2024

View reviewed changes

rerun macos job (apparently module build order can be nondeterministic?)

3ac5d90

f-f approved these changes Nov 4, 2024

View reviewed changes

f-f merged commit fb24ee6 into purescript:master Nov 4, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: bail if purs exits non-ok #1285

fix: bail if purs exits non-ok #1285

cakekindel commented Sep 16, 2024 •

edited

Loading

cakekindel commented Sep 16, 2024

cakekindel commented Sep 16, 2024

f-f commented Sep 16, 2024

f-f Sep 16, 2024 •

edited

Loading

cakekindel Sep 17, 2024

f-f left a comment

cakekindel commented Oct 16, 2024

f-f commented Oct 31, 2024

cakekindel commented Oct 31, 2024 •

edited

Loading

f-f left a comment

cakekindel commented Nov 2, 2024 •

edited

Loading

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

f-f commented Nov 3, 2024 •

edited

Loading

cakekindel commented Nov 3, 2024 •

edited

Loading

f-f commented Nov 4, 2024

cakekindel Nov 4, 2024

f-f Nov 4, 2024

f-f left a comment

fix: bail if purs exits non-ok #1285

fix: bail if purs exits non-ok #1285

Conversation

cakekindel commented Sep 16, 2024 • edited Loading

Description of the change

Checklist:

cakekindel commented Sep 16, 2024

cakekindel commented Sep 16, 2024

f-f commented Sep 16, 2024

f-f Sep 16, 2024 • edited Loading

Choose a reason for hiding this comment

cakekindel Sep 17, 2024

Choose a reason for hiding this comment

f-f left a comment

Choose a reason for hiding this comment

cakekindel commented Oct 16, 2024

f-f commented Oct 31, 2024

cakekindel commented Oct 31, 2024 • edited Loading

f-f left a comment

Choose a reason for hiding this comment

cakekindel commented Nov 2, 2024 • edited Loading

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

f-f commented Nov 3, 2024 • edited Loading

cakekindel commented Nov 3, 2024 • edited Loading

f-f commented Nov 4, 2024

cakekindel Nov 4, 2024

Choose a reason for hiding this comment

f-f Nov 4, 2024

Choose a reason for hiding this comment

f-f left a comment

Choose a reason for hiding this comment

cakekindel commented Sep 16, 2024 •

edited

Loading

f-f Sep 16, 2024 •

edited

Loading

cakekindel commented Oct 31, 2024 •

edited

Loading

cakekindel commented Nov 2, 2024 •

edited

Loading

f-f commented Nov 3, 2024 •

edited

Loading

cakekindel commented Nov 3, 2024 •

edited

Loading