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

Lower required Go version to 1.20 #509

Closed
wants to merge 8 commits into from
Closed

Conversation

s-l-teichmann
Copy link
Contributor

This makes it easier to be used in other projects as a library.

JanHoefelmeyer
JanHoefelmeyer previously approved these changes Nov 15, 2023
Copy link
Contributor

@JanHoefelmeyer JanHoefelmeyer left a comment

Choose a reason for hiding this comment

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

LGTM

@ThomasJunk ThomasJunk self-requested a review November 15, 2023 11:23
ThomasJunk
ThomasJunk previously approved these changes Nov 15, 2023
Copy link
Contributor

@ThomasJunk ThomasJunk left a comment

Choose a reason for hiding this comment

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

LGTM

@tschmidtb51
Copy link
Collaborator

We also need to change the workflows to make sure that we are not accidentally using 1.21 functions...

@tschmidtb51 tschmidtb51 linked an issue Nov 15, 2023 that may be closed by this pull request
@s-l-teichmann
Copy link
Contributor Author

We also need to change the workflows to make sure that we are not accidentally using 1.21 functions...

@tschmidtb51
No, we should not do this. We should always use the latest stable to build our stuff. The go.mod file ensures that we don't use language features of a higher version. The binaries we deliver should benefit from the newer compilers.

@tschmidtb51
Copy link
Collaborator

@tschmidtb51 No, we should not do this. We should always use the latest stable to build our stuff. The go.mod file ensures that we don't use language features of a higher version. The binaries we deliver should benefit from the newer compilers.

@s-l-teichmann Thanks for the clarification.
Processwise: As I did the changes via the web, it might be easiest if you create a new PR with just the commits it originally had...

Copy link
Collaborator

@tschmidtb51 tschmidtb51 left a comment

Choose a reason for hiding this comment

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

Did someone test that successfully?

I just tried to run in on a machine with Go 1.20.4 and got the following result:

make build_linux 
mkdir -p bin-linux-amd64/ 
env GOARCH=amd64 GOOS=linux go build -o bin-linux-amd64/  -ldflags "-X github.com/csaf-poc/csaf_distribution/v3/util.SemVersion=v3.0.0-rc.1-5-gf572a3b" -v ./cmd/...
internal/options/log.go:12:2: package log/slog is not in GOROOT (/usr/local/go/src/log/slog)
make: *** [Makefile:78: build_linux] Error 1

If I remember correctly, the slog was also a reason why we choose Go 1.21... Or is that an error on my side?

@juan131
Copy link
Contributor

juan131 commented Nov 16, 2023

@tschmidtb51 I think slog was already in Go 1.20 as experimental, see https://pkg.go.dev/golang.org/x/exp/slog
but I'll give it a try and let you know

@juan131
Copy link
Contributor

juan131 commented Nov 16, 2023

I could reproduce the issue with this simple Dockerfile @tschmidtb51:

FROM bitnami/golang:1.20.11 as builder
COPY . /csaf_distribution
WORKDIR /csaf_distribution
RUN mkdir /output && \
    go build -o /output -ldflags "-X github.com/csaf-poc/csaf_distribution/v3/util.SemVersion=v3.0.0-rc.1-5-gf572a3b" -v ./cmd/...

FROM bitnami/bitnami-shell
COPY --from=builder /output /app
WORKDIR /app
ENTRYPOINT ["/app/csaf_checker"]
$ docker build . -t csaf-distro-image
(...)
11.09 internal/options/log.go:12:2: package log/slog is not in GOROOT (/opt/bitnami/go/src/log/slog)

I guess this overcomplicates maintaining compatibility with 1.20 significantly. Should we at least create a go-1.20 branch as suggested by @mpermar and replace slog there?

@s-l-teichmann
Copy link
Contributor Author

Did someone test that successfully?

I just tried to run in on a machine with Go 1.20.4 and got the following result:

make build_linux 
mkdir -p bin-linux-amd64/ 
env GOARCH=amd64 GOOS=linux go build -o bin-linux-amd64/  -ldflags "-X github.com/csaf-poc/csaf_distribution/v3/util.SemVersion=v3.0.0-rc.1-5-gf572a3b" -v ./cmd/...
internal/options/log.go:12:2: package log/slog is not in GOROOT (/usr/local/go/src/log/slog)
make: *** [Makefile:78: build_linux] Error 1

If I remember correctly, the slog was also a reason why we choose Go 1.21... Or is that an error on my side?

@tschmidt Yes, you are right. I totally forgot this. My bad.

@s-l-teichmann s-l-teichmann marked this pull request as draft November 16, 2023 08:37
@tschmidtb51
Copy link
Collaborator

We also need to change the workflows to make sure that we are not accidentally using 1.21 functions...

@tschmidtb51 No, we should not do this. We should always use the latest stable to build our stuff. The go.mod file ensures that we don't use language features of a higher version. The binaries we deliver should benefit from the newer compilers.

@s-l-teichmann: You mentioned that we shouldn't build with go 1.20 but those file changes (in the Github Actions) seem still to be part of the PR. Was that intentional?

@juan131
Copy link
Contributor

juan131 commented Nov 23, 2023

any update on this? Thanks in advance

@tschmidtb51
Copy link
Collaborator

tschmidtb51 commented Nov 23, 2023

We have discussed the path forward:

  • A new, cleaned up PR will be created.
  • The policy will be to support the latest stable and the oldstable go version.
  • Test need to cover both versions, Release just the latest
  • For the meantime (until the next Go release, 2024-02), we accept the risk that comes from the experimental libraries (as long as nothing breaks).
  • If something breaks, we'll need to reevaluate.

@s-l-teichmann s-l-teichmann mentioned this pull request Nov 24, 2023
@s-l-teichmann
Copy link
Contributor Author

s-l-teichmann commented Nov 24, 2023

Replaced by PR #514

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consuming CSAF model from go v1.20 projects
6 participants