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

Split hyper into smaller subpackages #200

Closed
wants to merge 8 commits into from

Conversation

DiamondLovesYou
Copy link

hyper-core, hyper-protocol, hyper-net, hyper-client, and hyper-server.

Existing extern crate hyper; behaviors remain the same, though obviously some rust paths are now different.
Also bump the version major because this is quite a breaking change.

I haven't checked the tests yet; I'll be able to get to that sometime after my chem final. I created the PR anyway to let you look it over while I'm so occupied.

@s-panferov
Copy link
Contributor

Can we upload it to crates.io with such structure? The last time I tried to publish my project with local dependencies cagro said No.

@seanmonstar
Copy link
Member

wow. i started this a couple days ago, but didn't have time to finish, as it was a lot of work. thanks!

@@ -1,32 +1,37 @@
[package]

name = "hyper"
version = "0.0.1"
version = "1.0.0"
Copy link
Member

Choose a reason for hiding this comment

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

I'm not ready to call hyper 1.0 yet.

Copy link
Member

Choose a reason for hiding this comment

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

Semver says that while we're in version 0.x, breaking changes are to be expected.

Copy link
Author

Choose a reason for hiding this comment

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

Alright, fair enough.

@seanmonstar seanmonstar changed the title Split hyper into smaller subpackages: hyper-core, hyper-protocol, hyper-... Split hyper into smaller subpackages Dec 18, 2014
@seanmonstar
Copy link
Member

Why split into 5 packages? client and server are obvious. why not just 1 other crate, protocol or something?

@seanmonstar
Copy link
Member

@reem ping, since this is a big change. love your thoughts also

@reem
Copy link
Contributor

reem commented Dec 19, 2014

My primary concern, without even looking at the code, is that we won't be able to upload this to crates.io unless we upload each subcrate, which would be a large maintenance burden for me since I've been manually going through and updating crates.io after every change, and I would now have to do this for many crates and coordinate those changes.

@seanmonstar
Copy link
Member

@reem wait what, really? I didn't think that'd be the case... It looks like rust-openssl points at itself instead of the crates.io version for openssl-sys https://github.com/sfackler/rust-openssl/blob/master/Cargo.toml

@reem
Copy link
Contributor

reem commented Dec 19, 2014

@seanmonstar I talked to @sfackler and he confirmed my suspicion that the path dependency is only used for local builds and he has to change it to a crates.io dependency for uploads.

@sfackler
Copy link
Contributor

You don't have to change anything, actually. If you have something like

[dependencies.foo]
path = "../thingy"
version = "0.1.0"

It'll use the local version when building manually, but look for ^0.1.0 on crates.io when publishing. All you have to do is make sure the versions stay synced.

@reem
Copy link
Contributor

reem commented Dec 19, 2014

@sfackler is it possible to use version = "^0.0.0" or something like that?

@sfackler
Copy link
Contributor

You can use any of the version string configurations you'd normally be able to do: http://doc.crates.io/crates-io.html#using-crates.io-based-crates

@reem
Copy link
Contributor

reem commented Dec 19, 2014

Ok, so that clarifies the situation a bit - it is possible to use path for development, but it still does require uploading and maintaining crates.io versions for all the sub packages.

@seanmonstar
Copy link
Member

Um, wow. I didn't realize that. I'd hoped it could still be one published
crate that provided many.

Is there an issue in cargo for this to happen, or is it by design?

On Thu, Dec 18, 2014, 5:46 PM Jonathan Reem notifications@github.com
wrote:

Ok, so that clarifies the situation a bit - it is possible to use path for
development, but it still does require uploading and maintaining crates.io
versions for all the sub packages.


Reply to this email directly or view it on GitHub
#200 (comment).

@reem
Copy link
Contributor

reem commented Dec 19, 2014

I think it's partially by design (builds should be fully isolated), but it may be possible for Cargo to support path dependencies by ensuring they are packaged within the distribution.

Thanks,
Jonathan Reem

On Thu, Dec 18, 2014 at 9:24 PM, Sean McArthur notifications@github.com
wrote:

Um, wow. I didn't realize that. I'd hoped it could still be one published
crate that provided many.
Is there an issue in cargo for this to happen, or is it by design?
On Thu, Dec 18, 2014, 5:46 PM Jonathan Reem notifications@github.com
wrote:

Ok, so that clarifies the situation a bit - it is possible to use path for
development, but it still does require uploading and maintaining crates.io
versions for all the sub packages.


Reply to this email directly or view it on GitHub
#200 (comment).


Reply to this email directly or view it on GitHub:
#200 (comment)

@seanmonstar
Copy link
Member

@alexcrichton any future plans to support something like this, or bad idea?

@alexcrichton
Copy link
Contributor

This seems like a great idea! As @sfackler said the same Cargo.toml will serve for publishing and developing, and I'd like to add something along the lines of cargo publish --all which will publish all packages in the current repository, automating the publish-each-package-separately step.

@DiamondLovesYou
Copy link
Author

Why split into 5 packages? client and server are obvious. why not just 1 other crate, protocol or something?

hyper-net (contains net.rs from before this PR) is used by both the client && server, but nowhere else: it exists to provide net.rs Stuff to both. I created hyper-core to hold all the Things that everyone depends on. Depending on what features are requested, all of these crates are accessible from the main hyper crate.

These are intended to be as small as possible individually.

Ok, so that clarifies the situation a bit - it is possible to use path for development, but it still does require uploading and maintaining crates.io versions for all the sub packages.

I've added a script that'll publish all the packages. Once rust-lang/cargo#948 is fixed, it can be made even better. Obviously, I haven't tested it.

@DiamondLovesYou
Copy link
Author

This seems like a great idea! As @sfackler said the same Cargo.toml will serve for publishing and developing, and I'd like to add something along the lines of cargo publish --all which will publish all packages in the current repository, automating the publish-each-package-separately step.

👍

…er-net, hyper-client, and hyper-server.

Existing ```extern crate hyper;``` behaviors remain the same, though obviously some rust paths are now different.
Also bump the version major because this is quite a breaking change.
@reem
Copy link
Contributor

reem commented Dec 22, 2014

I'm still not clear on what the exact benefit of this change is - what do we gain by having a bunch of small packages in one place over just one package?

@DiamondLovesYou
Copy link
Author

@reem See #189.

@reem
Copy link
Contributor

reem commented Dec 22, 2014

I'm not sure I really empathize with the use case in #189, since hyper itself is not that big of a package yet and depending on the whole thing will add marginal additional compilation time etc.

That said I can still review this if @seanmonstar thinks we really want to do this, given the additional maintenance burden.

@DiamondLovesYou
Copy link
Author

@reem Compile time doesn't matter, size does. Also, the pepper API already provides it's own client API, so I'd like users to that and not use hyper's.

@reem
Copy link
Contributor

reem commented Dec 22, 2014

@DiamondLovesYou LTO should cut out unused stuff, and your users don't need to know you are using hyper (see Iron for an example of completely hiding the dependency).

@DiamondLovesYou
Copy link
Author

@reem Well, unforunetly in my case, LTO is broken. Nevertheless, what exactly are the server && client doing in this package? Shouldn't developers be using Iron for such features? I know I will. Moreover, small crates are a Good Thing: changes to server or client will be isolated, limiting those affected (remember extra?).

I realize this PR will require a few changes of hyper's users, however seeing as hyper hasn't reached 1.0 yet, this is the best time for such.

@seanmonstar
Copy link
Member

@DiamondLovesYou server and client are in this package as that is expected by most developers. Think of hyper as http in nodejs or Python. They include basic, functional, fast implementations that work for small things, and can be built upon, such as what Iron does.

As for this PR, I'm not against it, and think it may be worthwhile, but only after cargo supports it better.

@reem
Copy link
Contributor

reem commented Jan 21, 2015

It looks like progress and discussion on this PR has stalled, and it will require some serious rebasing to be relevant again. It seems that we don't need this change right now, but are open to it if done in a way that keeps maintenance and use simple. As a result, I'm closing this PR.

Feel free to open another PR with the changes updated or if you think further discussion is warranted.

@reem reem closed this Jan 21, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants