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

Split ruby.yml, revise Actions, all OS's test from 2.3 thru master/head [changelog skip] #2114

Merged
merged 1 commit into from
Feb 13, 2020

Conversation

MSP-Greg
Copy link
Member

@MSP-Greg MSP-Greg commented Feb 12, 2020

Description

Revisions to Actions workflows.

  1. All OS's test on Ruby 2.3 thru master/head. Previously, many master/head builds were not fully tested. As currently set up, all are fully tested.

  2. Bundler is pre-installed on all builds, so it is no longer installed/updated. Because of that, 'frozen string' does not work in older Rubies. Added conditional steps to only add 'frozen string' where it will pass.

  3. There are several places in the GitHub UI where lists of jobs appear. I reformatted the string shown to hopefully make it not truncated in any of those places...

  4. All OS's (including Windows) can have Ruby 2.2 added, but it is currently failing. Not sure if it's code or tests. Another PR can fix that.

  5. Puma does compile and test successfully with a Windows mswin build, but changes are needed for it, as jaro_winkler won't compile on it, and it's a dependency for RuboCop.

Your checklist for this pull request

  • I have reviewed the guidelines for contributing to this repository.
  • I have added an entry to History.md if this PR fixes a bug or adds a feature. If it doesn't need an entry to HISTORY.md, I have added [changelog skip] the pull request title.
  • I have added appropriate tests if this PR fixes a bug or adds a feature.
  • My pull request is 100 lines added/removed or less so that it can be easily reviewed.
  • If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
  • If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
  • I have updated the documentation accordingly.
  • All new and existing tests passed, including Rubocop.

@dentarg
Copy link
Member

dentarg commented Feb 12, 2020

3. There are several places in the GitHub UI where lists of jobs appear. I reformatted the string shown to hopefully make it not truncated in any of those places...

👍 ⭐️

@nateberkopec
Copy link
Member

giphy

- os: windows-latest
ruby: 2.3
os: [ ubuntu-18.04, macos ]
ruby: [ 2.3, 2.4, 2.5, 2.6, 2.7, ruby-head ]
Copy link
Member

Choose a reason for hiding this comment

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

So, does this add ruby-head as a must-pass? Is that a good idea?

Copy link
Member Author

@MSP-Greg MSP-Greg Feb 12, 2020

Choose a reason for hiding this comment

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

Is that a good idea?

What's Don think? You don't really want to get me going on Ruby master builds, but...

Without pointing fingers, some of the previous builds used in CI were not fully tested, especially with the main MRI test suite 'test-all'. So, a daily cron job would run and create a build based on a commit that didn't pass ruby/ruby CI.

All the builds used with current actions are fully tested. If a build doesn't pass CI, the last passing build will be kept in place for CI use.

Hence, I think it's a risk worth taking. If it breaks, ping me. JFYI, that's what I've been working on for while, Windows builds (converting to Actions, adding mswin, etc) and custom actions scripts.

does this add ruby-head as a must-pass?

Due to the Actions UI, etc, there isn't a worthwhile way to do 'allow failure'.

EDIT: Puma has been testing against Windows master for a while. Not something one keeps track of, but do you recall times when it failed and the other Windows builds did not?

I do most of my local work with master, and I generate my doc site with it. There's a lot gems used for YARD and all the different docs formats on all the repos doc'd. I also use more gems for my code to hit the GitHub API and update the main page.

There have been a few times when it failed. First, RubyGems screwed up the Windows binstubs. I then added tests for them. Secondly, they converted RDoc to use Ripper, and some of those changes broke YARD's parsing. Otherwise, nothing else comes to mind...

Copy link
Member

Choose a reason for hiding this comment

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

Oh, interesting. So it's not really ruby-head, it's actually "the last ruby trunk build that passed it's own CI?"

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct. If you can make up a good abbreviation for that, be happy to use it.

Since ruby/ruby switched to git, trunk is no more, although it might be an alias just because...

Copy link
Member

Choose a reason for hiding this comment

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

Cool. I’m still not sure I want to make uncaught bugs on Ruby head crash our build and make it fail until they’re fixed...

Copy link
Member Author

@MSP-Greg MSP-Greg Feb 13, 2020

Choose a reason for hiding this comment

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

How about we call it 'master'?

I want to make uncaught bugs on Ruby head crash

I think that's unlikely. It would certainly be a good idea to test master sometime in Q4. But if you're going to do that, might as well go year round.

I've been building/using master for 3+ years, and 'uncaught bugs' are very rare. Puma is an important gem, and if it exposes a bug in master, isn't it better that they be identified before a Ruby release in December?

In the unlikely event that it does cause CI failures here, it's only two line of code to change...

EDIT: I forgot to mention something that should be checked against (breaking change), but I don't think it's an issue with Puma, see:
https://www.ruby-lang.org/en/news/2019/12/12/separation-of-positional-and-keyword-arguments-in-ruby-3-0/

Copy link
Member

Choose a reason for hiding this comment

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

We'll try it, but I reserve the right to rollback 😆

Copy link
Member Author

Choose a reason for hiding this comment

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

Please ping me first on any failures. Thanks.

@MSP-Greg
Copy link
Member Author

MSP-Greg commented Feb 13, 2020

JFYI, the following are the Ruby 2.2 failures on macOS & Ubuntu. On Windows, there is code in the extconf.rb file (append_cflags) that won't work with mkmf.rb from 2.2.

  2) Failure:
TestLauncher#test_dependencies_and_files_to_require_after_prune_is_correctly_built_with_extra_deps [/home/runner/work/puma/puma/test/test_launcher.rb:30]:
Expected /puma\/lib$/ to match # encoding: US-ASCII
#    valid: true
"/home/runner/work/puma/extensions/x86_64-linux/2.2.0-static/puma-4.3.1".

  3) Failure:
TestLauncher#test_dependencies_and_files_to_require_after_prune_is_correctly_built_for_no_extra_deps [/home/runner/work/puma/puma/test/test_launcher.rb:14]:
Expected /puma\/lib$/ to match # encoding: US-ASCII
#    valid: true
"/home/runner/work/puma/extensions/x86_64-linux/2.2.0-static/puma-4.3.1".

@nateberkopec nateberkopec merged commit 71bcd84 into puma:master Feb 13, 2020
@MSP-Greg MSP-Greg deleted the actions-2 branch February 13, 2020 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants