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

bazel: add way to run lints w/ bazel-built binaries #61555

Closed
rickystewart opened this issue Mar 5, 2021 · 7 comments · Fixed by #65518
Closed

bazel: add way to run lints w/ bazel-built binaries #61555

rickystewart opened this issue Mar 5, 2021 · 7 comments · Fixed by #65518
Assignees
@rickystewart
Labels
A-build-system C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)

Comments

@rickystewart
Copy link
Collaborator

No description provided.

@rickystewart rickystewart added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) A-build-system labels Mar 5, 2021
@rickystewart rickystewart self-assigned this Mar 5, 2021
@rickystewart rickystewart mentioned this issue Mar 5, 2021
Closed
@rickystewart
Copy link
Collaborator Author

I have a draft commit for this: rickystewart@f8f36b7

Two problems with this:

  1. go vet tries to compile a bunch of the source code, but it balks at the cgo commands to link in the native libraries:
        /usr/bin/ld: cannot find -lroach
        collect2: error: ld returned 1 exit status
        /usr/bin/ld: cannot find -lproj
        collect2: error: ld returned 1 exit status
        /usr/bin/ld: cannot find -ledit
        collect2: error: ld returned 1 exit status
        /usr/bin/ld: cannot find -lproj
        collect2: error: ld returned 1 exit status
        /usr/bin/ld: cannot find -lroach
        collect2: error: ld returned 1 exit status
        /usr/bin/ld: cannot find -lroach
        collect2: error: ld returned 1 exit status

The problem here is that we haven't appropriately set LDFLAGS to specify where these libraries live. So we need to update this code such that 1) we build libproj, libroach, and libedit, 2) we capture them in the data for the command, and 3) we update LDFLAGS appropriately in the script. Step (1) is the issue here, because I can't capture this stuff in the data for some reason.

  1. TestGCAssert throws up hundreds of lines of spurious failures like this:
        ../../sql/colexec/sort.eg.go:436:	s.sortCol.Get(s.order[i]): call was not inlined
        ../../sql/colexec/sort.eg.go:437:	s.sortCol.Get(s.order[j]): call was not inlined
        ../../sql/colexec/sort.eg.go:514:	s.sortCol.Get(s.order[i]): call was not inlined
        ../../sql/colexec/sort.eg.go:515:	s.sortCol.Get(s.order[j]): call was not inlined
        ../../sql/colexec/sort.eg.go:584:	s.sortCol.Get(s.order[i]): call was not inlined
        ../../sql/colexec/sort.eg.go:585:	s.sortCol.Get(s.order[j]): call was not inlined
        ../../sql/colexec/sort.eg.go:654:	s.sortCol.Get(s.order[i]): call was not inlined

Not sure why. We've already determined the go version in use isn't bad.

@jordanlewis
Copy link
Member

For point 2, it's either that the lint is working correctly and the compiler is no longer inlining the calls, which would be bad, or that the lint is somehow too fragile with the code paths that it's matching on. I'm guessing it's the latter, since the Bazel sandbox presumably does weird stuff to the paths of generated code.

Here's the code that does the path matching: https://github.com/jordanlewis/gcassert/blob/master/gcassert.go

Ricky, I'll try to dig into this soon, but realistically it might not be until Friday. Maybe we could poke at it together on a screenshare?

@rickystewart
Copy link
Collaborator Author

Hey Jordan -- sure, I'm back in office now, so whenever is fine :)

@rickystewart
Copy link
Collaborator Author

filed bazel-contrib/rules_foreign_cc#619 for issues arising from (1).

@rickystewart
Copy link
Collaborator Author

My understanding is bazel-contrib/rules_go#2858 (will probably be merged soon) helps with (2), and probably various other Bazel issues as well.

@rickystewart
Copy link
Collaborator Author

#65485 for the TestGCAssert thing

@rickystewart
Copy link
Collaborator Author

#65517 for remaining issues with TestRoachVet

@rickystewart rickystewart mentioned this issue May 20, 2021
Merged
rickystewart added a commit to rickystewart/cockroach that referenced this issue May 20, 2021
@rickystewart
bazel: provide way to run lint in bazel
6a127dd 
With this patch, lints can be run with `bazel run build/bazelutil:lint`.
The test binary takes the typical Go command-line arguments, so e.g.
`bazel run build/bazelutil:lint -- -test.v`.

We model this as a `sh_binary` that depends on the lint test binary as
well as all the dependencies that it has (`roachvet`, `crlfmt`, and
others). The shell script is a light wrapper that finds the actual
location of all the dependencies and stages them all in the `PATH`
appropriately. We also need to munge `LDFLAGS` because the linter calls
into the compiler in a couple places, and `cgo` wants to link into some
of the C libraries (`libedit`, `libproj`, etc.)

There are a couple tests that are still broken, which are skipped for
now. As a next step, we'll want to get this running in CI.

Closes cockroachdb#61555

Release note: None
craig bot pushed a commit that referenced this issue May 27, 2021
@rickystewart
Merge #65518 #65678
61d5d37 
65518: bazel: provide way to run lint in bazel r=rail a=rickystewart

With this patch, lints can be run with `bazel run build/bazelutil:lint`.
The test binary takes the typical Go command-line arguments, so e.g.
`bazel run build/bazelutil:lint -- -test.v`.

We model this as a `sh_binary` that depends on the lint test binary as
well as all the dependencies that it has (`roachvet`, `crlfmt`, and
others). The shell script is a light wrapper that finds the actual
location of all the dependencies and stages them all in the `PATH`
appropriately. We also need to munge `LDFLAGS` because the linter calls
into the compiler in a couple places, and `cgo` wants to link into some
of the C libraries (`libedit`, `libproj`, etc.)

There are a couple tests that are still broken, which are skipped for
now. As a next step, we'll want to get this running in CI.

Closes #61555.

Release note: None

65678: bazel: fix `pprofui` test in bazel r=rail a=rickystewart

We just need to set `HOME` to some value. Also add a couple more
`gazelle:resolve` hints.

Release note: None

(Ignore the bottom commit which comes from #65677.)


Co-authored-by: Ricky Stewart <[email protected]>
@craig craig bot closed this as completed in 155a8fa May 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rickystewart rickystewart

Labels
A-build-system C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

bazel: provide way to run lint in bazel rickystewart/cockroach
2 participants
@jordanlewis @rickystewart