Skip to content

Support RISV64 #2079

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

Closed
qdm12 opened this issue Jun 24, 2021 · 12 comments · Fixed by #2080
Closed

Support RISV64 #2079

qdm12 opened this issue Jun 24, 2021 · 12 comments · Fixed by #2080
Labels
enhancement New feature or improvement

Comments

@qdm12
Copy link

qdm12 commented Jun 24, 2021

your feature request related to a problem? Please describe.

GOARCH=riscv64 go build ./cmd/golangci-lint

fails with:

/go/pkg/mod/github.com/prometheus/[email protected]/cpuinfo.go:71:9: undefined: parseCPUInfo

Describe the solution you'd like.

I expect golangci-lint to be buildable for riscv64 as it is for all other platforms.

There is the dependency github.com/prometheus/procfs which added support for riscv64 in v0.6.0 (prometheus/procfs@910e685)

golangci-lint probably depends on many dependencies using versions pre-v0.6.0 of procfs so it's more of a long shot than something that can be fixed in a single PR I believe.

Describe alternatives you've considered.

go mod edit -replace=github.com/prometheus/procfs=github.com/prometheus/[email protected]
go mod tidy

Fixes the build, but probably in a risky-ish way I guess. Not a priority issue at all, but I thought it would be useful to track it.

Additional context

No response

@qdm12 qdm12 added the enhancement New feature or improvement label Jun 24, 2021
@boring-cyborg
Copy link

boring-cyborg bot commented Jun 24, 2021

Hey, thank you for opening your first Issue ! 🙂 If you would like to contribute we have a guide for contributors.

@ldez
Copy link
Member

ldez commented Jun 24, 2021

Hello,

The replace directives are a major issue because they are not transitive.
As some people use golangci-lint with the "tools pattern", the replace directives are a problem.

In all cases, the replace directive is not the good solution to force the version of a transtive lib, the right way is to use // indirect.

@ldez ldez mentioned this issue Jun 24, 2021
@qdm12
Copy link
Author

qdm12 commented Jun 24, 2021

@ldez thanks for the heads up! Using go get github.com/prometheus/[email protected] && go mod tidy also fixes the build. Although I'm not sure it's that safe still compared to upgrading dependencies one by one 🤔

@ldez
Copy link
Member

ldez commented Jun 24, 2021

If that build and run it's safe 😉

The best way to handle that is to fix the root cause:

$ go mod why -m github.com/prometheus/procfs
# github.com/prometheus/procfs
github.com/golangci/golangci-lint/pkg/golinters
github.com/yeya24/promlinter
github.com/prometheus/client_golang/prometheus
github.com/prometheus/procfs

But the prometheus library is like the grpc or protobuf libraries: the root of a major dependency hell in Go, each patch or minor version can break your project because of the dependencies.

The "indirect" approach follows semver, the Minimal Version Selection (MVS), and it's transitive, then it a safe way.

@qdm12
Copy link
Author

qdm12 commented Jun 24, 2021

go mod why

TIL about this command thanks!

each patch or minor version can break your project.

Oh really? Don't they carefully follow semver?

The "indirect" approach follows semver

Are we sure it's safe though? I guess it still replaces older versions with 0.6.0 for all dependencies right? As in it does compile so that's good, but I can't guarantee it behaves the same as before right? Especially since these are 0.x.x releases (unstable).

Ill dig through their release notes and get back.

@qdm12
Copy link
Author

qdm12 commented Jun 24, 2021

Actually I was wrong, the RISCV64 fix came with release v0.3.0, not v0.6.0.

So we could use v0.3.0 to stay on the safe side, although v0.6.0 might be safer with the several bug fixes it comes with 🤔

@ldez
Copy link
Member

ldez commented Jun 24, 2021

I guess it still replaces older versions with 0.6.0 for all dependencies right?

It's not really a replacement: by using indirect we feed the MVS with new information, and MVS chooses the right version.

In all the cases, MVS chooses only one version of a dependency, then currently we use v0.1.3.

In golangci-lint, we don't really use prometheus or procfs, the dependency is only used by promlinter to parse metric's names.

Also golangci-lint is not designed to be used as a library, then we don't have to care about use in another context.

so yes it's safe 😉

@ldez
Copy link
Member

ldez commented Jun 24, 2021

@ldez
Copy link
Member

ldez commented Jun 24, 2021

Still not confident? Want to be 300% sure? Check the diff between v0.1.3 and v0.6.0.

prometheus/procfs@v0.1.3...v0.6.0

There are no breaking changes of the API 😉

@ldez
Copy link
Member

ldez commented Jun 24, 2021

So don't be afraid:

  1. we don't use procfs
  2. github.com/prometheus/client_golang is compatible with v0.6.0
  3. there are no breaking changes of the API between v0.1.3 and v0.6.0
  4. we don't use procfs 😸
  5. golangci-lint is not designed to be used as a library
  6. go modules (and MVS) works well

Yes, it's safe 😄

Edit: I close the issue by mistake 😄

@ldez ldez closed this as completed Jun 24, 2021
@ldez ldez reopened this Jun 24, 2021
@qdm12
Copy link
Author

qdm12 commented Jun 24, 2021

Thanks so much for all the clarifications!

Sorry for being way too paranoid. Anyway, in the worst case promlinter may behave strangely in some corner cases, but as you mentioned golangcilint isn't meant to be used as a lib or a productiom program so that's fine.

A tiny few notes:

then check the compilation and run the tests

Assuming the tests cover the code and are deep enough to detect behavioral changes. Probably the case, but not always.

Check the diff between v0.1.3 and v0.6.0.

I did and also asked on their repo if they can think of any breaking change: prometheus/procfs#389

We can merge that 0.6.0 PR you made then, nice 👍 I'll come back here if I find anything. I'm also onto the helm and buildx repositories asking them to upgrade their procfs, although this might be more risky so we will definitely find any breaking change I think.

Edit: I close the issue by mistake 😄

Tell me about it, this close button is way too close to the comment button 😄

@ldez
Copy link
Member

ldez commented Jun 24, 2021

another element: between v1.7.1 and v1.11.0 of github.com/prometheus/client_golang, the procfs library has been updated.

As you can see there no changes, except go.mod and go.sum, related to procfs.

prometheus/client_golang@v1.7.1...v1.11.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or improvement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants