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.

Sign up for GitHub

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

Jump to bottom

[CI] Run golangci-lint with alternate build tags #11557

Closed
djaglowski opened this issue Jun 27, 2022 · 14 comments
Closed

[CI] Run golangci-lint with alternate build tags #11557

djaglowski opened this issue Jun 27, 2022 · 14 comments
Labels
ci-cd CI, CD, testing, build issues closed as inactive good first issue Good for newcomers priority:p2 Medium Stale

Comments

@djaglowski
Copy link
Member

Hmm, if I'm understanding this correctly, then make gofmt includes all *.go files regardless of build tags, whereas the linter does not.

Okay, yes, I think that's the case. Still, this doesn't feel like a problem specific to go fmt: it happens with all linters that we run as part of golangci-lint, right? The only difference is that gofmt can run on all files regardless of build tags, while other linters probably can't.

I guess this is an easy solution for go fmt, just wondering if the 'correct' solution is to run golangci-lint multiple times with different build tag combinations.

Originally posted by @mx-psi in #11440 (comment)
@djaglowski djaglowski added priority:p2 Medium ci-cd CI, CD, testing, build issues labels Jun 27, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Nov 9, 2022

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

@github-actions github-actions bot added the Stale label Nov 9, 2022
@mx-psi mx-psi added good first issue Good for newcomers and removed Stale labels Nov 9, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Jan 9, 2023

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

@github-actions github-actions bot added the Stale label Jan 9, 2023
@mx-psi mx-psi removed the Stale label Jan 10, 2023
@github-actions
Copy link
Contributor

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

@github-actions github-actions bot added the Stale label Mar 13, 2023
@mx-psi mx-psi removed the Stale label Mar 13, 2023
This was referenced Apr 24, 2023
Merged
Merged
@github-actions
Copy link
Contributor

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

@github-actions github-actions bot added the Stale label May 15, 2023
@mx-psi mx-psi removed the Stale label May 15, 2023
@github-actions
Copy link
Contributor

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

Pinging code owners:

  • issue: Github issue template generation code needs this to generate the corresponding labels.

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@github-actions github-actions bot added the Stale label Jul 17, 2023
@mx-psi mx-psi removed the Stale label Jul 17, 2023
@github-actions
Copy link
Contributor

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

Pinging code owners:

  • issue: Github issue template generation code needs this to generate the corresponding labels.

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@github-actions github-actions bot added the Stale label Sep 18, 2023
@mx-psi mx-psi removed the Stale label Sep 18, 2023
@gord02
Copy link
Contributor

gord02 commented Oct 20, 2023

Hello everyone. I'd like to try and work on this. What is an example of one of the build tag combinations that would potentially need to be used in a golangci-lint run?

@mx-psi
Copy link
Member

mx-psi commented Oct 23, 2023

Hello everyone. I'd like to try and work on this. What is an example of one of the build tag combinations that would potentially need to be used in a golangci-lint run?

Based on the PR linked on the original post, the e2e, integration and windows build tags were the ones causing the most trouble. You can't enable the windows build tag without disabling the linux tag first.

Here are other build tags we use:

❯ rg 'go:build' --no-filename | sort | uniq | cut -d' ' -f 2-
darwin
darwin || freebsd || dragonfly || netbsd || openbsd
!darwin && !freebsd && !dragonfly && !netbsd && !openbsd && !linux && !solaris && !windows
e2e
integration
integration && !windows
!linux
linux
!linux && !darwin
linux || darwin
!linux && !darwin && !freebsd && !openbsd
linux || darwin || freebsd || openbsd
!linux && !darwin && !freebsd && !openbsd && !solaris
linux || darwin || freebsd || openbsd || solaris
linux || solaris
!linux && !windows
!linux && !windows && !darwin
!race
race
tools
!windows
windows
!windows && !linux

Not all of these are relevant but it should give you a starting point.

@pjanotti
Copy link
Contributor

I got to this issue after noticing that lint is not been run for Windows. In my quick tests I can't get lint to consider the windows as a build-tag, it needed to be set as GOOS. I'm planning to start fixing lint issues on Windows and later to enable it as part of CI. Let me know if you have any concerns/recommendations in this regard.

/cc @mx-psi @djaglowski

@djaglowski djaglowski mentioned this issue Jan 17, 2024
Merged
@pjanotti pjanotti mentioned this issue Jan 23, 2024
Merged
dmitryax pushed a commit that referenced this issue Jan 24, 2024
@pjanotti
Add GOOS=windows to lint-matrix (#30743)
6a2e178 
**Description:**

Add a lint step to CI with `GOOS=windows`. This will ensure that code
for Windows keeps a clean lint pass.

Until the lint Go cache is invalidated the step with `GOOS=windows` will
be a bit longer than the Linux step. I'm measuring in my fork to see
what we should expect after the cache is updated. I had an alternative
implementation using Windows runners instead, but, overall it required
separated Go caches and many more runners, running on Linux also ensures
that the step can also be run by contributors on their dev boxes.

**Link to tracking Issue:**
Related to #11557 and #27866, but, it doesn't fix any of the two.

**Testing:**
Validated that the step is passing for all components
cparkins pushed a commit to AmadeusITGroup/opentelemetry-collector-contrib that referenced this issue Feb 1, 2024
@pjanotti @cparkins
Add GOOS=windows to lint-matrix (open-telemetry#30743)
0aa1641 
**Description:**

Add a lint step to CI with `GOOS=windows`. This will ensure that code
for Windows keeps a clean lint pass.

Until the lint Go cache is invalidated the step with `GOOS=windows` will
be a bit longer than the Linux step. I'm measuring in my fork to see
what we should expect after the cache is updated. I had an alternative
implementation using Windows runners instead, but, overall it required
separated Go caches and many more runners, running on Linux also ensures
that the step can also be run by contributors on their dev boxes.

**Link to tracking Issue:**
Related to open-telemetry#11557 and open-telemetry#27866, but, it doesn't fix any of the two.

**Testing:**
Validated that the step is passing for all components
@github-actions GitHub Actions
Copy link
Contributor

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

@github-actions github-actions bot added the Stale label Feb 12, 2024
@mx-psi mx-psi removed the Stale label Feb 12, 2024
@led0nk
Copy link
Contributor

led0nk commented Apr 1, 2024

what's the status on this issue ? i can't see if its still needed or what we need to provide

@mx-psi
Copy link
Member

mx-psi commented Apr 2, 2024

@led0nk AFAIK nobody has been actively working on this. You may rerun the command on #11557 (comment) (I used ripgrep but grep should also work) to get an initial list of build tag combinations (not all would be applicable)

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jun 3, 2024

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

@github-actions github-actions bot added the Stale label Jun 3, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Aug 2, 2024

This issue has been closed as inactive because it has been stale for 120 days with no activity.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Aug 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
ci-cd CI, CD, testing, build issues closed as inactive good first issue Good for newcomers priority:p2 Medium Stale
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@djaglowski @mx-psi @pjanotti @led0nk @gord02