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.

Sign up for GitHub

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

Jump to bottom

Introduce rebar_uri to avoid deprecation warnings on OTP 23 [master] #2195

Merged
merged 4 commits into from
Jan 28, 2020
Merged

Introduce rebar_uri to avoid deprecation warnings on OTP 23 [master] #2195

merged 4 commits into from
Jan 28, 2020

Conversation

michaelklishin
Copy link
Contributor

@michaelklishin michaelklishin commented Dec 26, 2019 •
edited
Loading

See #2191 for the background.

The new module attempts to act as a shim between http_uri (deprecated in OTP 23) and uri_string (available as of OTP 21+) with as little wheel reinvention as possible.

michaelklishin
michaelklishin commented Dec 26, 2019
View reviewed changes
test/rebar_utils_SUITE.erl
@@ -322,6 +322,7 @@ is_list_of_strings(_Config) ->
?assert(rebar_utils:is_list_of_strings("foo") == false).

url_append_path(_Config) ->
?assertEqual({ok, "https://repo.hex.pm:443/repos/org"}, rebar_utils:url_append_path("https://repo.hex.pm", "/repos/org")),
?assertEqual({ok, "https://repo.hex.pm:443/repos/org?foo=bar"}, rebar_utils:url_append_path("https://repo.hex.pm",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this test case is wrong. /repos/org?key=value is not a path segment. The new module will correctly encode the question mark but previous string concatenation one would leave it unencoded. So the test was updated to use a more sensible path "extension".

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed. Hopefully we just didn't have a thing that relied on that behaviour in a critical way.

@michaelklishin michaelklishin force-pushed the otp-23-master-compat-part2 branch from ba1e5cf to 7057cbe Compare December 26, 2019 18:55
@michaelklishin michaelklishin marked this pull request as ready for review December 27, 2019 09:08
@michaelklishin michaelklishin mentioned this pull request Dec 27, 2019
Merged
@michaelklishin michaelklishin changed the title Introduce rebar_uri Introduce rebar_uri to avoid deprecation warnings on OTP 23 [master] Dec 29, 2019
ferd
ferd reviewed Dec 29, 2019
View reviewed changes
src/r3_hex_api.erl Outdated Show resolved Hide resolved
src/rebar_uri.erl Show resolved Hide resolved
src/rebar_utils.erl Outdated Show resolved Hide resolved
src/rebar_utils.erl Outdated Show resolved Hide resolved
test/rebar_uri_SUITE.erl Show resolved Hide resolved
test/rebar_utils_SUITE.erl
@@ -322,6 +322,7 @@ is_list_of_strings(_Config) ->
?assert(rebar_utils:is_list_of_strings("foo") == false).

url_append_path(_Config) ->
?assertEqual({ok, "https://repo.hex.pm:443/repos/org"}, rebar_utils:url_append_path("https://repo.hex.pm", "/repos/org")),
?assertEqual({ok, "https://repo.hex.pm:443/repos/org?foo=bar"}, rebar_utils:url_append_path("https://repo.hex.pm",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed. Hopefully we just didn't have a thing that relied on that behaviour in a critical way.

@michaelklishin
Copy link
Contributor Author

I have submitted hexpm/hex_core#82.

@michaelklishin michaelklishin force-pushed the otp-23-master-compat-part2 branch from 4415ca2 to fa577bc Compare January 2, 2020 15:12
@michaelklishin
Copy link
Contributor Author

I moved append_path/2 to rebar_uri and addressed a few bits of feedback. Do we depend on hexpm/hex_core#82 being merged first? I received no response there so far, likely due to the holidays.

@ferd
Copy link
Collaborator

ferd commented Jan 6, 2020

I think it would make sense to have the hex_core PR come in first. However if we want to move this one forwards, I'd say that we would have to just exclude the change to vendored files; this does not get rid of all warnings, but moves the overall state forwards by removing most warnings.

@michaelklishin
Copy link
Contributor Author

Removing only some warnings won't make Rebar build on OTP 23 with "warnings as errors" on.

@ferd
Copy link
Collaborator

ferd commented Jan 7, 2020

It won't, but it's not worse than the current state of this PR while we wait for the other one to move forwards. It just reduces the risk of clashing changes being done in the meanwhile.

@michaelklishin
Copy link
Contributor Author

@ferd hexpm/hex_core#82 is in, besides looking for any potential differences in the vendored file, is there anything I should do to update this PR?

@ferd
Copy link
Collaborator

ferd commented Jan 24, 2020

I don't think so. I believe @starbelly was working on integrating the hex-core stuff. They changed how they validate packages, which turns to have an impact on the lockfiles, which requires more complex changes on that end as well.

@michaelklishin
Copy link
Contributor Author

So this can go in now then? Or should I just wait? :)

@starbelly
Copy link
Contributor

This should be closed in favor of #2213

@michaelklishin
Copy link
Contributor Author

This involves changes beyond #2213.

@michaelklishin
Copy link
Contributor Author

I will revert the src/r3_hex_api.erl changes.

@ferd
Copy link
Collaborator

ferd commented Jan 27, 2020

yeah only need to drop the vendored file changes, then I'll merge this, and #2213 will complete the objective for this PR. Next release will build warning-free regardless.

@starbelly
Copy link
Contributor

@michaelklishin

This involves changes beyond #2213.

Yup, my bad.

@michaelklishin
Introduce rebar_uri
db84238 
Attempts to bridge http_uri (deprecated in OTP 23)
and uri_string (available as of OTP 21+).

(cherry picked from commit 7057cbe)
@michaelklishin
r3_hex_api: make query string encoding warning free on OTP 19 and 23
5b5ea53 
(cherry picked from commit 1c9e91b)
@michaelklishin
Continue transitioning away from http_uri
00ce318 
(cherry picked from commit fa577bc)
@michaelklishin
Undo r3_hex_api changes in favor of erlang#2213
2f4521e 
(cherry picked from commit 08eb611)
@michaelklishin michaelklishin force-pushed the otp-23-master-compat-part2 branch from 08eb611 to 2f4521e Compare January 28, 2020 14:41
@michaelklishin
Copy link
Contributor Author

@ferd should be ready to go. I ended up undoing r3_hex_api changes without rebase -i, hopefully this would be acceptable as is.

@ferd ferd merged commit 1dbe537 into erlang:master Jan 28, 2020
@michaelklishin michaelklishin deleted the otp-23-master-compat-part2 branch January 28, 2020 21:50
@ferd ferd mentioned this pull request Jan 31, 2020
Merged
@ferd ferd mentioned this pull request Feb 27, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ferd ferd ferd left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@michaelklishin @ferd @starbelly