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

Use VCS module paths in Go reference from Gherkin => Messages #1608

Merged
merged 12 commits into from
Jun 16, 2021

Conversation

mattwynne
Copy link
Member

@mattwynne mattwynne commented Jun 11, 2021

Summary

Ref #1550 - switch to using VCS module paths within the monorepo instead of pointing to a subrepo.

TODO:

Motivation and Context

If we can use module paths that point to the directory within the cucumber/common repo, we can get rid of a whole load of complexity around creating subrepo mirrors.

How Has This Been Tested?

cd gherkin/go && make

Types of changes

  • Bug fix (non-breaking change which fixes an issue).
  • New feature (non-breaking change which adds functionality).
  • Breaking change (fix or feature that would cause existing functionality to not work as expected).

@mattwynne mattwynne marked this pull request as draft June 11, 2021 21:54
@mattwynne mattwynne changed the base branch from main to update-go-make June 11, 2021 21:58
@mattwynne
Copy link
Member Author

i had a fiddle around but I can't seem to make this work. @lonnblad @jaysonesmith @aslakhellesoy @aurelien-reeves anyone care to take a look at it?

@mattwynne
Copy link
Member Author

mattwynne commented Jun 11, 2021

The error I see is:

go: github.com/cucumber/common/messages/[email protected]: missing github.com/cucumber/common/messages/go.mod and .../v16/go.mod at revision messages/v16.0.0

This combination seems to at least help it to figure out the right git tag. If I use github.com/cucumber/common/messages/go/v16 as the string it seems to get even more confused:

go: github.com/cucumber/common/messages/go/[email protected]: reading github.com/cucumber/common/messages/go/messages/go/go.mod at revision messages/go/v16.0.0: unknown revision messages/go/v16.0.0

@mattwynne mattwynne changed the title Use VCS module paths in Go Use VCS module paths in Go reference from Gherkin => Messages Jun 11, 2021
Base automatically changed from update-go-make to main June 11, 2021 23:20
@@ -1,7 +1,7 @@
module github.com/cucumber/gherkin-go/v19

require (
github.com/cucumber/messages-go/v16 v16.0.1
github.com/cucumber/common/messages/v16 v16.0.0
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no Git tag named v16.0.0 in the monorepo. all tags are of the form library/version, so you need to use something like this:

github.com/cucumber/common/messages/v16 messages/v16.0.0

I haven't tested if this is valid go module syntax. If it isn't, we have to start using a different naming scheme for tags - perhaps with a hyphen or dot instead of a slash,

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems to have ways of inferring the tag from the version string that I can't quite fathom yet. If you see the first error (...at revision messages/v16.0.0) it seems to have figured out the tag name and found it, but it has the wrong path to the actual go.mod file (missing github.com/cucumber/common/messages/go.mod)

Copy link
Member Author

Choose a reason for hiding this comment

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

In the second case it makes a different guess for the tag name, and can't find it in git: unknown revision messages/go/v16.0.0

Copy link
Contributor

Choose a reason for hiding this comment

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

go look for the module on the master branch.
But it does not exist (yet) because messages/go.mod still have module github.com/cucumber/messages-go/v16

with the following in gherkin/go/go.mod I was able to have a coherent error message:

github.com/cucumber/common/messages/go/v16 e0d826edaa5d24e50910eaa689194a8b3340c907

# go: github.com/cucumber/common/messages/go/[email protected]: parsing go.mod:
#         module declares its path as: github.com/cucumber/common/messages/v16
#                 but was required as: github.com/cucumber/common/messages/go/v16

I think we will have to update the modules like the following (in messages/go/go.mod):

module github.com/cucumber/common/messages/go/v16

So we may need at least 2 PR to make it work:

The first one updating all go.mod replacing the module path from the subrepos to the appropriate monorepo sub-directory (for example module github.com/cucumber/messages-go/v16 => module github.com/cucumber/common/messages/go/v16)

Then a second PR to update all the go require directive to point to the new location.

Ref #1550

Co-authored-by: Aurelien Reeves <[email protected]>
@aslakhellesoy
Copy link
Contributor

Wooot!!

Don't forget to update CHANGELOG before merging this, with an entry under Changed so we remember to make a major release next time.

And then I suppose we can archive all the mirrors and remove the mirroring stuff from the build? Including the executable we run to do the actual mirroring.

@aurelien-reeves
Copy link
Contributor

Wooot!!

Don't forget to update CHANGELOG before merging this, with an entry under Changed so we remember to make a major release next time.

And then I suppose we can archive all the mirrors and remove the mirroring stuff from the build? Including the executable we run to do the actual mirroring.

Changelog have already been updated as part of #1616
Maybe just add references to the PR into those?

@mattwynne mattwynne marked this pull request as ready for review June 16, 2021 18:04
@mattwynne mattwynne merged commit a2ff65d into main Jun 16, 2021
@mattwynne mattwynne deleted the 1550-use-vcs-module-refs-in-go branch June 16, 2021 18:04
@mattwynne
Copy link
Member Author

I think having the CHANGELOGs reference the main issue #1550 is good enough. Do you agree?

We'll need to do the new major releases of messages and gherkin as part of cucumber/godog#389 I guess.

@mattwynne mattwynne mentioned this pull request Jun 16, 2021
9 tasks
@mattwynne
Copy link
Member Author

@aslakhellesoy I've updated the checklist at the top of #1550 - can you take a look and see if you agree with the remaining steps?

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.

3 participants