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

tools/third_party/h2+hpack+hyperframe are duplicated in tools/third_party/hyper/ #12603

Closed
foolip opened this issue Aug 21, 2018 · 7 comments
Closed

Comments

@foolip
Copy link
Member

foolip commented Aug 21, 2018

#11806 added tools/third_party/h2, tools/third_party/hpack and tools/third_party/hyperframe. (Also enum and certifi, which aren't part of the duplicated code.)

#12193 added tools/third_party/hyper/, which internally has copies of h2, hpack and hyperframe.

Pairs of duplicated directories:

The contents are not identical.

This seems very likely to cause us some trouble down the line, at the least confusion about which version is used.

@dhdavvie
Copy link
Contributor

Ahh yes. Hyper uses an older version of the h2 library. To avoid issues and avoiding downgrading the server's version of h2, I decided to include the downgraded h2 dependency within hyper's own folder. The only real way to avoid this would be to downgrade the entire server's version of h2, which I feel would not be in the best interest of the implementation, what do you think?

@dhdavvie
Copy link
Contributor

Actually I lie, I took a look at Hyper's use of h2 and managed to create a version that uses the new h2. I can look into shipping this customised version of Hyper to remove the confusion, although this may cause some complications later with CI when having custom libraries

@foolip
Copy link
Member Author

foolip commented Aug 21, 2018

@dhdavvie, I don't understand how these libraries fit together so I have nothing useful to say about how the dependencies ought to be untangled, just that what we have seems like a mistake, especially if both bits of code really are used.

Are wptserve/tests/functional/base.py and wptserve/tests/functional/test_response.py the only place that use hyper? They are the only real matches outside of hyper when searching for "from hyper", and the "import hyper" matches are for hyperframe...

@dhdavvie
Copy link
Contributor

I agree that it is definitely not ideal. I will look into how stable the implementation is when I manually change the Hyper library to use the newer version of h2, and this way we can hopefully remove the almost redundant library duplications.

Yes, and wptserve/tests/functional/test_handler.py

@mdittmer
Copy link
Contributor

friendly ping to @dhdavvie. Are there plans to dedup this quarter?

@dhdavvie
Copy link
Contributor

@mdittmer Thanks for the ping. I should have some time to give this a quick look, and see if I am able to manually upgrade Hyper's version of H2. There is maybe an alternative solution where we downgrade the server's version of H2 to match Hyper's, but this seems less favorable.

@foolip
Copy link
Member Author

foolip commented Dec 25, 2018

Fixed in #14355.

@foolip foolip closed this as completed Dec 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants