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

[Ruby 3.0] Replace obsolete URI.escape with alternatives #18646

Merged
merged 6 commits into from
May 14, 2021

Conversation

ainame
Copy link
Contributor

@ainame ainame commented May 5, 2021

Checklist

  • I've run bundle exec rspec from the root directory to see all new and existing tests pass
  • I've followed the fastlane code style and run bundle exec rubocop -a to ensure the code style is valid
  • I've read the Contribution Guidelines
  • I've updated the documentation if necessary.

Motivation and Context

As per #17931, I've been working on Ruby 3.0 support. Prior to this, I've fixed most deprecation warnings from Ruby 2.7 #18021. It now prevents PRs from making new warnings by checking them in CircleCI.

- run:
name: Check compatibility with Ruby 3.0
command: |
touch ~/test-reports/ruby_warnings.txt
# TODO remove 'grep -v ".bundle"' once 3rd party dependencies support Ruby 3.0
! cat ~/test-reports/ruby_warnings.txt | grep -v ".bundle" | grep -E "warning:\s.*(deprecated).*$" && echo "No deprecation message found."

However, there is still another type of deprecation warning introduced in Ruby 1.9; warning: URI.escape is obsolete. This message bypassed the above checkπŸ˜‚ I recognised this warning before but didn't know URI.escape was finally removed in Ruby 3.0 until I run fastlane on Ruby 3.0, which became possible after #18599.

This PR replace URI.escape with some alternatives. Some of them were fixed in this PR previously #16409 but it had some defects that the PR changed results. This PR also restores previous results.

You can read some history in blog posts; for example
https://docs.knapsackpro.com/2020/uri-escape-is-obsolete-percent-encoding-your-query-string

Description

