ENV setup, Windows tar, Windows bash shell #64

MSP-Greg · 2020-06-12T01:15:12Z

Three Commits:

A. ENV setup, Windows tar, Windows bash shell

B. Add envPreInstall, common.setupPath, use MSYS2 bash for Windows bash

index.js - envPreInstall - sets ENV values for runners
common.js - setupPath - collects all Path operations into one function, runs before Ruby install
Add MSYS2 paths to all Windows builds for MSYS2 Bash use

C. Add Actions 'bash test' and 'Windows JRuby' (gem install sassc) steps

Closes #81

MSP-Greg · 2020-06-12T01:29:36Z

Re CI:

Maybe split it into a few workflows by OS? The number of jobs sort of breaks the web UI. Also, remove a few jobs? The full duplication when running different OS versions seems overkill.
Wondering if a Ruby test script may be helpful for CI, at least on Windows. Things like whether Gem.path contains two items, Gem env has the proper shell entries, etc

Path manipulation is really odd, almost unstable. As mentioned, the 'commands' in core run thru stdout, and it may be async depending on the io type and OS. See https://nodejs.org/api/process.html#process_a_note_on_process_i_o.

I need to look into this more. Since the stdout syntax is published, I may look at using a stdout.write to combine several Path and/or env variable statements. I believe the core methods both modify ENV variables in the node environment and write to stdout for the C# runner to process the command...

MSP-Greg · 2020-06-12T17:43:15Z

Updated to write 'core commands' directly to the runner. This fixes the step env display, and also eliminates rare intermittent failures.

eregon · 2020-06-13T12:52:49Z

Updated to write 'core commands' directly to the runner. This fixes the step env display, and also eliminates rare intermittent failures.

Could you rather fix it upstream in https://github.com/actions/toolkit?
I'd much prefer to use official APIs than hardcoding it ourselves.

MSP-Greg · 2020-06-13T13:18:02Z

I believe it's an official API, see
https://help.github.com/en/actions/reference/workflow-commands-for-github-actions

All that actions/core does is combine the above with doing the same in the current node process.

eregon · 2020-06-13T16:24:50Z

Yes I know, but wouldn't it be good to fix it (i.e., make it a synchronous write) for all actions, not just this one?
Manually writing those still feels suboptimal to me, especially when there is a proper API designed for it.

MSP-Greg · 2020-06-13T17:17:10Z

In theory, it is a sync write on Windows (Boolean(process.stdout.isTTY) is false), but as mentioned, I've seen intermittent errors. Also, behavior varies by OS. And that's just on the node/js side, C# is another issue.

At present, we're using very few ENV calls for Ubuntu or macOS, but that could change.

there is a proper API designed for it

actions/core is a 'helper' action. It may have not been tested against all possible uses. The API is the pipe text description, actions/core is a wrapper around that.

JFYI, js split(':', 2) is equal to Ruby split(':')[0, 2], not Ruby split(':', 2). Either way, : is not a valid directory or file name character in Windows.

MSP-Greg · 2020-06-14T04:02:34Z

Added a class to common.js that handles ENV operations. Replaces all calls to core.exportVariable & core.addPath. As mentioned, the problem this fixes is intermittent, causing failures in less that 5% of jobs (may actually be more like < 1%).

eregon · 2020-06-14T10:55:46Z

common.js

