Skip to content

[0.9 backport] util/tracing: remove incorrect import enforcing comment#2297

Closed
crazy-max wants to merge 1 commit into
moby:v0.9from
crazy-max:0.9_backport_fix_otel_import
Closed

[0.9 backport] util/tracing: remove incorrect import enforcing comment#2297
crazy-max wants to merge 1 commit into
moby:v0.9from
crazy-max:0.9_backport_fix_otel_import

Conversation

@crazy-max
Copy link
Copy Markdown
Member

This import comment caused compilation of buildx to fail if `GO111MODULE` was
set to `off`:

Without `GO111MODULE` set (but with `-mod=vendor`:

    echo $GO111MODULE

    export PKG=github.com/docker/buildx
    export LDFLAGS="-X ${PKG}/version.Version=$(git describe --match 'v[0-9]*' --always --tags) -X ${PKG}/version.Revision=$(git rev-parse HEAD) -X ${PKG}/version.Package=${PKG}"
    GOFLAGS=-mod=vendor go build -o bin/docker-buildx -ldflags "${LDFLAGS}" ./cmd/buildx
    bin/docker-buildx version
    github.com/docker/buildx v0.6.0 d9ee3b1

When setting `GO111MODULE=off`, it fails on the incorrect import path in the
vendored file (looks like GO111MODULE=on ignores import-path comments?):

    export GO111MODULE=off
    root@5a55ec1c1eed:/go/src/github.com/docker/buildx# GOFLAGS=-mod=vendor go build -o bin/docker-buildx -ldflags "${LDFLAGS}" ./cmd/buildx
    vendor/github.com/moby/buildkit/client/client.go:20:2: code in directory /go/src/github.com/docker/buildx/vendor/github.com/moby/buildkit/util/tracing/otlptracegrpc expects import "go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc"
    vendor/go.opentelemetry.io/otel/exporters/otlp/otlptrace/internal/connection/connection.go:33:2: found import comments "go.opentelemetry.io/otel/exporters/otlp/internal/otlpconfig" (options.go) and "go.opentelemetry.io/otel/exporters/otlp/otlptrace/internal/otlpconfig" (optiontypes.go) in /go/src/github.com/docker/buildx/vendor/go.opentelemetry.io/otel/exporters/otlp/otlptrace/internal/otlpconfig

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@crazy-max crazy-max requested a review from thaJeztah August 5, 2021 03:51
@crazy-max crazy-max added this to the v0.9.1 milestone Aug 5, 2021
@thaJeztah
Copy link
Copy Markdown
Member

FWIW, if all changes in master are good to include in v0.9.x (no backward incompatible changes/features), perhaps instead the next release can be tagged from master (or just fast-forward the v0.9 branch). That saves a lot of work doing cherry picks.

Quick look at what's in master since the last release, I think all of those should be fine;
v0.9.0...master

@crazy-max
Copy link
Copy Markdown
Member Author

Yes atm it's safe to do that from master. Just wanted this specific commit to fix the otel import.

@thaJeztah
Copy link
Copy Markdown
Member

Yeah, the tricky bit is that once you start cherry-picking, there's no real "going back", and you'll have to cherry-pick each and every fix/change you want in the next release.

If it's needed "temporarily", you could of course just pick a non-tagged commit from master. If you're planning to do a release, perhaps postpone using the v0.9 branch and just tag from master (works slightly better with go modules as well, as it doesn't understand release branches, causing weird pseudo versions to be created, based on the last tag from master)

Or if you really want to release from a release branch, I would suggest fast-forwarding the v0.9 branch to be the same as master

@thaJeztah
Copy link
Copy Markdown
Member

Tempted to say; remove the v0.9 branch for now, and (if we need a v0.9.1) to tag it from master. Not sure if (besides this fix) we already need a v0.9.1 release (are there any fixes that make a difference in the binaries)? If not, and it's only needed to update the module in buildx, perhaps temporarily use tip of master (pseudo version)

@crazy-max
Copy link
Copy Markdown
Member Author

Remove the v0.9 branch for now, and (if we need a v0.9.1) to tag it from master.

Sounds like the best bet to me 👍

@crazy-max crazy-max closed this Aug 5, 2021
@crazy-max crazy-max deleted the 0.9_backport_fix_otel_import branch February 15, 2022 02:02
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