There's no 100% equivalent to URI.escape. So we need to use alternatives case by case.

  1. URI.encode_www_form_component - can be used where you encode a part of URI query parameter
  2. URI.encode_www_form - can be used with query parameters in Array on Hash
  3. CGI.escape - similar to URI.encode_www_form_component but it follows CGI's spec
  4. Addressable::URI.encode - is 3rd party dependency (we've already adopted) but is the closest option to URI.escape

I applied 1 for places where we process query parameters and 3 for the rest of URLs and file paths.

Note that there is different behaviour between URI.escape and URI.encode_www_form_component especially for whitespaces. While URI.escape and Addressable::URI.escape can work well for file paths including whitespaces, URI.encode_www_form_component is only effective for "query parameters" really.

These are examples of differences among those APIs (except for CGI.escape).

[3] pry(main)> URI.escape 'a b'
(pry):3: warning: URI.escape is obsolete
=> "a%20b"
[4] pry(main)> URI.encode_www_form_component 'a b'
=> "a+b"
[5] pry(main)> Addressable::URI.encode('a b')
=> "a%20b"
[2] pry(main)> URI.escape('~/app/fastlane/screenshots/en_US/a 1.jpg')
(pry):2: warning: URI.escape is obsolete
=> "~/app/fastlane/screenshots/en_US/a%201.jpg"
[3] pry(main)> Addressable::URI.encode('~/app/fastlane/screenshots/en_US/a 1.jpg')
=> "~/app/fastlane/screenshots/en_US/a%201.jpg"
[4] pry(main)> URI.encode_www_form_component('~/app/fastlane/screenshots/en_US/a 1.jpg')
=> "%7E%2Fapp%2Ffastlane%2Fscreenshots%2Fen_US%2Fa+1.jpg"
[10] pry(main)> URI.escape('http://fastlane.tools/search?query=γ‚γ„γ†γˆγŠ')
(pry):10: warning: URI.escape is obsolete
=> "http://fastlane.tools/search?query=%E3%81%82%E3%81%84%E3%81%86%E3%81%88%E3%81%8A"
[11] pry(main)> URI.encode_www_form_component('http://fastlane.tools/search?query=γ‚γ„γ†γˆγŠ')
=> "http%3A%2F%2Ffastlane.tools%2Fsearch%3Fquery%3D%E3%81%82%E3%81%84%E3%81%86%E3%81%88%E3%81%8A"
[12] pry(main)> Addressable::URI.encode('http://fastlane.tools/search?query=γ‚γ„γ†γˆγŠ')
=> "http://fastlane.tools/search?query=%E3%81%82%E3%81%84%E3%81%86%E3%81%88%E3%81%8A"

Testing Steps

We can test the followings -

  • deliver/lib/assets/summary.html.erb - with deliver command with HTML preview (See if screenshots are correctly shown and linked)
  • frameit/lib/frameit/frame_downloader.rb - with fastlane frameit download_frames
  • fastlane/lib/fastlane/actions/xcodebuild.rb - with Fastfile below

Fastfile

platform :ios do
  desc "Push a new release build to the App Store"
  lane :release do
    app_store_connect_api_key(in_house: false)
    match
    increment_build_number
    
    ENV['XCODE_BUILD_PATH'] = 'build'
    xcarchive(
      sdk: 'iphoneos',
      scheme: 'FastlaneTest'
    )

    xcexport(
      export_path: File.join(ENV['XCODE_BUILD_PATH'], 'export'),
      export_options_plist: {
        method: 'app-store',
        uploadSymbols: true,
        manifest: {
          appURL: 'https://fastlane.tools/search?query=γƒ•γ‚‘γ‚Ήγƒˆγƒ¬γƒΌγƒ³',
          displayImageURL: 'https://fastlane.tools/γƒ•γ‚‘γ‚Ήγƒˆγƒ¬γƒΌγƒ³.jpg',
          fullSizeImageURL: 'https://fastlane.tools/γƒ•γ‚‘γ‚Ήγƒˆγƒ¬γƒΌγƒ³.jpg',
          assetPackManifestURL: 'https://fastlane.tools/γƒ•γ‚‘γ‚Ήγƒˆγƒ¬γƒΌγƒ³.plist',
        }
      }
    )
  end

Result - exportOption.plist (See if the URLs are escaped correctly)

``` manifest appURL https://fastlane.tools/search?query=%E3%83%95%E3%82%A1%E3%82%B9%E3%83%88%E3%83%AC%E3%83%BC%E3%83%B3 assetPackManifestURL https://fastlane.tools/%E3%83%95%E3%82%A1%E3%82%B9%E3%83%88%E3%83%AC%E3%83%BC%E3%83%B3.plist displayImageURL https://fastlane.tools/%E3%83%95%E3%82%A1%E3%82%B9%E3%83%88%E3%83%AC%E3%83%BC%E3%83%B3.jpg fullSizeImageURL https://fastlane.tools/%E3%83%95%E3%82%A1%E3%82%B9%E3%83%88%E3%83%AC%E3%83%BC%E3%83%B3.jpg method app-store teamID 2Q7H62SFG3 uploadSymbols ```

We can't really test them and might be good to consider deprecation/removal😞

@google-cla google-cla bot added the cla: yes label May 5, 2021
@ainame ainame force-pushed the fix-minor-deprecation branch 3 times, most recently from 08183ac to 3b86635 Compare May 6, 2021 06:08
@ainame ainame closed this May 7, 2021
@ainame ainame reopened this May 7, 2021
@ainame ainame marked this pull request as ready for review May 7, 2021 00:53
Comment on lines +110 to +111
! cat ~/test-reports/ruby_warnings.txt | grep -E "warning:\s.*(deprecated).*$" && echo "No deprecation message found."
! cat ~/test-reports/ruby_warnings.txt | grep -E "warning:\s.*(obsolete).*$" && echo "No obsolete message found."
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This covers "UI.escape is obsolete" message and 3rd party dependencies nowπŸ˜„

@@ -237,7 +237,7 @@
<div class="app-screenshot-row">

<% screenshots.each_with_index do |screenshot, index| %>
<a href="<%= URI.escape(screenshot.path) %>" target="_blank"><img class="app-screenshot" src="<%= render_relative_path(@export_path, URI.escape(screenshot.path)) %>" title="Screenshot #<%=index%> for <%=language%>"></a>
<a href="<%= render_relative_path(@export_path, Addressable::URI.encode(screenshot.path)) %>" target="_blank"><img class="app-screenshot" src="<%= render_relative_path(@export_path, Addressable::URI.encode(screenshot.path)) %>" title="Screenshot #<%=index%> for <%=language%>"></a>
Copy link
Contributor Author

@ainame ainame May 7, 2021

Choose a reason for hiding this comment

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

This link was actually brokenπŸ˜… I fixed it by adding render_relative_path

@ainame ainame force-pushed the fix-minor-deprecation branch 3 times, most recently from 5ede484 to 6fd103e Compare May 7, 2021 01:47
Comment on lines +181 to +188
name: brew update if needed # temporary solution for this https://discuss.circleci.com/t/macos-image-users-homebrew-brownout-2021-04-26/39872/2
command: |
# if the version is not 3.x.x, try brew update
ruby -e 'exit(1) if `brew -v` =~ /2\.\d+\.\d+/' ||\
brew update || \
# if brew update fails due to shallow clone, try unshallowing and then brew update again
git -C /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core fetch --unshallow && \
brew update
Copy link
Contributor Author

@ainame ainame May 7, 2021

Choose a reason for hiding this comment

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

https://discuss.circleci.com/t/macos-image-users-homebrew-brownout-2021-04-26/39872/2

Apparently, CircleCI provides both old and new macOS images so that some builds succeed with Homebrew v3 others fails with Homebrew v2😞 This way brew update will be called only when a build picks up an old macOS image that has Homebrew v2. This can fall back to unshallow command when the initial attempt of brew update fails.

Copy link
Member

Choose a reason for hiding this comment

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

This is insane πŸ˜› Love it ❀️

Copy link
Member

@rogerluan rogerluan left a comment

Choose a reason for hiding this comment

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

Nice explanation of the rationale behind your decisions in the PR description πŸ’ͺ

LGTM! πŸš€

Copy link
Collaborator

@minuscorp minuscorp left a comment

Choose a reason for hiding this comment

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

LGTMπŸš€ great work as always! You rock!❀️

@ainame
Copy link
Contributor Author

ainame commented May 10, 2021

Thank you both!

Copy link
Member

@joshdholtz joshdholtz left a comment

Choose a reason for hiding this comment

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

πŸ”₯ as always! Thank you πŸ₯°

Comment on lines +181 to +188
name: brew update if needed # temporary solution for this https://discuss.circleci.com/t/macos-image-users-homebrew-brownout-2021-04-26/39872/2
command: |
# if the version is not 3.x.x, try brew update
ruby -e 'exit(1) if `brew -v` =~ /2\.\d+\.\d+/' ||\
brew update || \
# if brew update fails due to shallow clone, try unshallowing and then brew update again
git -C /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core fetch --unshallow && \
brew update
Copy link
Member

Choose a reason for hiding this comment

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

This is insane πŸ˜› Love it ❀️

@fastlane-bot
Copy link

Congratulations! πŸŽ‰ This was released as part of fastlane 2.183.0 πŸš€

@fastlane fastlane locked and limited conversation to collaborators Jul 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants