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

deps: update oapi-codegen to v2 #4432

Merged
merged 3 commits into from
Nov 3, 2023
Merged

Conversation

matzf
Copy link
Contributor

@matzf matzf commented Nov 2, 2023

The oapi-codegen libraries went through a reorganization; the repositories for the oapi-codegen build tool was separated from the runtime libraries.
In our normal code dependencies, the oapi-codegen build tool no longer shows up. Import it in a dummy .go file as an easy way to ensure that we have all the appropriate dependencies (via the go.mod/go_deps.bzl) . This leaves some clutter in the go.mod file which is not aesthetically pleasing, but is not a real issue due to dependency graph pruning.
The alternative solution would have been to manage the dependencies of the rules_openapi bazel rules explicitly in a rules_openapi_dependencies macro. This turns out to be rather cumbersome, and can lead to unexpected or even invalid dependency version combinations, because rules_go does not have a way to separate the dependencies of the build tools from the dependencies of the code.

Aside; fix cfg = "exec" for the oapi-codegen buildtool so that this tool is built for the environment of the builder, not the target platform.


This change is Reviewable

The oapi-codegen libraries went through a reorganization; the
repositories for the oapi-codegen build tool was separated from the
runtime libraries.
In our go.mod / go_deps.bzl, oapi-codegen build tool no longer shows up.
Instead, we explicitly need to require it, in a new macro rules_openapi_dependencies().

Aside; fix `cfg = "exec"` for the oapi-codegen buildtool so that this
tool is built for the environment of the builder, not the target
platform.
Copy link
Contributor

@oncilla oncilla left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 36 files reviewed, 1 unresolved discussion (waiting on @matzf)

a discussion (no related file):
Have you considered adding oapi-codgen to tools.go?

I think it would be a nicer solution than manually maintaining the required go dependencies.


Copy link
Contributor Author

@matzf matzf left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 36 files reviewed, 1 unresolved discussion (waiting on @oncilla)

a discussion (no related file):

Previously, oncilla (Dominik Roos) wrote…

Have you considered adding oapi-codgen to tools.go?

I think it would be a nicer solution than manually maintaining the required go dependencies.

Interesting, no I haven't considered this, I'll give it a shot.

Generally I find it awkward that the go.mod file is polluted with build tools and all kinds of random garbage, so I wouldn't agree that is a nicer solution, but it certainly is the easiest.

I was happy to remove things from go.mod (I was only disappointed that it wasn't more). What I would have preferred is to keep the dependencies of the oapi-codegen tool entirely separate from the other go_deps.bzl -- it's strange to me that rules_go seems to insist that everything in the workspace must be compiled with the same dependency versions.


Instead of manually managing the oapi-codegen dependencies, add a dummy
.go file that forces the dependencies into go.mod.
Copy link
Contributor Author

@matzf matzf left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 38 files reviewed, 1 unresolved discussion (waiting on @oncilla)

a discussion (no related file):

Previously, matzf (Matthias Frei) wrote…

Interesting, no I haven't considered this, I'll give it a shot.

Generally I find it awkward that the go.mod file is polluted with build tools and all kinds of random garbage, so I wouldn't agree that is a nicer solution, but it certainly is the easiest.

I was happy to remove things from go.mod (I was only disappointed that it wasn't more). What I would have preferred is to keep the dependencies of the oapi-codegen tool entirely separate from the other go_deps.bzl -- it's strange to me that rules_go seems to insist that everything in the workspace must be compiled with the same dependency versions.

Done.


Copy link
Contributor

@oncilla oncilla left a comment

Choose a reason for hiding this comment

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

Reviewed 30 of 36 files at r1, 14 of 14 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @matzf)

a discussion (no related file):

Previously, matzf (Matthias Frei) wrote…

Done.

I think with dependency graph pruning, there is less of an issue with having a lot of dependencies in go.mod file.

alternative is to have a multi-module repo. But that has another can of worms that we don't want to worry about yet IMO.


Copy link
Contributor

@oncilla oncilla left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @matzf)

@matzf matzf merged commit 5d697c0 into scionproto:master Nov 3, 2023
1 check passed
@matzf matzf deleted the bump-oapi-codegen branch November 3, 2023 08:12
juagargi pushed a commit to netsec-ethz/scion that referenced this pull request Mar 8, 2024
The oapi-codegen libraries went through a reorganization; the
repositories for the oapi-codegen build tool was separated from the
runtime libraries.
In our normal code dependencies, the oapi-codegen build tool no longer
shows up. Import it in a dummy .go file as an easy way to ensure that we
have all the appropriate dependencies (via the `go.mod`/`go_deps.bzl`) .
This leaves some clutter in the `go.mod` file which is not aesthetically
pleasing, but is not a real issue due to dependency graph 
pruning.
The alternative solution would have been to manage the dependencies of
the `rules_openapi` bazel rules explicitly in a `rules_openapi_dependencies` 
macro. This turns out to be rather cumbersome, and can lead to
unexpected or even invalid dependency version combinations, because
`rules_go` does not have a way to separate the dependencies of the build
tools from the dependencies of the code.

Aside; fix `cfg = "exec"` for the oapi-codegen buildtool so that this
tool is built for the environment of the builder, not the target
platform.
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.

2 participants