+    this.ENV = process.env
+  }
+
+  addVari(k,v) {


I'd like to keep this action source code, so no abbreviations please.

eregon · 2020-06-14T10:57:09Z

Added a class to common.js that handles ENV operations. Replaces all calls to core.exportVariable & core.addPath. As mentioned, the problem this fixes is intermittent, causing failures in less that 5% of jobs (may actually be more like < 1%).

Can you file a bug to that repo then? And do you have a log of a case(s) that failed?
Maybe it failed for another reason than we expect, or a bug in the runner, etc.
i.e, I'm not convinced core.addPath/exportVariable are to blame here, and if they are we should fix those, not duplicate the logic here.

eregon

I don't want to duplicate code from https://github.com/actions/toolkit/tree/master/packages/core
We should try to figure out exactly what goes wrong, or fix it upstream, or at least file an issue to ask them to investigate.

eregon · 2020-06-14T11:19:14Z

The number of jobs sort of breaks the web UI.

What do you mean? It seems to work fine for me (except being a long list).

eregon · 2020-06-14T11:20:33Z

2. Wondering if a Ruby test script may be helpful for CI, at least on Windows.

Yes, I think that would be useful, and avoid so much commands in test.yml.

MSP-Greg · 2020-06-14T17:44:00Z

What do you mean? It seems to work fine for me (except being a long list).

If one is using one tab and trying to look at Windows logs, one is constantly scrolling the window. PITA. Bad UI, the primary pane (the job list) should srroll independently of the secondary, dependent pane, the log.

Yes, I think that would be useful, and avoid so much commands in test.yml.

I think some should still be ran, but they can run within the script. It's helpful to check if the Windows Rubies have working bash scripts for all the bin files?

I don't want to duplicate code...

I'm working on it. The issue may just be the handling of 'Path', so I'm thinking the common.cmd class maybe should handle core.addPath, along with CleanPath, and use core.exportVariable as before...

MSP-Greg · 2020-06-15T02:35:30Z

@eregon

I got it working using nothing but the core functions. PITA. Most of it seems to be due to removing items from Path, which I think should be done when possible. The most important thing is cleaning the Path first, then having all the core.addPath statements after that.

One odd thing is that the Path shown in the first 'ENV' group in a step (see https://github.com/MSP-Greg/ruby-setup-ruby/runs/771215496#step:17:11) is the path after cleaning, but it doedn't show any additions using core.addPath. See the path output from gem env later on in the same step.

Anyway, all Windows Rubies are running in a bash shell, using their respective devkits.

If PR #65 is okay, I'll revise this after that's merged?

eregon · 2020-07-01T19:31:54Z

#65 is merged now, I am sorry about the delay.
Being able to just use the core functions sounds great.

Regarding removing the system ruby from PATH I'm not sure if it's very important. Maybe we could drop that and only use addPath()? Indeed, the output the show seems like some kind of bug in actions, I think it would be worth reporting to them.

The Ruby we setup should anyway have all executables the system ruby has + some extra, no? And on many development systems it's common to have a system ruby + RVM/rbenv/chruby and they don't remove anything from PATH, so it seems not so much an issue, and something Rubyists already have to deal with.
Is there anything Windows-specific regarding that?
I think maybe JRuby on Windows was confusing as it could sometimes pick the system Ruby's binaries when using ruby ... or ruby.exe ....
So if no longer cleaning the PATH simplifies things or makes the action more reliable I think we could no longer remove the "system Ruby" from PATH.

MSP-Greg · 2020-07-01T20:16:55Z

I am sorry about the delay.

No problem, as it only affects the 'automatic' bundle iinstall. I mentioned revising this, and it's been a couple of weeks. I can't recall. I believe it was stable with the removal.

eregon · 2020-07-02T09:29:33Z

It would also fix using the correct Bash on Windows, right (MSYS2' and not Git' Bash)?
That seems an important fix, to avoid issues like #57.

MSP-Greg · 2020-07-04T16:48:29Z

@eregon

Well, it's mostly finished, see https://github.com/MSP-Greg/ruby-setup-ruby/runs/837069369.

I currently have some Bash/PowerShell code contained in it for CI. Since that code is really about the Windows Ruby builds, there isn't a need in the CI here, at least not on an 'every commit' basis. The bash rake binstub in 2.3 has a bad shebang line, and there maybe something missing in the ruby-head build (bundler?). The issue is that the Windows Ruby's bin folders need files for both cmd/bat usage, and bash usage, and the bash files haven't been used by a lot of people...

Re Path issues, I changed the code back to a single core.exportVariable('PATH', ...), and it's displaying fine in the Actions UI. Part of the Path issue was related to needing a tar command, as Windows 2016 doesn't have a native one like Windows 2019/10. I changed the code to use Git's tar. I'll revise it and open and update this PR sometime soon.

eregon · 2020-07-05T12:49:09Z

https://github.com/MSP-Greg/ruby-setup-ruby/runs/837069345 seems to run in the Git Bash shell (and not the MSYS2 shell). Or is that just a UI bug?

MSP-Greg · 2020-07-05T14:21:37Z

I hate this. Apologies, you're correct, it is the Git shell.

You will note that the log is showing PATH as is set in the code, but it's using the wrong shell. Conversely, in the older code, the Path was shown incorrectly, but the MSYS2 bash shell was used.

I may try to work the casing of Path. The link above shows 'PATH', but as shown at https://github.com/MSP-Greg/ruby-setup-ruby/runs/777119210?check_suite_focus=true#step:4:7, the Path is incorrect, but it is shown as 'Path', and the MSYS2 is used.

So the issue is either the casing, in which case we may be able to get the path shown correctly and the right shell, or the issue is that the C# runner only recognizes path changes via addPath, in which case we probably have to live with an incorrect log of Path. I think that made sense.

Sorry for mixing things, but Ruby ABI is a contract/promise. What we're doing here seems to be a grey area where the Actions API doesn't really promise anything. Let me see if the casing has any affect...

MSP-Greg · 2020-07-05T17:54:45Z

The only way I could get it working causes the logged path to be incorrect. See:
https://github.com/MSP-Greg/ruby-setup-ruby/actions/runs/158427922

One change is that the MSYS2 paths are added to every Windows build, allowing JRuby CI to run with Bash. For older RI/MSYS builds (2.2 & 2.3), the MSYS paths are added before the MSYS2 paths.

I need to dice & chop it into more commits...

eregon · 2020-07-11T17:28:30Z

One change is that the MSYS2 paths are added to every Windows build, allowing JRuby CI to run with Bash

Sounds good.

Would using only addPath and not exportVariable("PATH", value) work both in the UI and use the MSYS shell?
That sounds kind of tempting.

Probably we should report a bug to https://github.com/actions/runner that exportVariable("PATH", value) doesn't work well.

MSP-Greg · 2020-07-11T17:54:47Z

Would using only addPath and not exportVariable("PATH", value) work both in the UI and use the MSYS shell?
That sounds kind of tempting.

Actually, I think I mixed things up. addPath does not show in the UI, only variables that have changes via exportVariable. Since both are being used, I got a bit confused.

I haven't looked at the code for a bit, I did change a few things, mostly to collect all the PATH manipulation into one function, so if the API is changed/updated, it's all in one place.

See actions/runner#576, I just added a comment about adding a function like removeTool, which is all we'd need. I think.

envPreInstall - sets ENV values for runners common.setupPath - collects all Path operations into one function, runs before Ruby install Add MSYS2 paths to all Windows builds for MSYS2 Bash use

MSP-Greg · 2020-08-26T03:46:21Z

Updated. All Windows Rubies now have MSYS2 added to path, so the JRuby issue is fixed, and the MSYS2 bash shell is the Windows default.

slonopotamus · 2020-08-27T08:46:08Z

and the MSYS2 bash shell is the Windows default.

Just to make it clear: this PR will change PowerShell to Bash for any workflow that uses ruby/setup-ruby or I am misunderstanding something? Or now MSYS2 Bash will be used instead of Git Bash?

MSP-Greg · 2020-08-27T13:18:41Z

Sorry I wasn't a bit more clear.

The default shell in Windows should remain as PowerShell, as changing that would break a lot of CI scripts/workflows.

If you run an Action step in a bash shell by changing the default or with the shell: bash setting, the MSYS2 bash shell will be used, not the Git bash shell.

The Git bash shell is an MSYS2 bash shell, but may have a few less commands than the MSYS2 shell currently installed in the Actions Windows runners. The Git shell is probably missing some of the more dev oriented commands.

See:
https://github.com/ruby/setup-ruby/runs/1029689743#step:16:3

vs the current behavior:
https://github.com/rubygems/rubygems/runs/1036211465?check_suite_focus=true#step:5:3

EDIT:

Re the above links, Actions logs are flushed, unlike Travis and AppVeyor. So, six months from now, they won't be available.

The new behavior shows the following in the hidden portion of a Windows bash step:

shell: C:\msys64\usr\bin\bash.EXE --noprofile --norc -e -o pipefail {0}

The old behavior shows:

shell: C:\Program Files\Git\bin\bash.EXE --noprofile --norc -e -o pipefail {0}

Note: MSYS2 provides git packages, but they are not installed on Actions. Hence, regardless of which shell is used, git commands will use the standard Windows git.

slonopotamus · 2020-08-27T13:53:39Z

If you run an Action step in a bash shell by changing the default or with the shell: bash setting, the MSYS2 bash shell will be used, not the Git bash shell.

Understood, thanks.

MSP-Greg · 2020-08-27T15:50:00Z

Since this PR changes the bash shell, and Bundler's Windows CI is ran from a bash shell, I ran its CI using this PR.

All CI passed.

See:
https://github.com/MSP-Greg/rubygems/actions/runs/227189348

eregon · 2020-08-27T18:53:50Z

This looks great, I'll try to review soon.

eregon · 2020-09-03T10:03:11Z

index.js

@@ -26,7 +28,7 @@ export async function run() {
 export async function setupRuby(options = {}) {
  const inputs = { ...options }
  for (const key in inputDefaults) {
-    if (!inputs.hasOwnProperty(key)) {
+    if (!Object.prototype.hasOwnProperty.call(inputs, key)) {


Is this needed? What does it do differently?

This was an ESLint thing. I still use it locally to (hopefully) catch typo's etc. I need a new keyboard, as my old one required a lot less travel.

eregon · 2020-09-03T10:05:01Z

common.js

+  } else {
+    newPath = newPathEntries
+  }
+  core.addPath(newPath.join(path.delimiter))


A loop over paths with a core.addPath() seems nicer (but would need to be done in reverse order).
Since this work though it seems fine.

eregon · 2020-09-03T10:08:43Z

ruby-builder.js

+    [path.join(rubyPrefix, 'bin'), path.join(rubyPrefix, 'gems', 'bin')] :
+    [path.join(rubyPrefix, 'bin')]
+
+  common.setupPath(newPathEntries)


This seems done rather early, even before the rubyPrefix exists. I guess we can move it at the end of this function?

Ah but for Windows it's useful to add MSYS2 in Path for extraction.

This seems done rather early

I can't recall the timeline of this. It involves the afterSetupPathHook, and the issue of installing packages before bundle install. I don't think I've modified setup-ruby-pkgs for that, but I'm not sure. I'm looking at it shortly.

I kept it there, it seems useful notably to get tar from MSYS2 on Windows.

JFYI, as I recall, that was needed only because the Win2016 image didn't have a native tar, which Win2019 did. For instance, locally (Windows 10), I have a native Windows tar

eregon · 2020-09-03T10:12:17Z

windows.js

-      if (k === 'Path') {
-        newPathEntries = v.replace(process.env['Path'], '').split(path.delimiter)
+      if (/^Path$/i.test(k)) {
+        const newPathStr = v.replace(`$(path.delimiter)${process.env['Path']}`, '')


$(path.delimiter) => ${path.delimiter} right?

I checked, that Path was doubled with that, before:
https://github.com/ruby/setup-ruby/runs/1066490911?check_suite_focus=true
(e.g., mysql is twice in Path)
after:
https://github.com/ruby/setup-ruby/pull/83/checks?check_run_id=1066493565

eregon · 2020-09-03T10:13:46Z

@MSP-Greg Great work, thanks! I'll make a PR with small cleanups.

MSP-Greg force-pushed the add-path branch from 4282dbf to 1f1154e Compare June 12, 2020 17:18

MSP-Greg force-pushed the add-path branch from 1f1154e to efb7c9d Compare June 14, 2020 03:53

MSP-Greg changed the title ~~Path setup, Windows tar, Windows bash shell~~ ENV setup, Windows tar, Windows bash shell Jun 14, 2020

MSP-Greg force-pushed the add-path branch from efb7c9d to 21b0c53 Compare June 14, 2020 04:13

eregon reviewed Jun 14, 2020

View reviewed changes

common.js Outdated

this.ENV = process.env

}

addVari(k,v) {

Copy link

Member

eregon Jun 14, 2020

Choose a reason for hiding this comment

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

I'd like to keep this action source code, so no abbreviations please.

eregon requested changes Jun 14, 2020

View reviewed changes

MSP-Greg mentioned this pull request Aug 25, 2020

sassc-ruby fails to install on Windows + JRuby #81

Closed

windows.js - remove code for embedded MSYS2

2f2535c

MSP-Greg force-pushed the add-path branch from 21b0c53 to c928e17 Compare August 25, 2020 22:53

MSP-Greg added 2 commits August 25, 2020 22:43

Add envPreInstall, common.setupPath, use MSYS2 bash for Windows bash

5e49abe

envPreInstall - sets ENV values for runners common.setupPath - collects all Path operations into one function, runs before Ruby install Add MSYS2 paths to all Windows builds for MSYS2 Bash use

Add Actions 'bash test' and 'Windows JRuby' (gem install sassc) steps

b70510f

MSP-Greg force-pushed the add-path branch from c928e17 to b70510f Compare August 26, 2020 03:43

eregon self-requested a review August 27, 2020 18:53

eregon approved these changes Sep 3, 2020

View reviewed changes

eregon merged commit a096b99 into ruby:master Sep 3, 2020

eregon mentioned this pull request Sep 3, 2020

Fix interpolation in addVCVARSEnv() and various cleanups #83

Merged

MSP-Greg mentioned this pull request Sep 3, 2020

Windows - switch to MSYS2 bash shell ? #63

Closed

MSP-Greg deleted the add-path branch September 7, 2020 17:06

This was referenced Jun 6, 2021

chore(deps): update ruby/setup-ruby action to v1.207.0 satoruk/bullet_log_parser#29

Open

chore(deps): update ruby/setup-ruby action to v1.207.0 satoruk/template-ruby-module#20

Open

Update ruby/setup-ruby action to v1.207.0 playglitch/glitch-site#2

Open

ENV setup, Windows tar, Windows bash shell #64

ENV setup, Windows tar, Windows bash shell #64

Conversation

MSP-Greg commented Jun 12, 2020 • edited Loading

MSP-Greg commented Jun 12, 2020

MSP-Greg commented Jun 12, 2020

eregon commented Jun 13, 2020

MSP-Greg commented Jun 13, 2020

eregon commented Jun 13, 2020

MSP-Greg commented Jun 13, 2020

MSP-Greg commented Jun 14, 2020

Choose a reason for hiding this comment

eregon commented Jun 14, 2020 • edited Loading

eregon left a comment

Choose a reason for hiding this comment

eregon commented Jun 14, 2020

eregon commented Jun 14, 2020

MSP-Greg commented Jun 14, 2020 • edited Loading

MSP-Greg commented Jun 15, 2020 • edited Loading

eregon commented Jul 1, 2020

MSP-Greg commented Jul 1, 2020

eregon commented Jul 2, 2020

MSP-Greg commented Jul 4, 2020 • edited Loading

eregon commented Jul 5, 2020

MSP-Greg commented Jul 5, 2020

MSP-Greg commented Jul 5, 2020

eregon commented Jul 11, 2020

MSP-Greg commented Jul 11, 2020

MSP-Greg commented Aug 26, 2020

slonopotamus commented Aug 27, 2020 • edited Loading

MSP-Greg commented Aug 27, 2020 • edited Loading

slonopotamus commented Aug 27, 2020

MSP-Greg commented Aug 27, 2020

eregon commented Aug 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eregon commented Sep 3, 2020

MSP-Greg commented Jun 12, 2020 •

edited

Loading

eregon commented Jun 14, 2020 •

edited

Loading

MSP-Greg commented Jun 14, 2020 •

edited

Loading

MSP-Greg commented Jun 15, 2020 •

edited

Loading

MSP-Greg commented Jul 4, 2020 •

edited

Loading

slonopotamus commented Aug 27, 2020 •

edited

Loading

MSP-Greg commented Aug 27, 2020 •

edited

Loading