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

Windows - switch to MSYS2 bash shell ? #63

Closed
MSP-Greg opened this issue Jun 11, 2020 · 5 comments
Closed

Windows - switch to MSYS2 bash shell ? #63

MSP-Greg opened this issue Jun 11, 2020 · 5 comments

Comments

@MSP-Greg
Copy link
Collaborator

MSP-Greg commented Jun 11, 2020

I've got the MSYS2 bash shell running as the default in my fork.

Using Reline, I set up a workflow where all jobs ran tests both using pwsl (PowerShell Core) and MSYS2 bash. All jobs (Ubuntu, macOS, Win) passed .

Bundler's tests will only run using bash, and they also pass, see https://github.com/MSP-Greg/rubygems/actions/runs/131762536

The one issue is we need to re-arrange things so there is some code that runs for all Rubies on Windows, as it needs to run for Windows JRuby jobs.

The code is in the commit MSP-Greg@b2da215d2df

Thoughts?

EDIT:

Haven't implemented it yet, but this issue is about ENV['Path'] handling. I think the best way to handle it is below:

  1. Remove system Ruby from Path
  2. Add msys2PathEntries to Path
  3. Install Ruby
  4. Add build specific entries (MSYS for Ruby 2.3 and earlier, MSVC for mswin)
  5. Add Ruby bin folder
@eregon
Copy link
Member

eregon commented Jun 11, 2020

So the change is to use core.addPath() instead of core.exportVariable() where possible and that automatically selects the MSYS2 bash when MSYS2 is core.addPath()-ed ? (actions/runner#497 (comment))

Looks good from a quick look, I'll look more at it later.
Would probably be helpful if it's as a PR, so it's easier to comment on specific lines.

@MSP-Greg
Copy link
Collaborator Author

Essentially. I ended looking at both Toolkit/core code and the runner C# code.

I would state that when dealing with Path, core.exportVariable() should be the first call, and only used if one needs to remove items. After that, items should only be added via core.addPath(), which is just string concat'ing, so multiple items can be added at once.

The core code passes the info to the C# runner via process.stdout.write(), so it's the similar to the script commands one can use (most or all start with ::)...

It was also messed up by the call to vcvars.bat in the mswin build code, which modifies Path.

Any the last run used the MSYS2 bash shell for everything Windows except 2.2 & 2.3, which use the MSYS bash shell. MSYS2 in the path for all the commands it has available, and its tar is now used.

Would probably be helpful if it's as a PR

I need to adjust the workflow, rebase, squash, etc. I'll submit later today/tonite (I'm -0500, US CDT). I just wanted to see if you had any issues, but I didn't expect any.

My main goal was to see if people could use either Bash or PowerShell Core code for scripting with all platforms.

@eregon
Copy link
Member

eregon commented Jun 14, 2020

My main goal was to see if people could use either Bash or PowerShell Core code for scripting with all platforms.

Yes, that would be great!

@MSP-Greg
Copy link
Collaborator Author

MSP-Greg commented Sep 3, 2020

See 'Add envPreInstall, common.setupPath, use MSYS2 bash for Windows bash', 6ead459c & #64

Ok to close...

@eregon eregon closed this as completed Sep 3, 2020
@eregon
Copy link
Member

eregon commented Sep 3, 2020

This seems to work fine now, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants