Update for new Ruby and Rails versions #499

BrianHawley · 2021-12-21T22:34:27Z

Test Ruby 2.7, 3.0, and 3.1, and Rails 6.1 and 7.0.
Exclude incompatible test matrix combinations.
Show Ruby deprecation warnings in tests where supported.
File.exists? is deprecated; use File.exist?.
URI.escape and URI.decode are deprecated.
Add frozen string literal comments.
Make sure constants are frozen.
Make sure that the code is frozen-string safe.
Adjust dev dependencies for Ruby 3.0+ compatibility.
Fix Ruby warnings in pdfkit_spec.rb
Comment out bad syntax in release-drafter action.

Fixes #498

BrianHawley · 2021-12-21T22:46:09Z

@serene are you still the maintainer?

I ran all the tests locally in all version combinations so I could make sure the deprecations were fixed. Once Travis runs, we should be able to see whether the config changes worked.

BrianHawley · 2022-01-06T20:25:36Z

Since Ruby 3.1.0 is out now, I updated the PR to include it in its tests. The tests pass locally with no warnings.

serene · 2022-01-06T23:39:43Z

I will add this to my list to review, but if others can put some eyes on it as well, I'd really appreciate it!

BrianHawley · 2022-01-07T17:27:17Z

lib/pdfkit/source.rb

@@ -38,11 +42,11 @@ def to_s
    private

    def shell_safe_url
-      url_needs_escaping? ? URI::escape(@source) : @source
+      url_needs_escaping? ? URI::DEFAULT_PARSER.escape(@source) : @source


This is a direct fix of the URI.escape deprecation, and has the same exact behavior, but my main concern is that I don't think this is the right escaping method.

URI.escape escapes a string for use in a URI as a query value. However, in this case, the string is being escaped for use in a shell command parameter, which doesn't really have the same requirements. For one thing, the percent-encoding that URI.escape generates might conflict with the %var environment variable syntax on Windows. There might be some other issues, but I haven't taken the time to check.

The change here has no effect on the existing behavior (except getting rid of the deprecation message), so it's probably the best to do here. However, in the long run, it's probably best to change to a more appropriate escaping method (in a separate PR).

BrianHawley · 2022-01-07T22:25:27Z

I notice that the Travis CI tests didn't run. Should we set up Github Actions tests?

serene · 2022-01-08T17:33:45Z

@BrianHawley if that's something you'd like to take on, that would be really great! Maybe as a separate PR?

BrianHawley · 2022-01-10T16:10:06Z

@serene yes, preferably as a separate PR. For one thing, I haven't done that before, so I'd have to take time to learn how :)

BrianHawley · 2022-01-24T16:21:21Z

I've been using a patched version of pdfkit with these changes for a month (used vendorer to automatically generate the patched version). Are we going to merge this soon and release a new version, so I can switch back to the regular gem?

thomasdarde · 2022-01-26T08:15:19Z

spec/spec_helper.rb

@@ -6,13 +8,15 @@
  add_filter 'spec/'
 end

+Warning[:deprecated] = true if defined?(Warning.[]=)


Nice PR @BrianHawley
What is the use of this line about deprecated ?

That makes Ruby deprecation warnings show up while running tests. I used those warnings to determine and fix any future compatibility issues, when I was running the tests locally against all supported versions. It's a good trick I use at work as well. The defined?(Warning.[]=) guard is because it only applies to Ruby 2.7+.

thomasdarde · 2022-01-26T08:16:58Z

Seems really nice, I did also use this version without any issues !

github-actions · 2022-03-30T00:34:39Z

This pull request has been marked as stale and will be automatically closed.

dedene · 2022-03-30T07:35:33Z

We are also using this version without any issues. Can't it be merged and released?

serene · 2022-03-30T13:55:09Z

I'm a little concerned that the build didn't run on this branch and merging it could cause issues for others.

leonyip · 2022-04-03T05:04:06Z

@serene is it possible that this repo had not migrated to travis-ci.com? https://blog.travis-ci.com/2021-05-07-orgshutdown

leonyip · 2022-04-03T18:03:17Z

@serene I just opened #502 for the Github Actions migration. This branch can be tested against that and should be able to merge. Thanks!

serene · 2022-04-04T14:08:01Z

@BrianHawley Can you pull in main to get the Github actions changes?

BrianHawley · 2022-04-04T16:12:44Z

@serene I pushed up a rebased version with the same test matrix changes applied to the Github Actions tests. The workflow needs approval before the tests will run.

BrianHawley · 2022-04-04T16:14:35Z

Cool, looks like the exclusions worked properly.

Looks like there was an internal error in one of the 2.5.9 steps that didn't affect the other 2.5.9 steps, and it's GA code that runs before the other parts of the matrix, so it seems to just be flaky. I don't know the GA capabilities well enough, but is there a way to rerun a step?

I just pushed up a useless commit to trigger the tests again.

BrianHawley · 2022-04-04T16:24:13Z

OK, that's not flaky. Let me check if another exclusion is needed.

serene · 2022-04-04T16:24:50Z

Should we even be supporting 2.5 anymore?

BrianHawley · 2022-04-04T16:26:39Z

I don't have a problem with leaving 2.5 support there. I mean, I don't use it, but others might need to. I'd rather keep it there long enough to figure out what the problem was. That failure was uninformative.

leonyip · 2022-04-04T16:27:19Z

@BrianHawley I sent a PR to your repo with the exclusions. It ran fine on my build.

BrianHawley · 2022-04-04T16:30:58Z

@leonyip I already made those exclusions, but I appreciate the PR. Still, it's not working in one case (which your PR also includes) with an uninformative error message, so I'm trying to figure out what's actually failing.

leonyip · 2022-04-04T16:36:56Z

Might have something to do with the bundler cache, can you omit the bundler-cache: true

BrianHawley · 2022-04-04T16:56:09Z

@serene I updated the default ruby version to the latest 2.6.x, and turned off the bundler cache. The workflow needs approval again.

I think that the method of selecting the Rails version isn't working well with the cache, probably because the Gemfile doesn't change even though its meaning does when that env var changes. A lot of other projects use the appraisal gem to resolve this kind of issue. We might consider switching to it here as well. (Not in this PR; possible future reference).

Update: Nope, turning off the bundler cache broke everything. What worked was updating rubygems and bundler.

BrianHawley · 2022-04-04T18:46:58Z

@serene new workflow attempt needs approving.

BrianHawley · 2022-04-04T19:09:34Z

I would like to squash this before merging. Last workflow approval, @serene :)

- Update default ruby version to latest 2.6 release. - Test Ruby 2.7, 3.0, and 3.1, and Rails 6.1 and 7.0. Use versions as strings to make sure 3.0 tests 3.0. - Use latest rubygems and bundler to fix the bundler cache. - Exclude incompatible test matrix combinations. - Show Ruby deprecation warnings in tests where supported. - File.exists? is deprecated; use File.exist?. - URI.escape and URI.decode are deprecated. - Add frozen string literal comments. - Make sure constants are frozen. - Make sure that the code is frozen-string safe. - Adjust dev dependencies for Ruby 3.0+ compatibility. - Fix Ruby warnings in pdfkit_spec.rb - Comment out bad syntax in release-drafter action. Fixes pdfkit#498

BrianHawley · 2022-04-04T19:26:22Z

@serene I had to change the Ruby version numbers to strings to make sure 3.0 tests 3.0.x, instead of 3.x (which becomes 3.1, which we already test). Could you approve the workflow again? Once this one passes, it should be good to merge.

BrianHawley force-pushed the fixes_498 branch from 02b9d07 to 63f4222 Compare December 21, 2021 22:56

BrianHawley force-pushed the fixes_498 branch from 63f4222 to c9c1448 Compare January 6, 2022 20:23

BrianHawley commented Jan 7, 2022

View reviewed changes

technicalpickles mentioned this pull request Jan 21, 2022

URI::escape and URI::decode are deprecated in Ruby 3 #500

Closed

thomasdarde reviewed Jan 26, 2022

View reviewed changes

github-actions bot added the no-pr-activity label Mar 30, 2022

serene removed the no-pr-activity label Mar 30, 2022

BrianHawley force-pushed the fixes_498 branch from c9c1448 to b06c5fd Compare April 4, 2022 16:11

BrianHawley force-pushed the fixes_498 branch from a0b0a81 to 4c48ec8 Compare April 4, 2022 19:13

BrianHawley force-pushed the fixes_498 branch from 4c48ec8 to 36ffbfb Compare April 4, 2022 19:25

serene merged commit 965c1bb into pdfkit:master Apr 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update for new Ruby and Rails versions #499

Update for new Ruby and Rails versions #499

BrianHawley commented Dec 21, 2021 •

edited

BrianHawley commented Dec 21, 2021 •

edited

BrianHawley commented Jan 6, 2022 •

edited

serene commented Jan 6, 2022

BrianHawley Jan 7, 2022 •

edited

BrianHawley commented Jan 7, 2022 •

edited

serene commented Jan 8, 2022

BrianHawley commented Jan 10, 2022

BrianHawley commented Jan 24, 2022

thomasdarde Jan 26, 2022

BrianHawley Jan 28, 2022 •

edited

thomasdarde commented Jan 26, 2022

github-actions bot commented Mar 30, 2022

dedene commented Mar 30, 2022

serene commented Mar 30, 2022

leonyip commented Apr 3, 2022

leonyip commented Apr 3, 2022

serene commented Apr 4, 2022

BrianHawley commented Apr 4, 2022 •

edited

BrianHawley commented Apr 4, 2022 •

edited

BrianHawley commented Apr 4, 2022

serene commented Apr 4, 2022

BrianHawley commented Apr 4, 2022 •

edited

leonyip commented Apr 4, 2022

BrianHawley commented Apr 4, 2022

leonyip commented Apr 4, 2022

BrianHawley commented Apr 4, 2022 •

edited

BrianHawley commented Apr 4, 2022 •

edited

BrianHawley commented Apr 4, 2022 •

edited

BrianHawley commented Apr 4, 2022 •

edited

Update for new Ruby and Rails versions #499

Update for new Ruby and Rails versions #499

Conversation

BrianHawley commented Dec 21, 2021 • edited

BrianHawley commented Dec 21, 2021 • edited

BrianHawley commented Jan 6, 2022 • edited

serene commented Jan 6, 2022

BrianHawley Jan 7, 2022 • edited

Choose a reason for hiding this comment

BrianHawley commented Jan 7, 2022 • edited

serene commented Jan 8, 2022

BrianHawley commented Jan 10, 2022

BrianHawley commented Jan 24, 2022

thomasdarde Jan 26, 2022

Choose a reason for hiding this comment

BrianHawley Jan 28, 2022 • edited

Choose a reason for hiding this comment

thomasdarde commented Jan 26, 2022

github-actions bot commented Mar 30, 2022

dedene commented Mar 30, 2022

serene commented Mar 30, 2022

leonyip commented Apr 3, 2022

leonyip commented Apr 3, 2022

serene commented Apr 4, 2022

BrianHawley commented Apr 4, 2022 • edited

BrianHawley commented Apr 4, 2022 • edited

BrianHawley commented Apr 4, 2022

serene commented Apr 4, 2022

BrianHawley commented Apr 4, 2022 • edited

leonyip commented Apr 4, 2022

BrianHawley commented Apr 4, 2022

leonyip commented Apr 4, 2022

BrianHawley commented Apr 4, 2022 • edited

BrianHawley commented Apr 4, 2022 • edited

BrianHawley commented Apr 4, 2022 • edited

BrianHawley commented Apr 4, 2022 • edited

BrianHawley commented Dec 21, 2021 •

edited

BrianHawley commented Dec 21, 2021 •

edited

BrianHawley commented Jan 6, 2022 •

edited

BrianHawley Jan 7, 2022 •

edited

BrianHawley commented Jan 7, 2022 •

edited

BrianHawley Jan 28, 2022 •

edited

BrianHawley commented Apr 4, 2022 •

edited

BrianHawley commented Apr 4, 2022 •

edited

BrianHawley commented Apr 4, 2022 •

edited

BrianHawley commented Apr 4, 2022 •

edited

BrianHawley commented Apr 4, 2022 •

edited

BrianHawley commented Apr 4, 2022 •

edited

BrianHawley commented Apr 4, 2022 •

edited