Skip to content

Update Actions #432

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.

Sign up for GitHub

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

Merged
eregon merged 2 commits into from
Feb 17, 2020
Merged

Update Actions #432

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
47b3454
Update Actions
MSP-Greg Feb 12, 2020
fcafe24
appveyor.yml - remove 64 bit Rubies, they are now tested in Actions
MSP-Greg Feb 17, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 13 additions & 7 deletions .github/workflows/macos.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,17 @@ on: [push, pull_request]
jobs:
build:
runs-on: macos-latest
strategy:
fail-fast: false
matrix:
ruby: [ 2.7, 2.6, 2.5, 2.4 ]
steps:
- uses: actions/checkout@master
- name: Install dependencies
run: |
gem install bundler --no-document
bundle install
- name: Run test
run: rake
- uses: actions/checkout@v2
- name: Set up Ruby
uses: ruby/setup-ruby@v1
with:
ruby-version: ${{ matrix.ruby }}
- name: Install dependencies
run: bundle install
- name: Run test
run: rake
27 changes: 13 additions & 14 deletions .github/workflows/ubuntu.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,19 +6,18 @@ jobs:
build:
runs-on: ubuntu-latest
strategy:
fail-fast: false
matrix:
ruby: [ '2.6', '2.5', '2.4', 'ruby-head']
ruby: [ ruby-head, 2.7, 2.6, 2.5, 2.4 ]
steps:
- name: Install libraries
run: sudo apt install haveged
- uses: actions/checkout@master
- name: Set up Ruby
uses: ruby/setup-ruby@v1
with:
ruby-version: ${{ matrix.ruby }}
- name: Install dependencies
run: |
gem install bundler --no-document
bundle install
- name: Run test
run: rake
- name: Install libraries
run: sudo apt install haveged
- uses: actions/checkout@v2
- name: Set up Ruby
uses: ruby/setup-ruby@v1
with:
ruby-version: ${{ matrix.ruby }}
- name: Install dependencies
run: bundle install
- name: Run test
run: rake

@eregon eregon Feb 16, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The indentation changes here make it harder to read the diff, could you keep the original indentation?

@MSP-Greg MSP-Greg Feb 16, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I could, but it was my impression that proper yaml should have array members indented...

@eregon eregon Feb 16, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah, I'm not sure if there is a recommendation on that, I think both styles were used in .travis.yml back then.
I personally prefer the lesser indented variant, so it's only 4-spaces indented for steps (6 starts to feel quite deep already).

@MSP-Greg MSP-Greg Feb 16, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Re indenting, not an issue I feel strongly about. But, since I changed the indents in .github/workflows/ubuntu-jruby.yml, knowing the preference affects what I do with the merge conflict.

My tired old eyes prefer the indented style...

34 changes: 18 additions & 16 deletions .github/workflows/windows.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,20 +8,22 @@ jobs:
strategy:
fail-fast: false
matrix:
ruby: [ '2.6.x', '2.5.x', '2.4.x' ]
ruby: [ mingw, mswin, 2.7, 2.6, 2.5, 2.4 ]
steps:
- uses: actions/checkout@master
- name: Set up Ruby
uses: actions/setup-ruby@v1
with:
ruby-version: ${{ matrix.ruby }}
- name: Update MSYS2
uses: MSP-Greg/msys2-action@master
with:
base: update
- name: Install dependencies
run: |
gem update --system --no-document --conservative
bundle install
- name: Run test
run: rake
- uses: actions/checkout@v2
- name: Set up Ruby
uses: MSP-Greg/actions-ruby@v1

@hsbt hsbt Feb 12, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why don't you use ruby/setup-ruby?

@MSP-Greg MSP-Greg Feb 12, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

  1. Can't update MSYS2, etc.
  2. No mswin build
  3. Both mswin & mingw builds are fully tested. Last time I checked, the MinGW build that ruby/setup-ruby uses doesn't run test-all.

@eregon eregon Feb 13, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Re 1., how about a setup-msys2 or so action doing just that (and nothing on other OS)? Maybe https://github.com/MSP-Greg/msys2-action already does that?
Re 2., fair point, I wouldn't be against adding the possibility to use a mswin build in ruby/setup-ruby. Can you open an issue at https://github.com/ruby/setup-ruby if you think it's important to test on mswin? I know little about Windows, but I thought most people use RubyInstaller (and I'd think extremely few do their own builds).
Re 3., doesn't the RubyInstaller-head build run test-all? If not, can you open an issue for that on https://github.com/oneclick/rubyinstaller2?

I see you used ruby/setup-ruby for non-Windows workflows, so I think this change is fine, since anyway there are Windows-specific steps in there. But it would be good to make it available for cases which can share a workflow for all 3 OS.

@MSP-Greg MSP-Greg Feb 13, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm trying to deprecate msys2-action and replace it with actions-ruby, which I could rename setup-ruby.

mswin build - ruby/ruby tests against it. So, as recently happened with ruby/openssl, mingw passed there, but when they pushed the changes to ruby/ruby, mswin failed. By having a mswin build available for CI, that doesn't need to happen.

At present, mswin builds are not often used. But, I know of one company using an embedded Ruby based on RI2, and they're having issues with the fact that their app builds with VS 2019 and the RI2 builds use a different msvc runtime (the one used by MSYS2 tools). So, there is reason to build/test against mswin/msvc.

doesn't the RubyInstaller-head build run test-all?

See below. I'm not sure about spec tests, they may run when it hits RI2, not sure.

https://github.com/oneclick/rubyinstaller2-packages/blob/63699f6b8155435cbb499f60de6e534c107fe9bd/mingw-w64-ruby-head/PKGBUILD#L65-L71

Simply put, my shit works, or, if it occasionally doesn't, it gets fixed quickly. ruby-loco has been used in CI for almost three years in a lot of repos. The one time I recall it breaking was over the RubyGems binstub issue, and I added tests for that. I also actively encourage repos to test with ruby master on all platforms, which benefits ruby/ruby...

EDIT: FWIW, I use ruby-loco mingw every time I update https://msp-greg.github.io/, and any work I do on PR's I start with ruby-loco. I 'walk what I talk'...

@eregon eregon Feb 16, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I've given a few more thoughts about this.
I think your 3 points are all valid points to use your action here.

  1. is part of package management, I wouldn't want to have that in setup-ruby (Integrate system operations into setup-ruby setup-ruby#21)
  2. For that to work, one needs a separate windows workflow/job, because mswin, mingw as Ruby versions just wouldn't make sense on Ubuntu & macOS. If it's a separate workflow/job then it's also easy to simply switch to your action in the windows workflow/job.
  3. We should resolve that in RubyInstaller, good catch.

I think it's good to use your action for fine control like testing on both mingw & mswin.

I'm trying to deprecate msys2-action and replace it with actions-ruby, which I could rename setup-ruby.

I think something with windows, mswin or mingw in the action name would be helpful to clarify it's meant for Ruby on Windows. Another setup-ruby would be confusing because it wouldn't be a superset of ruby/setup-ruby.

@MSP-Greg MSP-Greg Feb 16, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Re the name, how about 'setup-ruby-win'? I think that would be clear to most people.

Re RubyInstaller vs ruby-loco mingw, I've set up ruby-loco to test against a master branch. All test suites are run against install, they're isolated (so all run), and logic is there to determine if any fail. The mswin build is currently building in a more traditional way (like ruby/ruby CI), but I'm converting it over to match the mingw build. A lot of repos have been using it for quite a while, including RubyGems.

Off topic, but stepping back, should the Ruby organization support Ruby Windows binaries itself? Should the 'devkit' be a repo under ruby? Should ruby/ruby support dll manifests in Windows binaries? Etc, etc...

@eregon eregon Feb 16, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

setup-ruby-win sounds good to me.

I'd be OK to add mingw/mswin builds built by ruby-loco in ruby/setup-ruby, if it doesn't add much complexity to the action, PR welcome.

@eregon eregon Feb 22, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I filed oneclick/rubyinstaller2#169

with:
ruby-version: ${{ matrix.ruby }}
base: update
- name: Install dependencies
run: bundle install
- name: Run test
if: matrix.ruby != 'mswin'
run: rake
- name: Run test
if: matrix.ruby == 'mswin'
shell: cmd
run: |
call "%VCVARS%"
rake
2 changes: 0 additions & 2 deletions appveyor.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,4 @@ deploy: off
environment:
matrix:
- ruby_version: "24"
- ruby_version: "24-x64"
- ruby_version: "25"
- ruby_version: "25-x64"