Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix CI passing with failing tests #233

Merged
merged 4 commits into from
Feb 7, 2025
Merged

Fix CI passing with failing tests #233

merged 4 commits into from
Feb 7, 2025

Conversation

ludfjig
Copy link
Contributor

@ludfjig ludfjig commented Feb 3, 2025

Fixes #138

Somehow pwsh won't always fail the job if a step fail.
This PR cannot merge until the tests are passing

@ludfjig ludfjig marked this pull request as draft February 3, 2025 20:36
@marosset
Copy link
Contributor

marosset commented Feb 5, 2025

IIRC you can have powershell check the $LASTEXITCODE or something similar which gets set when non-powershell processes are run.
Let me find an example from when I've had to use that int he past...
that might be cleaner than switching to bash (which may break things like env var setting)

@marosset
Copy link
Contributor

marosset commented Feb 5, 2025

@marosset
Copy link
Contributor

marosset commented Feb 5, 2025

Maybe we can try updating set windows-shell to something like

set windows-shell := [
  "pwsh.exe",
  "-NoLogo",
  "-Command",
  " & { $cmd = $args -join ' '; Invoke-Expression $cmd; if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE } }"
]

@marosset
Copy link
Contributor

marosset commented Feb 5, 2025

Maybe we can try updating set windows-shell to something like

set windows-shell := [
  "pwsh.exe",
  "-NoLogo",
  "-Command",
  " & { $cmd = $args -join ' '; Invoke-Expression $cmd; if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE } }"
]

This is issues where powershell stops proccessing everything after the -- in some of the commands :-/

@ludfjig ludfjig force-pushed the fix_hidden_pwsh_error branch from c1069db to 463e31d Compare February 5, 2025 22:41
Signed-off-by: Ludvig Liljenberg <[email protected]>
Signed-off-by: Ludvig Liljenberg <[email protected]>
Signed-off-by: Ludvig Liljenberg <[email protected]>
@ludfjig ludfjig force-pushed the fix_hidden_pwsh_error branch from 463e31d to 082f830 Compare February 5, 2025 23:07
@ludfjig ludfjig marked this pull request as ready for review February 5, 2025 23:09
@simongdavies
Copy link
Contributor

Just wondering if Mark's suggested change is better since IIUC this fix will work for the specific GH job but if the just command is run locally in Windows or from another GH job then the same failure will still be there?

@ludfjig
Copy link
Contributor Author

ludfjig commented Feb 6, 2025

Just wondering if Mark's suggested change is better since IIUC this fix will work for the specific GH job but if the just command is run locally in Windows or from another GH job then the same failure will still be there?

Mark and I dug a bit deeper into this. Unfortunately this is the only solution we came up with, as the other suggestions did not seem to work. Locally on windows, this doesn't modify anything, and at least on my computer, powershell behaves as expected and exits with the right exit code.

If this specific workflow gets invoked from somewhere else, the bash shell will still apply, however if in another workflow someone uses the same just-command without specific a bash-shell, indeed the problem will resurface again. @simongdavies

@marosset
Copy link
Contributor

marosset commented Feb 6, 2025

yes, i had issues getting anything after -- in the cargo commands to not be removed by powershell processing.
I tried a few things like adding --% after the .exe call and couldn't get anything to work

@simongdavies
Copy link
Contributor

however if in another workflow someone uses the same just-command without specific a bash-shell, indeed the problem will resurface again

@ludfjig OK, can we put a comment in the just file somewhere to that effect then?

Signed-off-by: Ludvig Liljenberg <[email protected]>
@ludfjig ludfjig force-pushed the fix_hidden_pwsh_error branch from 3d3854b to 5dfd64e Compare February 6, 2025 21:33
@marosset
Copy link
Contributor

marosset commented Feb 7, 2025

Will just commands continue to work from powershell on Windows machines.
Personally I don't like using bash on Windows and I would like to make sure the current behavior works if possible

Copy link
Contributor

@marosset marosset left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if i understand this correctly, the github CI runnings will spawn a bash shell which will run just, which will then spawn powershll shell's for whatever just executes?

Seems odd but if it gets us a better CI signal this works for me.

@ludfjig
Copy link
Contributor Author

ludfjig commented Feb 7, 2025

Will just commands continue to work from powershell on Windows machines. Personally I don't like using bash on Windows and I would like to make sure the current behavior works if possible

This only affects ci. Any just commands on windows will still spawn a pwsh shell (even on ci).

@ludfjig ludfjig merged commit f9c4416 into main Feb 7, 2025
21 checks passed
@ludfjig ludfjig deleted the fix_hidden_pwsh_error branch February 7, 2025 22:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failed test cases not causing CI jobs to fail
3 participants