Add classpath + aggregate dependency information to minimal project buil...

sbt / sbt-remote-control

Create and manage sbt process using unicorns and forks

Other

74 stars 14 forks source link

Add classpath + aggregate dependency information to minimal project buil... #289

Closed jsuereth closed 9 years ago

jsuereth commented 9 years ago

...d.

Replaces #286

A few caveats

the 'configuration' exposed is Ivy's string-y format. Ideally we should hide this behind somethig more typesafe. To understand what it means, you have to fully understand configuration mappings in Ivy.
We've put the dependency listing as an optional field, since we're adding it to the 1.0 version of the protocol. This means any 1.0 client can send/receive this message.
We did not preserve binary compatibility of the API, only protocol compatibility. This should be "ok" since server and client are completely separated via classloader, and any client will have to pull in the protocol library.
We did not do extensive testing to make sure the appropriate values are returned in an integration test. That remains a TODO.

Thanks to @dancingrobot84 for the original patch.

Review by @havocp (Protocol update review by @eed3si9n )

cc> @dancingrobot84 @dpratt

havocp commented 9 years ago

looks reasonable but I guess we need to spell out what we do with protocol version for stuff like this. are we bumping it on every patch that adds a field, or only when we go to make a release? and the comment in Protocol.scala says we'll always be back-compat even when we bump it, which I think is right, but then why do we have the protocol-versioned directory in the tests?

havocp commented 9 years ago

I'm fine with saying we'll bump the protocol version essentially never. I think there's almost always a better answer - back-compat addition, feature tags, changing the lockfile we use to launch, etc.

The one argument for bumping it on every added field would be that then clients or servers can fail fast if they truly require a newer field. For example if a client needs this info you've added here and you bump to 2, then it could require 2 on startup and complain otherwise.

havocp commented 9 years ago

So here's an attempt to sketch how changes to the protocol get handled:

add new fields: bumping the protocol version allows clients to throw an error if they get a too-old server and require the new fields.
add new request: client can try it and get an error if not supported, or bumping protocol version allows them to theoretically check in advance
conditionally break semantics of existing messages/fields ("feature negotiation"): this can use the tags mechanism. if both client and server have the "such-and-such works in a new way" tag then they both switch over to the new way. alternatively, we could bump protocol version and if both have a certain version the semantics change, or something like that. Details will be worked out by the first person to want to break semantics...
unconditionally break semantics of existing messages/fields: we should 1) not do this, but 2) if we do, handle it during the server launch process so that we always launch a server matching the client, OR fail to launch if there's an existing incompatible server. e.g. we could use a distinct lockfile for distinct server generations.

jsuereth commented 9 years ago

@havocp @eed3si9n Check the new infrastructure i added to test new protocols and the bump to protoocl version 2.

havocp commented 9 years ago

LGTM. will let Eugene also look.

eed3si9n commented 9 years ago

LGTM