Skip to content

Enable linting for Windows#4261

Merged
tonistiigi merged 6 commits intomoby:masterfrom
jedevc:fix-windows-lint
Nov 9, 2023
Merged

Enable linting for Windows#4261
tonistiigi merged 6 commits intomoby:masterfrom
jedevc:fix-windows-lint

Conversation

@jedevc
Copy link
Copy Markdown
Member

@jedevc jedevc commented Sep 18, 2023

See #4228 (comment).

This PR enables linting for Windows, and also tidies up the issues from this 😱 (fmt.Errorf again, yay!)

Opened as draft until containerd/nydus-snapshotter#537 merges upstream and we can update.

Signed-off-by: Justin Chadwell <me@jedevc.com>
Signed-off-by: Justin Chadwell <me@jedevc.com>
Signed-off-by: Justin Chadwell <me@jedevc.com>
These functions are unused on windows and so cause linting issues.

Signed-off-by: Justin Chadwell <me@jedevc.com>
Signed-off-by: Justin Chadwell <me@jedevc.com>
@jedevc jedevc marked this pull request as ready for review November 2, 2023 11:05
@jedevc jedevc requested a review from crazy-max November 2, 2023 11:05
Comment on lines -21 to +23
GOARCH=amd64 golangci-lint run --build-tags "${BUILDTAGS}" && \
GOARCH=arm64 golangci-lint run --build-tags "${BUILDTAGS}" && \
GOOS=linux GOARCH=amd64 golangci-lint run --build-tags "${BUILDTAGS}" && \
GOOS=windows GOARCH=amd64 golangci-lint run --build-tags "${BUILDTAGS}" && \
GOOS=linux GOARCH=arm64 golangci-lint run --build-tags "${BUILDTAGS}" && \
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Should we do freebsd/amd64 too?

Copy link
Copy Markdown
Member Author

@jedevc jedevc Nov 2, 2023

Choose a reason for hiding this comment

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

Yeahhh, why not! 🎉 Done in eb32ebf (fyi @akhramov)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hmm. The lint run took 20 minutes. Is that just because it's establishing a cache for the new architectures, or would it be worthwhile to split the lint into multiple jobs?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah, we've essentially doubled the amount of linting we do now - it already takes around 10min on master (also what why is it so slow?).

Splitting SGTM, I'm a bit pressed for time atm, so might not get to it super soon.

Signed-off-by: Justin Chadwell <me@jedevc.com>
RUN --mount=target=/go/src/github.com/moby/buildkit --mount=target=/root/.cache,type=cache,sharing=locked \
GOARCH=amd64 golangci-lint run --build-tags "${BUILDTAGS}" && \
GOARCH=arm64 golangci-lint run --build-tags "${BUILDTAGS}" && \
GOOS=linux GOARCH=amd64 golangci-lint run --build-tags "${BUILDTAGS}" && \
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

follow-up: maybe we need some configuration options now to enable both full matrix and current architecture. In CI we should run all but in dev I don't really want to wait 4x longer. Maybe some parallelization pattern as well instead of &&

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.

4 participants