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

x/vgo: don't allow cycles? #24098

Closed
nilebox opened this issue Feb 24, 2018 · 5 comments

Comments

@nilebox
Copy link

commented Feb 24, 2018

The blogpost about minimal version selection mentions support for cyclic dependencies between modules.
While this is nice that MVS can easily resolve cycles, I am not sure if allowing them is a good idea.
Go doesn’t allow import cycles, why should vgo allow cycles between modules then?
Doesn’t that promote bad coding practices?

Also, the blogpost shows the dependencies between the same versions, i.e. A1 <-> B1. Such situation has a 'chicken and egg' problem: you can't release A1 module until B1 is released, and vice versa. You can probably tag an A git repo with A1 tag with go.mod referencing a non-existing-yet B1 version of the B module (not sure if vgo would allow that), but it seems fragile and hard to build and test before both A1 and B2 are released.

The more likely real-life scenario would probably look closer to something like this:
cycle_complex
i.e. no actual cycle between A1 and B1, but implicit cycle between A and B modules.
And I think it would solve the same problem that @rsc has stated as justification for cycles:

Declaring this kind of cycle can be important when singleton functionality moves from one module to another. Our algorithms must not assume the module requirement graph is acyclic.

I am still not sure if such tightly coupled separate modules is a good idea (why not just merge them into one module?), but at least it would mean no 'chicken and egg' problem for releasing process anymore.

@gopherbot gopherbot added this to the vgo milestone Feb 24, 2018

@rogpeppe

This comment has been minimized.

Copy link
Contributor

commented Feb 24, 2018

FWIW we have real repositories that have cycles between them. The cycles usually involve tests. An example is when a server implementation lives in one repo and its client in another. To test the client, its tests import the server so that they can verify properly that the client code runs against a real server. The API parameters live in a package in the client so that the client package can be imported without importing the heavyweight server package.

Releasing is indeed fairly painful, and I think if I was doing this again, I'd avoid the cyclic repo dependency. Given that we use a git-commit-locked dependencies, the recipe for making a change looks something like:

- make the change in the client repo.
- update the server repo so that it works with the client repo.
- commit the server repo and push to a feature branch, not master.
- update client lock file to point to new server commit.
- commit client repo and make a PR for that change (the PR tests pass because the dependency management too is able to pull commits from feature branches).
- merge into client repo.
- make a PR on the server repo that merges the feature branch and updates the lock file to point to the new client commit.
- merge server PR
- make a PR on the client repo that changes the lock file to point to the latest server master commit.
- merge client PR.
- delete feature branch in server repo.
- breathe a sigh of relief that it's all over.

At this point both client and server lock files point to valid commits on each other and we can proceed as normal.

I think one could probably use a similar approach with vgo.

Thinking about it, I think the main reason this was a problem was that we were making incompatible changes. With major versions as separate import paths, things might be easier.

Knowing what I do now, I think a better approach would be to avoid importing the server code in the client at all and make the tests build the server as an entirely separate binary. That way you can easily test the client code against different server versions too.

@nilebox

This comment has been minimized.

Copy link
Author

commented Feb 24, 2018

@rogpeppe thanks for the real world example. Since vgo doesn't support lock files, you would need to reference an unreleased v0.0.0-20180208041248-4e4a3210bb54 feature branch commit, and the rest should be the same.

What I don't like about this recipe (apart from being very complicated) is that the version you tested is not the same you released, and potentially there could be more (untested) changes in between them.

@sdboyer

This comment has been minimized.

Copy link
Member

commented Feb 25, 2018

Cycles are also used as part of a technique - not sure if it's explored in the blog posts - that allows you to safely (as vgo defines it) move a package out of one module and into another, without breaking existing importers.

@4ad

This comment has been minimized.

Copy link
Member

commented Mar 1, 2018

Cycles are a killer feature of vgo. One of the primary reasons you might want to use it.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Mar 30, 2018

Cycles are also important for expressing moments where a module pair must move forward together. See #24301 (comment) for an example.

@rsc rsc closed this Mar 30, 2018

@golang golang locked and limited conversation to collaborators Mar 30, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
6 participants
You can’t perform that action at this time.