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 go workspaces to simplify dependency versioning #5991

Closed
jpkrohling opened this issue Aug 29, 2022 · 9 comments
Closed

Use go workspaces to simplify dependency versioning #5991

jpkrohling opened this issue Aug 29, 2022 · 9 comments
Assignees

Comments

@jpkrohling
Copy link
Member

During the SIG meeting on August 10, we agreed that we'd do a proof-of-concept of Go Workspaces to simplify the management of versions (especially replaces statements) in the Go modules for a repository.

I'll work on the PoC, while @bryan-aguilar was requested to help with the tooling, given Bryan's experience with similar tools.

@jpkrohling jpkrohling self-assigned this Aug 29, 2022
jpkrohling added a commit to jpkrohling/opentelemetry-collector that referenced this issue Aug 29, 2022
I was able to positively confirm that all modules had some effect when added to the go.work. For instance:

```
$ make install-tools
cd ./internal/tools && go install github.com/client9/misspell/cmd/misspell
cd ./internal/tools && go install github.com/golangci/golangci-lint/cmd/golangci-lint
no required module provides package github.com/golangci/golangci-lint/cmd/golangci-lint; to add it:
        go get github.com/golangci/golangci-lint/cmd/golangci-lint
make: *** [Makefile:130: install-tools] Error 1

$ go work use internal/tools/

$ make install-tools
cd ./internal/tools && go install github.com/client9/misspell/cmd/misspell
cd ./internal/tools && go install github.com/golangci/golangci-lint/cmd/golangci-lint
cd ./internal/tools && go install github.com/google/addlicense
cd ./internal/tools && go install github.com/ory/go-acc
cd ./internal/tools && go install github.com/pavius/impi/cmd/impi
cd ./internal/tools && go install github.com/tcnksm/ghr
cd ./internal/tools && go install github.com/wadey/gocovmerge
cd ./internal/tools && go install go.opentelemetry.io/build-tools/checkdoc
cd ./internal/tools && go install go.opentelemetry.io/build-tools/semconvgen
cd ./internal/tools && go install golang.org/x/exp/cmd/apidiff
cd ./internal/tools && go install golang.org/x/tools/cmd/goimports
cd ./internal/tools && go install github.com/jcchavezs/porto/cmd/porto
cd ./internal/tools && go install go.opentelemetry.io/build-tools/multimod
```

Fixes open-telemetry#5991

Signed-off-by: Juraci Paixão Kröhling <[email protected]>
@jpkrohling
Copy link
Member Author

I'll rebase the go-work PR today and figure out what else is needed (build, release, ...).

@bryan-aguilar, could you help me with this by assessing what needs to change regarding tooling?

@codeboten, any hints you can give me as part of a similar PR you have for the contrib?

@bryan-aguilar
Copy link
Contributor

I created an issue against go-build-tools about adding workspace support to crosslink.

@codeboten
Copy link
Contributor

@jpkrohling here's what i've done so far in my pr open-telemetry/opentelemetry-collector-contrib#10904:

  • created a workspace: go work init
  • added all the components: go work use -r .
  • removed all replace statements
  • synced workspace & modules: go work sync

One thing I've noticed is that the go.sum files for many modules now include lines for components within the repository which they did not use to:
https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/fb991d141f409ea16e8292decf5a612988c4e180/exporter/elasticsearchexporter/go.sum#L213-L214

It's unclear to me whether this is a problem or not. I tested making a change locally to a component used by another module, and building the module picked up the local change, which is what we want.

Some of the things I have left to check on my list:

  • will dependabot continue to work?
  • are there any issues w/ go mod tidy + go workspaces?

@bogdandrutu
Copy link
Member

bogdandrutu commented Sep 16, 2022

Other question:

  1. How do we do a release? We need to depend on an unreleased version, and seems that because there is a need to add the sum entry, that is not possible. Can you check the "prepare" PR with workspaces?

@jpkrohling
Copy link
Member Author

jpkrohling commented Sep 16, 2022

Thanks, @codeboten. That's almost verbatim what I did, including the manual test to validate that changes in one module are visible in another.

I'll check dependabot, and I have not found evidence that go mod tidy wouldn't work or would do unexpected things. When workspaces are active, the module resolution uses the workspace as the main context, so go mod commands will just use it when relevant.

I remember seeing that there's something special about releasing modules that are part of workspaces, but I'll read the docs again and summarize my understanding.

@codeboten
Copy link
Contributor

I remember seeing that there's something special about releasing modules that are part of workspaces, but I'll read the docs again and summarize my understanding.

Based on what I'm reading here, it seems that in order for us to make use of go workspaces, we would have to release any modules that have dependencies before being able to update the dependencies themselves to that version. If so this seems like it will complicate the process quite a bit.

jpkrohling added a commit to jpkrohling/opentelemetry-collector that referenced this issue Sep 16, 2022
I was able to positively confirm that all modules had some effect when added to the go.work. For instance:

```
$ make install-tools
cd ./internal/tools && go install github.com/client9/misspell/cmd/misspell
cd ./internal/tools && go install github.com/golangci/golangci-lint/cmd/golangci-lint
no required module provides package github.com/golangci/golangci-lint/cmd/golangci-lint; to add it:
        go get github.com/golangci/golangci-lint/cmd/golangci-lint
make: *** [Makefile:130: install-tools] Error 1

$ go work use internal/tools/

$ make install-tools
cd ./internal/tools && go install github.com/client9/misspell/cmd/misspell
cd ./internal/tools && go install github.com/golangci/golangci-lint/cmd/golangci-lint
cd ./internal/tools && go install github.com/google/addlicense
cd ./internal/tools && go install github.com/ory/go-acc
cd ./internal/tools && go install github.com/pavius/impi/cmd/impi
cd ./internal/tools && go install github.com/tcnksm/ghr
cd ./internal/tools && go install github.com/wadey/gocovmerge
cd ./internal/tools && go install go.opentelemetry.io/build-tools/checkdoc
cd ./internal/tools && go install go.opentelemetry.io/build-tools/semconvgen
cd ./internal/tools && go install golang.org/x/exp/cmd/apidiff
cd ./internal/tools && go install golang.org/x/tools/cmd/goimports
cd ./internal/tools && go install github.com/jcchavezs/porto/cmd/porto
cd ./internal/tools && go install go.opentelemetry.io/build-tools/multimod
```

Fixes open-telemetry#5991

Signed-off-by: Juraci Paixão Kröhling <[email protected]>
@jpkrohling
Copy link
Member Author

I ran a few experiments, and here's what I have to share at this point:

  • the page @codeboten linked is exactly what I had in mind. I decided to create a script to demonstrate what it would take for us to properly release following those instructions: https://github.com/jpkrohling/collector-workspaces/blob/main/scripts/release.sh . We certainly would need some automation for that, as the inter-dependencies can be quite intricate. Everything is doable except for pushing commits directly to the main branch. We would need a way to bypass our current restrictions without opening us up for trouble. Without the intermediary tags and commits, we wouldn't be able to run "go mod tidy" for instance, as we would be pointing to non-existing versions (on remote).
  • Then, I realized that we should be having the exact same problem today, and checked which types of commits we have during the release. We actually only touch the go.mod files: https://github.com/open-telemetry/opentelemetry-collector/pull/6079/files . It just so happens that we have replaces in the go.mod files pointing to the local modules, which makes it all work. I tried that with he second script and it "worked", except that it yields changes to the go.sum right after the change. So, we would probably need one step after that: https://github.com/jpkrohling/collector-workspaces/blob/main/scripts/dirty.sh
  • We then go back to the replaces statements: go.work does support that as well, except that it requires specific versions to be replaced. Using the same replacements we have currently on core returns a message like:

go: workspace module github.com/jpkrohling/collector-workspaces is replaced at all versions in the go.work file. To fix, remove the replacement from the go.work file or specify the version at which to replace the module.

I'm now at a point where I'm doubting we will be gaining any improvements by using go.work, given that our main goal was to indeed reduce the replace statements across modules.

I'm stopping the experiment here so that we can regroup and discuss what we want to do next.

@codeboten
Copy link
Contributor

I'm now at a point where I'm doubting we will be gaining any improvements by using go.work, given that our main goal was to indeed reduce the replace statements across modules.

I'm at the same point in the contrib repo. Until we decide to automate the release of individual components (if we decide to do so), there seems to be very little to gain from workspaces if we end up having to use these replace statements anyways.

@jpkrohling
Copy link
Member Author

I think this was worth the experiment, but it's clear by now that we aren't gaining much from using go workspaces.

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 a pull request may close this issue.

4 participants