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

setup-ruby removes chocolatey from PATH #19

Closed
slonopotamus opened this issue Feb 9, 2020 · 13 comments
Closed

setup-ruby removes chocolatey from PATH #19

slonopotamus opened this issue Feb 9, 2020 · 13 comments

Comments

@slonopotamus
Copy link

See trivial workflow that reproduces the problem: https://github.com/slonopotamus/asciidoctor-diagram/runs/434520900?check_suite_focus=true

Before setup-ruby actions is called, there's C:\ProgramData\Chocolatey\bin on PATH. But it is gone after setup-ruby is called.

For the reference, here's workflow file:

name: CI
on: [push, pull_request]
jobs:
  test:
    runs-on: windows-latest
    steps:
      - run: |
          echo "Before setup-ruby"
          $Env:Path
          choco --version
      - uses: ruby/setup-ruby@v1
        with:
          ruby-version: 2.7
      - run: |
          echo "After setup-ruby"
          $Env:Path
          choco --version
@MSP-Greg
Copy link
Collaborator

MSP-Greg commented Feb 9, 2020

@slonopotamus

I'm essentially responsible for the code that does that.

On CI systems, there's often issues with both multiple copies of applications being available and also 'dll resolution hell'. It's one thing to have an error showing that a dll can't be found, it's much worse if a different dll is loaded and then has unexpected results.

Then again, one should be aware of issues like this, but a lot of macOS or *nix users won't expect them.

Regardless, I don't have an issue with removing the code. Can your workflows work around the path changes?

JFYI, most publicly facing Ruby MinGW builds use a manifest for dll resolution, and are bundled with their own dlls. But, the ruby/ruby CI is done without one.

@eregon
Copy link
Member

eregon commented Feb 9, 2020

Thanks for the report.
@MSP-Greg So what do you think, should we just remove Chocolatey from that line?

path = path.filter(e => !e.match(/\b(Chocolatey|CMake|mingw64|OpenSSL|Strawberry)\b/))

We could also have an option to not filter the PATH at all (on Windows).
But for example having another ruby in PATH leads to very confusing errors, so at the very least we should remove that one.

mingw64 should likely also be removed as it would conflict with msys2.
Strawberry Perl is also a sort of MinGW if I understand correctly, so is similar.

Not sure about CMake and OpenSSL. Maybe msys2 already provide these 2?

@slonopotamus
Copy link
Author

slonopotamus commented Feb 9, 2020

Can your workflows work around the path changes?

Sure, I can workaround this by reordering steps. It was just really unexpected and not very trivial to diagnose.

Hmm... Or I actually can't because I need other software installed via Chocolatey to be on PATH. Even if I manage to install them before you remove Chocolatey dir from PATH, they won't be available anymore for Ruby program.

@eregon
Copy link
Member

eregon commented Feb 9, 2020

It was just really unexpected and not very trivial to diagnose.

I'm sorry about that.

FWIW, here is what is in C:\ProgramData\Chocolatey\bin: https://github.com/eregon/setup-ruby-test/runs/434763137
So notably C/C++/Fortran compiling tools which might possibly conflict with msys2.

I'll make a PR and ask @MSP-Greg to review it.

@MSP-Greg
Copy link
Collaborator

MSP-Greg commented Feb 9, 2020

Sorry for being AFK for a bit.

People should be setting up their path to correctly load/run whatever requirements they have. I guess all the path cleaning could be removed...

Enabling msys2 could be an option, as automatically putting it a the front of the path may cause issues for people using other compilers.

I was kind of waiting to see if GH actually made a quick decision about whether to add MSYS2 to their images...

@MSP-Greg
Copy link
Collaborator

MSP-Greg commented Feb 9, 2020

@slonopotamus

It was just really unexpected and not very trivial

Apologies for that.

@eregon
Copy link
Member

eregon commented Feb 9, 2020

Enabling msys2 could be an option, as automatically putting it a the front of the path may cause issues for people using other compilers.

What do you mean by "Enabling msys2" ? Currently, it's added at the front of PATH.

@eregon
Copy link
Member

eregon commented Feb 9, 2020

A first step I did in #20 is to show which entries are removed from PATH:

Entries removed from PATH to avoid conflicts with MSYS2 and Ruby:
C:\hostedtoolcache\windows\Ruby\2.5.7\x64\bin
C:\ProgramData\Chocolatey\bin
C:\Program Files\Git\mingw64\bin
C:\Program Files\CMake\bin
C:\Strawberry\c\bin
C:\Strawberry\perl\site\bin
C:\Strawberry\perl\bin
C:\Program Files\OpenSSL\bin

@MSP-Greg
Copy link
Collaborator

MSP-Greg commented Feb 9, 2020

One possible messy item is 7-Zip, which is on the path in C:\ProgramData\Chocolatey\bin. It's also installed at C:\Program Files\7-Zip. I add that to path in actions-ruby just so 7z is available...

@MSP-Greg
Copy link
Collaborator

MSP-Greg commented Feb 9, 2020

What do you mean by "Enabling msys2" ? Currently, it's added at the front of PATH.

Whether to add a windows specific option to add MSYS2 to the path, and leave it out by default.

It's messy, because the newer Rubies (2.4+) will find it at C:\msys64 when one requires devkit, but the older ones (2.2 & 2.3) will not. I've got an issue in Ruby about adding a require 'devkit' to mkmf.rb, which would simplify a lot of things, but doesn't help older versions.

@eregon
Copy link
Member

eregon commented Feb 9, 2020

eregon added a commit that referenced this issue Feb 9, 2020
* This might lead to more conflicts, but the flip side of a setup action
  removing capabilities seems worse and unexpected.
* See #19
@eregon
Copy link
Member

eregon commented Feb 9, 2020

I released 1.15.0 with the fix, it should work now.
@slonopotamus @MSP-Greg Thanks for reporting the issue and helping with finding a solution.

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

4 participants
@slonopotamus @eregon @MSP-Greg and others