fix: avoid overwriting go mod/sum files on module generation #7194
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jedevc
force-pushed
the
avoid-overwrite-of-go-mod-sum
branch
6 times, most recently
from
efcf892
to
edd64b0
Compare
Signed-off-by: Justin Chadwell <[email protected]>
Previously it wasn't, we were missing `telemetry` and `querybuilder`. And `go mod tidy` wouldn't *always* seem to pick it up, which is a bit of a pain. Signed-off-by: Justin Chadwell <[email protected]>
This is a bit of a mess - but essentially, what *was* happening was that
we had some very strange issues with go.mod generation. When generating
code, we had a couple of issues:
- If we were generating on top of an existing go.{mod,sum}, we were
essentially replacing its contents, and then relying on `go mod tidy`
to give reasonable results - however, this meant that explicitly
installed dependencies were getting lost, and that we were losing the
pins in the sum file.
- Additionally, if we were generating a new `dagger` subfolder in an
existing monorepo, we were overriding the previous go.{mod,sum} with
dagger's go.{mod,sum}, and then relying on `go mod tidy` to find all
the user dependencies.
The main issue with the above is that we were replacing contents of
these important dependency files in weird ways that make go modules much
more difficult to consume, especially in monorepos.
To fix these issues, we heavily rework the logic - we explicitly find
the correct go.mod file we want to work on, and then apply *the same*
processing everywhere. This processing involves checking go versions,
adding dependencies that aren't present, merging go.sum values, etc.
However - in doing this, something becomes quite apparent - we were
relying on some weird undefined behavior in the Go SDK. Generally the
structure of the paths passed in was that `ModuleContextPath` was a
parent to `OutputDir`, and we wouldn't modify anything outside
`OutputDir`. But consider, if the output directory is `./dagger` (the
default), then we may need to modify the top-level `./go.mod` (that
isn't in `OutputDir`). We technically *were* doing this, but by using
`go mod tidy` to get it to do this magically - but as mentioned above,
we need to avoid this automatic behavior, and do some better merging
ourselves.
To resolve this, `OutputDir` becomes the top-level, while `ModulePath`
becomes a relative sub-path in `OutputDir` that the module can be found
at (in the above, that's `./dagger`). This is why this patch also needs
to touch the Typescript runtime files, since we change the format of the
arguments to use this new format (also the Typescript generation was
confusing here, so picked up a couple of refactors as well along the
way).
Signed-off-by: Justin Chadwell <[email protected]>
- The `go.mod` test was incorrect because prior to the commit before this one, we were actually generating a `go.mod` in the child. This was *hard* to notice, because essentially, there was a weird disparity between having a `go.mod` and having a `go.mod` *and* a `main.go` in the top-level (since `loadPackage` fails on the former, but succeeds on the latter). The test is modified to ensure we can catch this in the future, and the prior commit makes sure that this weird disparity is removed. - The `go.work` tests were incorrect because we shouldn't have been putting `go.mod`s in the children (the same `loadPackage` issue as above). Each of these test cases should only have *one* module, so we alter the tests to check for this. Signed-off-by: Justin Chadwell <[email protected]>
jedevc
force-pushed
the
avoid-overwrite-of-go-mod-sum
branch
from
edd64b0
to
a41d9f3
Compare
sipsma
reviewed
May 2, 2024
cmd/codegen/generator/generator.go
Outdated
Comment on lines
35
to
36
| // ModulePath is the subpath where a module can be found. | ||
| ModulePath string |
There was a problem hiding this comment.
I'd prefer to keep the name more specific and include Context since that's what it is. Otherwise there's 3 dirs this could be referring to (context is the whole git repo the module lives in, source is where the dagger.json lives, source subpath is where the actual source code for the module is)
There was a problem hiding this comment.
Fixed, and restored the name to ModuleContextPath.
sipsma
approved these changes
May 2, 2024
cmd/codegen/main.go
Outdated
| if err != nil { | ||
| return err | ||
| } | ||
| if strings.HasPrefix(modulePath, "..") || strings.HasPrefix(modulePath, "../") { |
There was a problem hiding this comment.
nit, just informational: filepath.IsLocal is convenient in these situations
There was a problem hiding this comment.
Ah, we can't use that here since we only want to check if it's a parent - however, I did simplify this to using strings.Cut to avoid the repetition here.
Signed-off-by: Justin Chadwell <[email protected]>
vikram-dagger
pushed a commit
to vikram-dagger/dagger
that referenced
this pull request
May 3, 2024
…7194) * chore: run dagger develop on typescript runtime Signed-off-by: Justin Chadwell <[email protected]> * fix: top-level dagger.gen.go should have correct imports Previously it wasn't, we were missing `telemetry` and `querybuilder`. And `go mod tidy` wouldn't *always* seem to pick it up, which is a bit of a pain. Signed-off-by: Justin Chadwell <[email protected]> * fix: avoid overwriting go mod/sum files on module generation This is a bit of a mess - but essentially, what *was* happening was that we had some very strange issues with go.mod generation. When generating code, we had a couple of issues: - If we were generating on top of an existing go.{mod,sum}, we were essentially replacing its contents, and then relying on `go mod tidy` to give reasonable results - however, this meant that explicitly installed dependencies were getting lost, and that we were losing the pins in the sum file. - Additionally, if we were generating a new `dagger` subfolder in an existing monorepo, we were overriding the previous go.{mod,sum} with dagger's go.{mod,sum}, and then relying on `go mod tidy` to find all the user dependencies. The main issue with the above is that we were replacing contents of these important dependency files in weird ways that make go modules much more difficult to consume, especially in monorepos. To fix these issues, we heavily rework the logic - we explicitly find the correct go.mod file we want to work on, and then apply *the same* processing everywhere. This processing involves checking go versions, adding dependencies that aren't present, merging go.sum values, etc. However - in doing this, something becomes quite apparent - we were relying on some weird undefined behavior in the Go SDK. Generally the structure of the paths passed in was that `ModuleContextPath` was a parent to `OutputDir`, and we wouldn't modify anything outside `OutputDir`. But consider, if the output directory is `./dagger` (the default), then we may need to modify the top-level `./go.mod` (that isn't in `OutputDir`). We technically *were* doing this, but by using `go mod tidy` to get it to do this magically - but as mentioned above, we need to avoid this automatic behavior, and do some better merging ourselves. To resolve this, `OutputDir` becomes the top-level, while `ModulePath` becomes a relative sub-path in `OutputDir` that the module can be found at (in the above, that's `./dagger`). This is why this patch also needs to touch the Typescript runtime files, since we change the format of the arguments to use this new format (also the Typescript generation was confusing here, so picked up a couple of refactors as well along the way). Signed-off-by: Justin Chadwell <[email protected]> * tests: fix incorrect go.{mod,work} tests - The `go.mod` test was incorrect because prior to the commit before this one, we were actually generating a `go.mod` in the child. This was *hard* to notice, because essentially, there was a weird disparity between having a `go.mod` and having a `go.mod` *and* a `main.go` in the top-level (since `loadPackage` fails on the former, but succeeds on the latter). The test is modified to ensure we can catch this in the future, and the prior commit makes sure that this weird disparity is removed. - The `go.work` tests were incorrect because we shouldn't have been putting `go.mod`s in the children (the same `loadPackage` issue as above). Each of these test cases should only have *one* module, so we alter the tests to check for this. Signed-off-by: Justin Chadwell <[email protected]> * review comments Signed-off-by: Justin Chadwell <[email protected]> --------- Signed-off-by: Justin Chadwell <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #7145 - essentially, we need to a more careful merge of
go.modandgo.sumwhen adding dependencies, instead of replacing them.Essentially, when doing a
dagger init --sdk=goin an existing go repo, we were following a code path that meant we weren't using the builtingo.mod/go.sums at all, and were instead relying ongo mod tidyto do something reasonable (which is also slower!).Additionally, when using
dagger develop, we never preservedgo.sum, always completely overwriting it (instead of merging it with the previous contents). This only worked becausego mod tidydoes something reasonable (again, which is slower).Doing this was a bit tricky - the previous logic here was quite difficult to follow along with, so I've also updated it to be a bit clearer.