Vendor `uri` library #7460

deivid-rodriguez · 2019-11-29T11:49:14Z

What was the end-user problem that led to this PR?

The problem was that after the gemification of the uri library (which will happen in ruby 2.7), bundler will activate the default version of the new library inside its own bundler/setup code. That means users won't be able to specify the version of the library they want/need to use in their own Gemfiles.

What was your diagnosis of the problem?

My diagnosis was that we should avoid using uri inside bundler/setup code.

What is your fix for the problem, implemented in this PR?

My fix is to vendor the uri library, like we did with fileutils.

deivid-rodriguez · 2019-11-30T17:45:14Z

This is now green against ruby-head, but this is still WIP since I want to clean things up, review some stuff, and extract some unrelated improvements to a separate PR.

7464: Improve rubygems switcher r=deivid-rodriguez a=deivid-rodriguez ### What was the end-user problem that led to this PR? The problem was we have some code in our tests to make sure that the `ENV["RGV"]` variable we use to set the rubygems version we want to test bundler against is always respected, including every subprocess our tests start. However: * The code was overly complicated. * It didn't support custom branches, for example, `RGV=lazily_load_uri`. This is a feature I find very useful when a PR you're working on also need some changes in the rubygems side, like it happened to me at #7460. ### What was your diagnosis of the problem? My diagnosis was that all the code needs to do is: * Set up the location of the rubygems code we'll test bundler against, be it a path, a branch, or a released version. * Modify `RUBYOPT` to include that location in the LOAD_PATH, so that `rubygems` is always loaded from there instead of the system's version. ### What is your fix for the problem, implemented in this PR? My fix is to do just that, remove all the stuff that wasn't needed, and do a bit of renaming to improve readability. Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>

3005: Lazily load `uri` r=deivid-rodriguez a=deivid-rodriguez # Description: The `uri` library will be a default gem on ruby 2.7. Because of this, requiring `uri` at the top of this file will cause the default `uri` gem to be activated during `bundler/setup`, and that will result in gem activation conflicts. So, it's better to lazily load `uri` here. I'm using this patch in rubygems/bundler#7460 to get bundler specs passing against ruby 2.7. # Tasks: - [x] Describe the problem / feature - [ ] Write tests - [x] Write code to solve the problem - [ ] Get code review from coworkers / friends I will abide by the [code of conduct](https://github.com/rubygems/rubygems/blob/master/CODE_OF_CONDUCT.md). Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>

3017: Refactor remote fetcher r=bronzdoc a=deivid-rodriguez # Description: In rubygems/bundler#7460 I'm vendoring the `uri` library inside `bundler` to add support for using the `uri` version inside Gemfile's just like any other gem. However, that was not the only thing needed since `bundler/setup` also touches some code from this class in rubygems. So the initial approach I took was to duplicate all the code inside bundler, replacing all reference to `URI` with `Bundler::URI` (the vendored namespace). Like in rubygems/bundler@3bcc579. However, this is ugly and hard to maintain so in this PR I'm refactoring this class so that it is enough to override only one method in the `bundler` subclass, like this: https://github.com/bundler/bundler/blob/aa3a25d30383a16e002dd94c209a6078746204a7/lib/bundler/gem_remote_fetcher.rb # Tasks: - [x] Describe the problem / feature - [ ] Write tests - [x] Write code to solve the problem - [ ] Get code review from coworkers / friends I will abide by the [code of conduct](https://github.com/rubygems/rubygems/blob/master/CODE_OF_CONDUCT.md). Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>

7471: Vendor latest thor's master r=deivid-rodriguez a=deivid-rodriguez ### What was the end-user problem that led to this PR? The problem was that I need to pull some changes from latest `thor` master to avoid loading the `uri` library for #7460. ### What is your fix for the problem, implemented in this PR? My fix is to run `bin/rake vendor:thor[master]` and commit changes. Not that I used an `automatiek` version including segiddins/automatiek#7 that avoids some false positive when replacing namespaces. Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>

7473: Cleanup some unnecessary code r=deivid-rodriguez a=deivid-rodriguez ### What was the end-user problem that led to this PR? The problem was #7460 is very big so I want to extract these changes to a separate PR, so that we're more aware of them. ### What was your diagnosis of the problem? My diagnosis was that this code can be removed. In particular, in the `GemRemoteFetcher` class there was the following comment https://github.com/bundler/bundler/blob/25595896eb0f8dfd004d941093bf1d8f4a39aeeb/lib/bundler/gem_remote_fetcher.rb#L9-L10 After having a look, I think the comment would make sense if where it says "gemstash", it actually meant "bundler". That would make sense to me since this is about fetching remote specs, so I would assume it's the client side running it. Assuming this is the correct interpretation, we can remove the code since our minimum supported rubygems version is 2.5.2, and this code was included in rubygems in 2.5.0. ### What is your fix for the problem, implemented in this PR? My fix is to remove the `GemRemoteFetcher` class, plus simplify other related code. Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>

We vendorize it as a dependency of `net-http-persistent`, so usages inside `net-http-persistent` get automatically replaced by `automatiek`.

deivid-rodriguez · 2019-12-08T20:08:44Z

This should be ready now, and it gets build against ruby-head back to green.

Only concern is that I had to change some unit tests for methods that now return or receive Bundler::URI's instead of URI's. I think this should be fine since these are internal methods not meant to be used by end users (I believe).

3017: Refactor remote fetcher r=hsbt a=deivid-rodriguez # Description: In rubygems/bundler#7460 I'm vendoring the `uri` library inside `bundler` to add support for using the `uri` version inside Gemfile's just like any other gem. However, that was not the only thing needed since `bundler/setup` also touches some code from this class in rubygems. So the initial approach I took was to duplicate all the code inside bundler, replacing all reference to `URI` with `Bundler::URI` (the vendored namespace). Like in rubygems/bundler@3bcc579. However, this is ugly and hard to maintain so in this PR I'm refactoring this class so that it is enough to override only one method in the `bundler` subclass, like this: https://github.com/bundler/bundler/blob/aa3a25d30383a16e002dd94c209a6078746204a7/lib/bundler/gem_remote_fetcher.rb # Tasks: - [x] Describe the problem / feature - [ ] Write tests - [x] Write code to solve the problem - [ ] Get code review from coworkers / friends I will abide by the [code of conduct](https://github.com/rubygems/rubygems/blob/master/CODE_OF_CONDUCT.md). Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>

deivid-rodriguez · 2019-12-12T16:43:17Z

lib/bundler/cli.rb

@@ -784,7 +784,7 @@ def warn_on_outdated_bundler
      return unless SharedHelpers.md5_available?

      latest = Fetcher::CompactIndex.
-               new(nil, Source::Rubygems::Remote.new(URI("https://rubygems.org")), nil).
+               new(nil, Source::Rubygems::Remote.new(Bundler::URI("https://rubygems.org")), nil).


I don't think all of these changes in lib are actually necessary because the Bundler::URI constant will beat URI anyways, but maybe they make it more explicit that we're using a vendored version of the uri gem?

I think we should replace vendored fileutils and uri to the minimum code like yaml_serializer in the next version.

But there is no enough time to release Bundler 2.1. We can try it Bundler 2.2 or Bundler 3.

Thanks, I agree and also started some work to be able to vendor only parts of libraries while still automating the process with automatiek 👍

deivid-rodriguez · 2019-12-13T10:06:49Z

@bundlerbot r=hsbt

7460: Vendor `uri` library r=hsbt a=deivid-rodriguez ### What was the end-user problem that led to this PR? The problem was that after the gemification of the `uri` library (which will happen in ruby 2.7), `bundler` will activate the default version of the new library inside its own `bundler/setup` code. That means users won't be able to specify the version of the library they want/need to use in their own Gemfiles. ### What was your diagnosis of the problem? My diagnosis was that we should avoid using `uri` inside `bundler/setup` code. ### What is your fix for the problem, implemented in this PR? My fix is to vendor the `uri` library, like we did with `fileutils`. Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>

3005: Lazily load `uri` r=deivid-rodriguez a=deivid-rodriguez # Description: The `uri` library will be a default gem on ruby 2.7. Because of this, requiring `uri` at the top of this file will cause the default `uri` gem to be activated during `bundler/setup`, and that will result in gem activation conflicts. So, it's better to lazily load `uri` here. I'm using this patch in rubygems/bundler#7460 to get bundler specs passing against ruby 2.7. # Tasks: - [x] Describe the problem / feature - [ ] Write tests - [x] Write code to solve the problem - [ ] Get code review from coworkers / friends I will abide by the [code of conduct](https://github.com/rubygems/rubygems/blob/master/CODE_OF_CONDUCT.md). Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>

3017: Refactor remote fetcher r=hsbt a=deivid-rodriguez # Description: In rubygems/bundler#7460 I'm vendoring the `uri` library inside `bundler` to add support for using the `uri` version inside Gemfile's just like any other gem. However, that was not the only thing needed since `bundler/setup` also touches some code from this class in rubygems. So the initial approach I took was to duplicate all the code inside bundler, replacing all reference to `URI` with `Bundler::URI` (the vendored namespace). Like in rubygems/bundler@3bcc579. However, this is ugly and hard to maintain so in this PR I'm refactoring this class so that it is enough to override only one method in the `bundler` subclass, like this: https://github.com/bundler/bundler/blob/aa3a25d30383a16e002dd94c209a6078746204a7/lib/bundler/gem_remote_fetcher.rb # Tasks: - [x] Describe the problem / feature - [ ] Write tests - [x] Write code to solve the problem - [ ] Get code review from coworkers / friends I will abide by the [code of conduct](https://github.com/rubygems/rubygems/blob/master/CODE_OF_CONDUCT.md). Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>

ghost · 2019-12-13T10:45:40Z

Build succeeded

continuous-integration/travis-ci/push

7464: Improve rubygems switcher r=deivid-rodriguez a=deivid-rodriguez ### What was the end-user problem that led to this PR? The problem was we have some code in our tests to make sure that the `ENV["RGV"]` variable we use to set the rubygems version we want to test bundler against is always respected, including every subprocess our tests start. However: * The code was overly complicated. * It didn't support custom branches, for example, `RGV=lazily_load_uri`. This is a feature I find very useful when a PR you're working on also need some changes in the rubygems side, like it happened to me at #7460. ### What was your diagnosis of the problem? My diagnosis was that all the code needs to do is: * Set up the location of the rubygems code we'll test bundler against, be it a path, a branch, or a released version. * Modify `RUBYOPT` to include that location in the LOAD_PATH, so that `rubygems` is always loaded from there instead of the system's version. ### What is your fix for the problem, implemented in this PR? My fix is to do just that, remove all the stuff that wasn't needed, and do a bit of renaming to improve readability. Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net> (cherry picked from commit 0cb5192)

7471: Vendor latest thor's master r=deivid-rodriguez a=deivid-rodriguez ### What was the end-user problem that led to this PR? The problem was that I need to pull some changes from latest `thor` master to avoid loading the `uri` library for #7460. ### What is your fix for the problem, implemented in this PR? My fix is to run `bin/rake vendor:thor[master]` and commit changes. Not that I used an `automatiek` version including segiddins/automatiek#7 that avoids some false positive when replacing namespaces. Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net> (cherry picked from commit 2559589)

7473: Cleanup some unnecessary code r=deivid-rodriguez a=deivid-rodriguez ### What was the end-user problem that led to this PR? The problem was #7460 is very big so I want to extract these changes to a separate PR, so that we're more aware of them. ### What was your diagnosis of the problem? My diagnosis was that this code can be removed. In particular, in the `GemRemoteFetcher` class there was the following comment https://github.com/bundler/bundler/blob/25595896eb0f8dfd004d941093bf1d8f4a39aeeb/lib/bundler/gem_remote_fetcher.rb#L9-L10 After having a look, I think the comment would make sense if where it says "gemstash", it actually meant "bundler". That would make sense to me since this is about fetching remote specs, so I would assume it's the client side running it. Assuming this is the correct interpretation, we can remove the code since our minimum supported rubygems version is 2.5.2, and this code was included in rubygems in 2.5.0. ### What is your fix for the problem, implemented in this PR? My fix is to remove the `GemRemoteFetcher` class, plus simplify other related code. Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net> (cherry picked from commit bada03d)

7460: Vendor `uri` library r=hsbt a=deivid-rodriguez ### What was the end-user problem that led to this PR? The problem was that after the gemification of the `uri` library (which will happen in ruby 2.7), `bundler` will activate the default version of the new library inside its own `bundler/setup` code. That means users won't be able to specify the version of the library they want/need to use in their own Gemfiles. ### What was your diagnosis of the problem? My diagnosis was that we should avoid using `uri` inside `bundler/setup` code. ### What is your fix for the problem, implemented in this PR? My fix is to vendor the `uri` library, like we did with `fileutils`. Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net> (cherry picked from commit 6394536)

deivid-rodriguez force-pushed the vendorize_uri_library branch 2 times, most recently from 3ae1be7 to c1cfa16 Compare November 30, 2019 14:41

deivid-rodriguez mentioned this pull request Dec 1, 2019

Improve rubygems switcher #7464

Merged

deivid-rodriguez mentioned this pull request Dec 1, 2019

Lazily load uri rubygems/rubygems#3005

Merged

4 tasks

deivid-rodriguez force-pushed the vendorize_uri_library branch 4 times, most recently from f30d8d4 to 3e954ad Compare December 3, 2019 15:59

deivid-rodriguez changed the title ~~Vendorize uri library~~ Vendor uri library Dec 4, 2019

deivid-rodriguez mentioned this pull request Dec 4, 2019

Refactor remote fetcher rubygems/rubygems#3017

Merged

4 tasks

deivid-rodriguez force-pushed the vendorize_uri_library branch from af504fb to 65f0a75 Compare December 4, 2019 16:59

deivid-rodriguez force-pushed the vendorize_uri_library branch 7 times, most recently from dd2cde1 to bee2abc Compare December 6, 2019 18:18

deivid-rodriguez mentioned this pull request Dec 6, 2019

Vendor latest thor's master #7471

Merged

deivid-rodriguez mentioned this pull request Dec 6, 2019

Cleanup some unnecessary code #7473

Merged

deivid-rodriguez added 4 commits December 8, 2019 14:11

Lazily load uri

61b74ae

Remove unneeded to_s calls

fd38d08

Refactor stuff to not load URI if not needed

bfa3d1e

This require seems unneeded

0490f16

deivid-rodriguez force-pushed the vendorize_uri_library branch from bee2abc to fd686dd Compare December 8, 2019 13:16

deivid-rodriguez added 3 commits December 8, 2019 14:32

Vendorize "uri" library

8f80bda

We vendorize it as a dependency of `net-http-persistent`, so usages inside `net-http-persistent` get automatically replaced by `automatiek`.

Use vendorized version of uri library

97b7bbc

No longer need workarounds since our uri version is fixed

6a65f74

deivid-rodriguez force-pushed the vendorize_uri_library branch from fd686dd to 6a65f74 Compare December 8, 2019 13:32

hsbt self-requested a review December 9, 2019 00:42

deivid-rodriguez commented Dec 12, 2019

View reviewed changes

hsbt approved these changes Dec 13, 2019

View reviewed changes

ghost merged commit 6a65f74 into master Dec 13, 2019

ghost deleted the vendorize_uri_library branch December 13, 2019 10:45

deivid-rodriguez added this to the 2.1.0 milestone Dec 13, 2019

pocke mentioned this pull request Jun 19, 2020

Add "require 'uri'" for LSP soutaro/steep#156

Merged

rogerhu mentioned this pull request Mar 8, 2021

Fix Bundler v2 and Puma 5.x issues rubygems/gemstash#289

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vendor `uri` library #7460

Vendor `uri` library #7460

deivid-rodriguez commented Nov 29, 2019 •

edited

deivid-rodriguez commented Nov 30, 2019

deivid-rodriguez commented Dec 8, 2019

deivid-rodriguez Dec 12, 2019

hsbt Dec 13, 2019

deivid-rodriguez Dec 13, 2019

deivid-rodriguez commented Dec 13, 2019

ghost commented Dec 13, 2019

Vendor uri library #7460

Vendor uri library #7460

Conversation

deivid-rodriguez commented Nov 29, 2019 • edited

What was the end-user problem that led to this PR?

What was your diagnosis of the problem?

What is your fix for the problem, implemented in this PR?

deivid-rodriguez commented Nov 30, 2019

deivid-rodriguez commented Dec 8, 2019

deivid-rodriguez Dec 12, 2019

Choose a reason for hiding this comment

hsbt Dec 13, 2019

Choose a reason for hiding this comment

deivid-rodriguez Dec 13, 2019

Choose a reason for hiding this comment

deivid-rodriguez commented Dec 13, 2019

ghost commented Dec 13, 2019

Build succeeded

Vendor `uri` library #7460

Vendor `uri` library #7460

deivid-rodriguez commented Nov 29, 2019 •

edited