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

Go modules in subdirectories #502

Closed
BatmanAoD opened this issue Jul 10, 2023 · 21 comments · Fixed by #544 or #565
Closed

Go modules in subdirectories #502

BatmanAoD opened this issue Jul 10, 2023 · 21 comments · Fixed by #544 or #565
Labels
🐛 bug Something isn't working

Comments

@BatmanAoD
Copy link
Collaborator

When using a go module that's defined in a subdirectory of a repository, Go expects the version tag to start with the path of the parent directory of the go.mod file (reference). So for <repo>/foo/bar/go.mod, Go would expect tags of the form foo/bar/v....

When Knope encounters a go.mod file in a subdirectory, it should create a tag in this format (presumably in addition to the standard v...) when tagging a release. It would probably be simplest to check whether the go.mod is in a subdirectory relative to Knope.toml (or pwd if there isn't one), but the "right" approach would be to query git to find the repository root. (The CLI command is git rev-parse --show-toplevel; hopefully git2 provides access to this functionality.) Conversely, when determining the current version of a go.mod file, it should look for tags in this format and use them if present.

(Related to #207)

@dbanty
Copy link
Member

dbanty commented Jul 12, 2023

Ugh. I tried to design the multi-package tagging to work the way Go wants, but I used the package name instead of the path 🤦. I think naming the package the same as the path to the module will make this work, as a workaround.

I don't think it'll be that much work to fix this, it'll technically be a breaking change, but probably not for anyone actually using Knope or they would have encountered this issue already 😅. Hopefully I can get to it this weekend

@BatmanAoD
Copy link
Collaborator Author

@dbanty Hm, I may be misunderstanding, but are you saying Knope should generate a tag using one or more components from the package name in go.mod? I'm currently not seeing Knope generate any tags with slashes in them by default, just v$version. (I added an explicit Command step to apply a "prefixed" tag.)

My package name does match the module name, so I don't think that should cause any problems.

@BatmanAoD
Copy link
Collaborator Author

....also, no rush! For now the explicit command = "git tag golang/v$version" is working fine.

@dbanty
Copy link
Member

dbanty commented Jul 13, 2023

@BatmanAoD I meant specifying the package name in knope as if you had multiple packages: https://knope-dev.github.io/knope/config/packages.html

But I'm glad you've got a workaround!

@BatmanAoD
Copy link
Collaborator Author

Okay, I see now! So it appears that regardless of the type of file being versioned, packages.foo causes a tag called foo/v<version> to be created. (It also appears to make the Version tag unusable?) This behavior makes sense, but is there any way to also still create the "standard" version tag, without a prefix?

I also see that having both package and packages isn't supported, which does make intuitive sense, but makes it harder to handle Go libraries correctly. I think the "right" behavior is just that if a go.mod file is listed in versioned_files, a "multiple packages" style tag should be created, using the go.mod path itself but replacing go.mod with v<version>.

So this:

[package]
versioned_files = [... "golang/go.mod"]

...would result in tags of the form golang/v<version>. Does that make sense?

@dbanty
Copy link
Member

dbanty commented Jul 13, 2023

Yeah, 100% that is the correct behavior, and the ultimate fix for this issue. I also need to think of a way to make Command variables useful with the multi-package syntax, but that’s a more challenging issue 😅

@BatmanAoD
Copy link
Collaborator Author

Is Command executed exactly once, or once for each package? The latter seems like the "right" behavior, and I think it makes the Version replacement trivial.

@dbanty dbanty added the 🐛 bug Something isn't working label Jul 16, 2023
@dbanty dbanty mentioned this issue Aug 17, 2023
dbanty added a commit that referenced this issue Aug 17, 2023
Fixes #502

---------

Co-authored-by: Dylan Anthony <dbanty@users.noreply.github.com>
@BatmanAoD
Copy link
Collaborator Author

BatmanAoD commented Aug 22, 2023

Okay, this now generates the correct tag! 🎉

...unfortunately, the behavior is still a little surprising: if Knope also generates a commit (e.g. because it updated the module path in go.mod, or if it had to update a different language's package file), then the Go foo/v... tag is on the previous commit, whereas the standard v... tag is on the new commit:

commit 88b36cde0440eb5a0881226f631e3580dd80b47d (HEAD -> main, tag: v11.0.0)
Author: Kyle Strand <kstrand@abc.com>
Date:   Tue Aug 22 17:27:20 2023 -0600

    chore: prepare release 11.0.0

commit bcb8899c1e20e8d4e3a114998c2bc5a4440be8f3 (tag: golang/v11.0.0)
Author: Kyle Strand <kstrand@abc.com>
Date:   Tue Aug 22 17:27:12 2023 -0600

    fix!: empty

...actually, I am pretty sure that for versions above 1.0, Go will reject this, since the tagged go.mod file will still have the previous major version in the module path. 😞

@dbanty
Copy link
Member

dbanty commented Aug 22, 2023

Ugh, that makes sense because the PrepareRelease step is what sets the version, but the prep commit hasn’t happened yet.

I could add a commit into PrepareRelease itself, I’d just have to special-case Go to set the version after the commit instead of before… but only the bit that doesn’t modify go.mod 😵‍💫.

I would not create the tag until the Release step, but I’m actively working on allowing you to split the two steps into different workflows so you can upload assets during release (which should be built with the correct version).

So I guess adding commit with a special Go-specific step is the way to go unless you have any other ideas 😁.

@dbanty dbanty reopened this Aug 22, 2023
@BatmanAoD
Copy link
Collaborator Author

Hmm, I'm not sure I understand the problem with delaying the tagging until Release. Is it that there's no other way to maintain the calculated version number for Go packages between the PreRelease step and the Release step?

If that's the issue, here's a possibly-heretical suggestion: how about generating a comment with the full version number in go.mod?

@dbanty
Copy link
Member

dbanty commented Aug 23, 2023

Heretical? Maybe… but probably also a lot easier to manage 😁. You’re the Go dev, so if it’s okay with you then it’s okay with me!

@BatmanAoD
Copy link
Collaborator Author

I do use Go, but only under duress 😆

Let me ask a few friends who are probably more in tune with idiomatic Go tooling practices what they think.

@dbanty
Copy link
Member

dbanty commented Sep 4, 2023

@BatmanAoD One last thing I want to sort out, to make sure that this solution works: how does one typically embed a version in a Go binary? I saw some usage of debug.ReadBuildInfo(), but from golang/go#29228 it seems like that requires building a separate module, versioning that, and importing it just to spit out the version. That would mean you'd release some inner package before the outer one which is... interesting. I guess that brings up another question of would you want to release some packages in Git mode (no GitHub release, only tag) and others in GitHub mode?

Is there another common technique that you know of which would require needing the version at build time (after PrepareRelease determines the next version, but before Release tags it)?

@BatmanAoD
Copy link
Collaborator Author

Oh jeeze, sorry, that is waaay outside the scope of my ecosystem knowledge.

@dbanty
Copy link
Member

dbanty commented Sep 4, 2023

No worries! I’ll just fix the known tagging problem for now and we can adjust if someone has trouble in the future. Thanks!

@dbanty
Copy link
Member

dbanty commented Sep 4, 2023

Okay, I think #565 does it! I'd appreciate you testing it out if you get some time @BatmanAoD

@BatmanAoD
Copy link
Collaborator Author

Hm...I think this might actually have broken having Go and non-Go code in the same "package":

With just go.mod in knope.toml:

❯ knope release --dry-run
Would write 5.11.1 to go.mod
Would add files to git:
  go.mod
Would run git commit -m "chore: prepare release 5.11.1"
Would create Git tag v5.11.1
Would run git push && git push --tags

With just crates/mycrate/Cargo.toml in knope.toml:

❯ knope release --dry-run
Would write 5.11.1 to crates/mycrate/Cargo.toml
Would add files to git:
  crates/mycrate/Cargo.toml
Would run git commit -m "chore: prepare release 5.11.1"
Would create Git tag v5.11.1
Would run git push && git push --tags

With both:

❯ knope release --dry-run
Error:   × Problem with workflow release

Error: step::inconsistent_versions (link)

  × Versioned files within the same package must have the same version. Found 5.11.0 which does not match 0.0.0
  help: Manually update all versioned_files to have the correct version

@dbanty
Copy link
Member

dbanty commented Sep 8, 2023

Thanks for continuing to test this! I just pushed a fix, with added tests and a better error message so now when there are inconsistent versions it will tell you where it got those versions from.

@BatmanAoD
Copy link
Collaborator Author

Thanks for continuing to work on it!

Oh, I was testing main, not fix-go-mod-tagging.😅 But trying again with that branch, the behavior appears correct!

I still have the unreachable-tag issue (#505), but not yet any useful information about how Knope is reaching the problematic tag.

@BatmanAoD
Copy link
Collaborator Author

I think this issue can be closed, though!

@dbanty
Copy link
Member

dbanty commented Sep 9, 2023

I still have the unreachable-tag issue (#505), but not yet any useful information about how Knope is reaching the problematic tag.

Yeah… probably gonna be some complex thing about Git internals I didn’t understand 😅. We’ll keep adding more logging until we find it!

I think this issue can be closed, though!

Excellent, I’ll get a release out with the recent improvements then look at expanding —verbose or something.

dbanty added a commit that referenced this issue Sep 9, 2023
…565)

Closes #502 (again)

---------

Co-authored-by: Dylan Anthony <dbanty@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working
Projects
None yet
2 participants