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

Add ridk option to disable ridk env variables #562

Closed
wants to merge 1 commit into from

Conversation

k0kubun
Copy link
Member

@k0kubun k0kubun commented Jan 16, 2024

I tried to use ruby/setup-ruby to set up PATH in mswin build on ruby/ruby ruby/ruby#9566, but ridk environment variables added by it seem to break the build:

Run ../src/win32/configure.bat --disable-install-doc --with-opt-dir=C:/vcpkg/installed/x64-windows
../src/win32/setup.mak:3: *** missing separator.  Stop.
NMAKE : fatal error U1077: 'C:\msys64\usr\bin\make.exe' : return code '0x2'
Stop.

Since only PATH is needed for the mswin build and adding ridk environment variables only breaks it, I'd like to add an option to disable those environment variables. This PR proposes to add it as ridk: none.

@MSP-Greg
Copy link
Collaborator

MSP-Greg commented Jan 17, 2024

@k0kubun

Question - Something similar I've thought about would be an input like win_platform, it's main use would be
win_platform: mswin. You could load any Windows Ruby (all but mswin are MSY2 gcc), but it would use the setup that is used for the mswin build?

It would add all the ENV settings needed for an mswin build, and also install the pre-compiled MSFT/vpkg archive.

Several ruby org 'extension' repos are using the same logic when they run CI with mswin.

EDIT: we could also add win_platform: ucrt, which would setup MSYS2 'UCRT64' regardless of the Ruby installed...

@k0kubun
Copy link
Member Author

k0kubun commented Jan 17, 2024

I'm not interested in setting up environment variables for mswin build using ruby/setup-ruby because Ruby committers don't have write access to ruby/setup-ruby and doing so in ruby/setup-ruby only makes it harder for us to fix broken builds like this time. As long as your proposed solution also allows win_platform: none, I'm fine with that too. I just need a way to modify only PATH.

@MSP-Greg
Copy link
Collaborator

because Ruby committers don't have write access to ruby/setup-ruby

Well, it would seem that a discussion with @eregon might be a good idea.

I just need a way to modify only PATH

Not sure what you mean. Do you only want ruby/setup-ruby to add the selected Ruby to PATH?

@k0kubun
Copy link
Member Author

k0kubun commented Jan 17, 2024

Do you only want ruby/setup-ruby to add the selected Ruby to PATH?

Exactly. I want to write:

      - uses: ruby/setup-ruby@v1
        with:
          ruby-version: '2.7'
          bundler: none
          ridk: none # or win_platform: none

and let the action execute only "Modifying Path" (and "Print Ruby version"). For our use case, we want no other environment variables.

We currently hard-code C:\hostedtoolcache\windows\Ruby\2.7.8\x64\bin to choose the Ruby version. It's fine for Ruby 2.7 since it's past EOL, but once we upgrade the version to, say, 3.2.2, it would be broken when 3.2.3 comes out. I want to resolve a version cached by GitHub Actions using this.

@MSP-Greg
Copy link
Collaborator

I believe PR #563 adds the functionality you've described.

See https://github.com/MSP-Greg/setup-ruby/actions/runs/7550784437/job/20556972770#step:3:17

@k0kubun
Copy link
Member Author

k0kubun commented Jan 17, 2024

That looks good. Thank you for working on that. Could you merge and/or release that one?

@MSP-Greg
Copy link
Collaborator

You're welcome.

As you know, ruby/setup-ruby installs both MSYS2 toolchains and packages and also MSFT/vcpkg packages. At some point, I'd like to add options mingw, ucrt, and mswin to the input.

As it is now, it works well for 'normal' CI, where the Ruby that's being installed/activated has the build tools needed to compile extension code for itself (eg Nokogiri, Puma, Ruby bundled extension gems, etc).

But, if one wants to build Ruby, one has to install a Ruby that matches the one being built, which is the problem you're having.

JFYI, ruby-loco uses ruby/setup-ruby for the build tools and package installation. I figured I should 'walk the talk', and it's a good test of ruby/setup-ruby. Sorry for mentioning 'third party software'...

Could you merge and/or release that one?

I don't have commit rights. I'm sure @eregon can review it soon.

@eregon
Copy link
Member

eregon commented Jan 17, 2024

As you know, ruby/setup-ruby installs both MSYS2 toolchains and packages and also MSFT/vcpkg packages. At some point, I'd like to add options mingw, ucrt, and mswin to the input.

Does it make sense to pick a subset of those in practice?
I guess it's not a good idea to mix e.g. mswin and mingw stuff, right?

Maybe something like extras: all|none|some-specific-ones could be good, but we would need to understand which combinations are useful then.

@eregon
Copy link
Member

eregon commented Jan 17, 2024

because Ruby committers don't have write access to ruby/setup-ruby

We need a proper review for PRs to setup-ruby (e.g. no direct push), because e.g. any input added can basically never be removed without bumping the major version and that's a high cost.

@MSP-Greg
Copy link
Collaborator

MSP-Greg commented Jan 17, 2024

Does it make sense to pick a subset of those in practice?

The code to allow one or the code to allow all is not much different. But, it will take time I don't have right now. So, just setup none.

I guess it's not a good idea to mix e.g. mswin and mingw stuff, right?

It's not quite that simple. The Windows files installed here can be categorized as 'bash tools', 'build tools/compilers' and 'packages'. 'bash tools' are needed/used by all Windows Ruby platforms (RUBY_PLATFORM), 'build tools/compilers' and 'packages' are platform dependent.

Maybe something like extras: all|none|some-specific-ones could be good, but we would need to understand which combinations are useful then.

extras: is about as vague as it gets. I think win-ruby-platform-override would be a better choice, as it's pretty clear as to what it does? Or, win means Windows, ruby-platform is RUBY_PLATFORM, and override is dangerous.

@eregon eregon closed this in #563 Jan 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants