Skip to content
This repository has been archived by the owner. It is now read-only.

Vendor uri library #7460

Merged
7 commits merged into from Dec 13, 2019
Merged

Vendor uri library #7460

7 commits merged into from Dec 13, 2019

Conversation

deivid-rodriguez
Copy link
Member

@deivid-rodriguez deivid-rodriguez commented Nov 29, 2019

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 deivid-rodriguez force-pushed the vendorize_uri_library branch 2 times, most recently from 3ae1be7 to c1cfa16 Compare Nov 30, 2019
@deivid-rodriguez
Copy link
Member Author

deivid-rodriguez commented Nov 30, 2019

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.

ghost pushed a commit that referenced this issue Dec 1, 2019
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>
ghost pushed a commit to rubygems/rubygems that referenced this issue Dec 2, 2019
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>
@deivid-rodriguez deivid-rodriguez force-pushed the vendorize_uri_library branch 4 times, most recently from f30d8d4 to 3e954ad Compare Dec 3, 2019
@deivid-rodriguez deivid-rodriguez changed the title Vendorize uri library Vendor uri library Dec 4, 2019
ghost pushed a commit to rubygems/rubygems that referenced this issue Dec 6, 2019
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>
@deivid-rodriguez deivid-rodriguez force-pushed the vendorize_uri_library branch 7 times, most recently from dd2cde1 to bee2abc Compare Dec 6, 2019
ghost pushed a commit that referenced this issue Dec 6, 2019
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>
ghost pushed a commit that referenced this issue Dec 8, 2019
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
Copy link
Member Author

deivid-rodriguez commented Dec 8, 2019

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).

@hsbt hsbt self-requested a review Dec 9, 2019
ghost pushed a commit to rubygems/rubygems that referenced this issue Dec 9, 2019
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>
@@ -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).
Copy link
Member Author

@deivid-rodriguez deivid-rodriguez Dec 12, 2019

Choose a reason for hiding this comment

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

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?

Copy link
Member

@hsbt hsbt Dec 13, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@deivid-rodriguez deivid-rodriguez Dec 13, 2019

Choose a reason for hiding this comment

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

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

hsbt
hsbt approved these changes Dec 13, 2019
@deivid-rodriguez
Copy link
Member Author

deivid-rodriguez commented Dec 13, 2019

@bundlerbot r=hsbt

ghost pushed a commit that referenced this issue Dec 13, 2019
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>
hsbt pushed a commit to rubygems/rubygems that referenced this issue Dec 13, 2019
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>
hsbt pushed a commit to rubygems/rubygems that referenced this issue Dec 13, 2019
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
Copy link

ghost commented Dec 13, 2019

Build succeeded

@ghost ghost merged commit 6a65f74 into master Dec 13, 2019
3 checks passed
@ghost ghost deleted the vendorize_uri_library branch Dec 13, 2019
@deivid-rodriguez deivid-rodriguez added this to the 2.1.0 milestone Dec 13, 2019
deivid-rodriguez pushed a commit that referenced this issue Dec 13, 2019
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)
deivid-rodriguez pushed a commit that referenced this issue Dec 13, 2019
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)
deivid-rodriguez pushed a commit that referenced this issue Dec 13, 2019
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)
deivid-rodriguez pushed a commit that referenced this issue Dec 13, 2019
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)
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants