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
fix: avoid overwriting go mod/sum files on module generation #7194
Conversation
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]>
edd64b0
to
a41d9f3
Compare
cmd/codegen/generator/generator.go
Outdated
// ModulePath is the subpath where a module can be found. | ||
ModulePath string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, and restored the name to ModuleContextPath
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One naming comment, but otherwise LGTM, thanks for the extra cleanup too!
cmd/codegen/main.go
Outdated
if err != nil { | ||
return err | ||
} | ||
if strings.HasPrefix(modulePath, "..") || strings.HasPrefix(modulePath, "../") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit, just informational: filepath.IsLocal
is convenient in these situations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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]>
…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]>
Fixes #7145 - essentially, we need to a more careful merge of
go.mod
andgo.sum
when adding dependencies, instead of replacing them.Essentially, when doing a
dagger init --sdk=go
in an existing go repo, we were following a code path that meant we weren't using the builtingo.mod
/go.sum
s at all, and were instead relying ongo mod tidy
to 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 tidy
does 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.