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

Define versioning strategy #129

Open
joshdifabio opened this issue Jan 2, 2017 · 23 comments
Open

Define versioning strategy #129

joshdifabio opened this issue Jan 2, 2017 · 23 comments
Milestone

Comments

@joshdifabio
Copy link
Contributor

joshdifabio commented Jan 2, 2017

Do we intend to use semantic versioning for these standards? I assume that we do and that the semver guarantees will apply to both the service provider interface (SPI) and the client API, as is standard within the PHP ecosystem. If that is the case, I think we should consider also maintaining a semantic version number for only the client API. Perhaps this could be done within composer.json:

{
    "name": "async-interop/event-loop",
    "provide": {
        "async-interop/event-loop-api": "1.0"
    }
}

Client packages could then require async-interop/event-loop-api while loop implementations would require async-interop/event-loop.

This would allow us to make breaking changes to the SPI, for example adding a method to Driver, without breaking all the client packages of event-loop.

@trowski
Copy link
Contributor

trowski commented Jan 5, 2017

Just be sure I'm understanding this approach, the following lines in a project's composer.json

"require": {
    "async-interop/event-loop": "^0.4",
    "async-interop/event-loop-implementation": "^0.4"
}

would become

"require": {
    "async-interop/event-loop-api": "^0.4",
    "async-interop/event-loop-implementation": "^0.4"
}

with loop implementations being the only libs to require async-interop/event-loop, allowing for separate versioning of the async-interop/event-loop as long as the API remains compatible.

If that's correct, I like it.

@joshdifabio
Copy link
Contributor Author

@trowski That's how I'd see it, yes.

Regarding packages which require event-loop-implementation; I would suggest that those dependencies are only necessary in packages which call Loop::execute() or Driver::run(). Personally, I would only add that dependency to my application, not to any standalone libs I work on unless they actually run the loop. How do others see that working?

@kelunik
Copy link
Member

kelunik commented Jan 5, 2017

@joshdifabio I see it the other way round. event-loop-implementation should be required by all libraries, applications don't need it, as they require a implementation that provides it anyway.

@kelunik
Copy link
Member

kelunik commented Jan 5, 2017

I thought about people wrongly depending on event-loop instead of event-loop-api, but that doesn't really matter, because if we break the interface, event-loop will get a new major version and nothing will break for these people.

Would you mind providing a PR?

@joshdifabio
Copy link
Contributor Author

I thought about people wrongly depending on event-loop instead of event-loop-api, but that doesn't really matter, because if we break the interface, event-loop will get a new major version and nothing will break for these people.

Yeah, that's why I thought the safest thing would be for event-loop represent the SPI + API.

Would you mind providing a PR?

Will do.

@joshdifabio
Copy link
Contributor Author

joshdifabio commented Jan 5, 2017

If we expect libs to always require both event-loop-api and event-loop-implementation, should we just amalgamate them? Perhaps we can just use event-loop-implementation instead and loop implementors can use the current event-loop API version as their event-loop-implementation version? It feels wrong to have the duplication otherwise.

Edit: We could even add something like conflict: { async-interop/event-loop-implementation: "<1.0 >1.0" } to event-loop to guarantee that implementations use the correct API version.

@kelunik
Copy link
Member

kelunik commented Jan 5, 2017

@joshdifabio event-loop-api and event-loop-implementation are two different things. event-loop-api is the accessor and factory API / basically everything except Driver. event-loop-implementation on the other hand exists only to make it easy to find implementations on Packagist and to remind application developers that they actually have to require some implementation to run their app, as we use a magic bootstrap factory.

What does that conflict thing really add? How does it guarantee that?

@joshdifabio
Copy link
Contributor Author

joshdifabio commented Jan 5, 2017

@kelunik is there value in making the two separate? What would the version number of event-loop-implementation mean?

What does that conflict thing really add? How does it guarantee that?

Imagine event-loop is at v0.5 and the API is at v0.2. In this case, I think we could do the following in event-loop:

{
    "name": "async-interop/event-loop",
    "conflict": {
        "async-interop/event-loop-implementation": "<0.2 >0.2"
    }
}

Now, if a loop implementation used the wrong API version, for example:

{
    "name": "foo/loop",
    "requires": { "async-interop/event-loop": "^0.5" }
    "provides": { "async-interop/event-loop-implementation": "0.1" }
}

Composer would prevent this buggy loop implementation from being used because of the conflict clause in event-loop.

@joshdifabio
Copy link
Contributor Author

Anyway, I don't think the conflict clause is that important; the important question to me is whether we want to force people to always require two packages instead of one and what the version of event-loop-implementation would mean in that context.

In my experience, if doing one thing implicitly means doing another, the two should be grouped together.

@kelunik
Copy link
Member

kelunik commented Jan 5, 2017

Even if they mean different things, I think we can kill event-loop-implementation, because if packages only depend on event-loop-api and not event-loop, something has to provide event-loop anyway, that's the loop implementation then.

@bwoebi
Copy link
Member

bwoebi commented Jan 5, 2017

All libraries what depend on directly is actually the API. The API depends on an implementation being provided.

In case you access e.g. the Driver interface directly, then you should specify implementation too. (to restrict what the API depends on.) But most libraries don't.

{
    "name": "async-interop/event-loop",
    "requires": { "async-interop/event-loop-implementation": "0.5" }
    "provides": { "async-interop/event-loop-api": "0.2" }
}

And for the loop impl:

{
    "name": "foo/loop",
    "requires": { "async-interop/event-loop": "^0.5" }
    "provides": { "async-interop/event-loop-implementation": "0.5" }
}

and for libraries:

{
    "name": "foo/library",
    "requires": { "async-interop/event-loop-api": "^0.2" }
}

(and applications similar to libraries, just they require a concrete implementation too.

@kelunik
Copy link
Member

kelunik commented Jan 5, 2017

@bwoebi The event-loop-implementation package is unnecessary, as it's just always the same version as event-loop directly.

@joshdifabio
Copy link
Contributor Author

I think I like Bob's suggestion, even if the circular dependencies feel a bit dirty.

@bwoebi The event-loop-implementation package is unnecessary, as it's just always the same version as event-loop directly.

I think the loop implementations need to provide something in order to be searchable on Packagist, so event-loop-implementation is needed for that reason?

@bwoebi
Copy link
Member

bwoebi commented Jan 5, 2017

@kelunik The event-loop requires an implementation when being used.

I think I like Bob's suggestion, even if the circular dependencies feel a bit dirty.

The fact is that there is a circular dependency. The implementation depends on the driver and the loop standard needs a Driver to work.

[To make it more cleanly we could split driver and impl, but I do not see much point in that except a little purism.]

@kelunik
Copy link
Member

kelunik commented Jan 5, 2017

[To make it more cleanly we could split driver and impl, but I do not see much point in that except a little purism.]

I thought about that, too. Makes PRs harder though, but there shouldn't anyway be many changes after the final version is released.

@kelunik The event-loop requires an implementation when being used.

It's enough for packages to have event-loop / an implementation in require-dev. If they depend on event-loop-api and not event-loop, Composer will report that event-loop is missing.

After thinking about it, I think I prefer separate packages, makes it all cleaner and avoids confusing virtual packages.

@joshdifabio
Copy link
Contributor Author

Interesting. So we'd have something like this?

some-loop-impl requires event-loop-driver, provides event-loop-driver-implementation
event-loop requires event-loop-driver, event-loop-driver-implementation
third-party-thing requires event-loop

Is that what you mean? Seems good to me!

@bwoebi
Copy link
Member

bwoebi commented Jan 5, 2017

Basically this, just without ripping this repo apart.

@kelunik
Copy link
Member

kelunik commented Jan 5, 2017

@bwoebi Without ripping this repo apart, we end up with having third party packages require event-loop-api instead of event-loop.

@kelunik
Copy link
Member

kelunik commented Jan 5, 2017

Splitting it is stupid, as Driver depends on InvalidWatcherException etc.

@kelunik
Copy link
Member

kelunik commented Jan 5, 2017

loop-impl requires event-loop, provides event-loop-implementation
event-loop requires event-loop-implementation, provides event-loop-api
third-party-thing requires event-loop-api
app requires loop-impl, third-party-thing

@joshdifabio
Copy link
Contributor Author

Splitting it is stupid, as Driver depends on InvalidWatcherException etc.

Good point.

I'll make a PR this evening unless someone else does it first.

@kelunik
Copy link
Member

kelunik commented Jan 8, 2017

@joshdifabio Ping, you wanted to provide a PR.

@kelunik kelunik modified the milestone: 1.0.0 Jan 8, 2017
@joshdifabio
Copy link
Contributor Author

Apologies, this slipped my mind. I'll provide a PR this afternoon.

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

No branches or pull requests

4 participants