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: TestLint/TestGCAssert fails when run under Bazel #65485

Closed
rickystewart opened this issue May 19, 2021 · 2 comments · Fixed by #98011 or #103471
Closed

bazel: TestLint/TestGCAssert fails when run under Bazel #65485

rickystewart opened this issue May 19, 2021 · 2 comments · Fixed by #98011 or #103471
Assignees
@healthy-pod
Labels
A-build-system C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-dev-inf

Comments

@rickystewart
Copy link
Collaborator

rickystewart commented May 19, 2021 •
edited by exalate-issue-sync bot
Loading

We see a lot of error output like this:

...
        ../../sql/colexec/sort.eg.go:2731:	s.sortCol.Get(s.order[j]): call was not inlined
        ../../sql/colexec/sort.eg.go:2921:	s.sortCol.Get(s.order[i]): call was not inlined
        ../../sql/colexec/sort.eg.go:2922:	s.sortCol.Get(s.order[j]): call was not inlined
        ../../sql/colexec/sort.eg.go:2990:	s.sortCol.Get(s.order[i]): call was not inlined
        ../../sql/colexec/sort.eg.go:2991:	s.sortCol.Get(s.order[j]): call was not inlined
        ../../sql/colexec/sort.eg.go:3051:	s.sortCol.Get(s.order[i]): call was not inlined
...

The same errors don't appear when the lint test is run outside the Bazel context. We're not sure if these errors are spurious or false alarms.

Epic CRDB-8349
Jira issue: CRDB-7629
@rickystewart rickystewart added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. A-build-system labels May 19, 2021
@rickystewart rickystewart mentioned this issue May 19, 2021
Closed
@rickystewart rickystewart mentioned this issue Aug 23, 2022
Merged
15 tasks
@yuzefovich yuzefovich mentioned this issue Oct 28, 2022
Merged
@rickystewart rickystewart mentioned this issue Mar 3, 2023
Merged
craig bot pushed a commit that referenced this issue Mar 6, 2023
@rickystewart
Merge #98011
856658b 
98011: bazel: add `gcassert` job in TC r=healthy-pod a=rickystewart

This simple job generates code then runs `gcassert` as a standalone
binary.

Closes #65485.

Epic: none
Release note: None

Co-authored-by: Ricky Stewart <[email protected]>
@craig craig bot closed this as completed in f9de621 Mar 6, 2023
@rickystewart rickystewart reopened this Apr 7, 2023
@rickystewart
Copy link
Collaborator Author

Not actually done, this job was flaky in TC and is skipped.

@rickystewart rickystewart mentioned this issue Apr 24, 2023
Closed
@srosenberg srosenberg mentioned this issue May 2, 2023
Merged
srosenberg added a commit to srosenberg/gcassert that referenced this issue May 2, 2023
@srosenberg
Couple of small internal "debug UX" improvements owing to [1],
b0f4f59 
 * redirect 'go build' output to a temp. file
 * report 'go build' error
 * fixup unit tests

[1] cockroachdb/cockroach#65485
@srosenberg
Copy link
Member

srosenberg commented May 2, 2023 •
edited
Loading

Not actually done, this job was flaky in TC and is skipped.

TL;DR The reason it's failing under bazel is due to the missing ./dev gen cgo step.

Reproducing

Let's use the PR [1] which captures "go build" output,

/home/srosenberg/go/src/github.com/gcassert/gcassert $(cat ./pkg/testutils/lint/gcassert_paths.txt | sed 's|^|./pkg/|')
See /tmp/gcassert-329905986.log for full output.
exit status 1

Upon closer examination of /tmp/gcassert-329905986.log,

pkg/server/status/runtime_jemalloc.go:21:11: fatal error: 'jemalloc/jemalloc.h' file not found
 #include <jemalloc/jemalloc.h>
          ^~~~~~~~~~~~~~~~~~~~~
1 error generated.

Thus, during type-checking a native (transitive) dependency is unresolved, causing go build to bail out early.

Fixing

Generate zcgo files,

./dev gen go cgo

Run,

/home/srosenberg/go/src/github.com/gcassert/gcassert $(cat ./pkg/testutils/lint/gcassert_paths.txt | sed 's|^|./pkg/|')
See /tmp/gcassert-1563406990.log for full output.

echo $?
0

Appendix

It would be nice to save the 'go build' output in CI, for ease of debugging future issues. Also, we should run the (gcassert) unit tests and bail out early if they fail. There is some sensitivity to compiler version; e.g., master has two failing unit tests (both fixed in [1]).

[1] jordanlewis/gcassert#10

@healthy-pod healthy-pod mentioned this issue May 11, 2023
Closed
healthy-pod pushed a commit to healthy-pod/cockroach that referenced this issue May 11, 2023
dev,generate-cgo: externalize cgo files gen into a seperate binary
c44cb55 
Previously, we only needed to gen cgo files using `dev` so that
was were that logic lived. Now, we need to gen cgo files outside
dev to fix an issue with gcassert [see cockroachdb#65485].

This code change externalizes the code into a separate binary
and let's `dev gen cgo` point to it.

Release note: None
Epic: None

Informs cockroachdb#65485
healthy-pod pushed a commit to healthy-pod/cockroach that referenced this issue May 11, 2023
gcassert: upgrade gcassert and generate cgo files before running it
4b5f433 
Release note: None
Epic: none

Closes cockroachdb#65485
healthy-pod pushed a commit to healthy-pod/cockroach that referenced this issue May 15, 2023
gcassert: upgrade gcassert and generate cgo files before running it
de5e2c0 
Release note: None
Epic: none

Closes cockroachdb#65485
healthy-pod pushed a commit to healthy-pod/cockroach that referenced this issue Jun 7, 2023
dev,generate-cgo: externalize cgo files gen into a seperate binary
38e5f3f 
Previously, we only needed to gen cgo files using `dev` so that
was were that logic lived. Now, we need to gen cgo files outside
dev to fix an issue with gcassert [see cockroachdb#65485].

This code change externalizes the code into a separate binary
and let's `dev gen cgo` point to it.

Release note: None
Epic: None

Informs cockroachdb#65485
@healthy-pod healthy-pod mentioned this issue Jun 7, 2023
Merged
healthy-pod pushed a commit to healthy-pod/cockroach that referenced this issue Jun 7, 2023
dev,generate-cgo: externalize cgo files gen into a seperate binary
83d4e72 
Previously, we only needed to gen cgo files using `dev` so that
was were that logic lived. Now, we need to gen cgo files outside
dev to fix an issue with gcassert [see cockroachdb#65485].

This code change externalizes the code into a separate binary
and let's `dev gen cgo` point to it.

Release note: None
Epic: None

Informs cockroachdb#65485
craig bot pushed a commit that referenced this issue Jun 8, 2023
Merge #104554
78dfbbc 
104554: dev,generate-cgo: externalize cgo files gen into a seperate binary r=healthy-pod a=healthy-pod

Previously, we only needed to gen cgo files using `dev` so that was were that logic lived. Now, we need to gen cgo files outside dev to fix an issue with gcassert [see #65485].

This code change externalizes the code into a separate binary and let's `dev gen cgo` point to it.

Release note: None
Epic: None

Informs #65485

Co-authored-by: healthy-pod <[email protected]>
healthy-pod pushed a commit to healthy-pod/cockroach that referenced this issue Jun 8, 2023
gcassert: fix issues when running under bazel
03b8ee6 
This code change pulls changes from jordanlewis/gcassert#11
and jordanlewis/gcassert#12 to let gcassert use bazel to build
if the `--use-bazel` flag is set.

It now checks if inline directives passed by inspecting object
files because the compiler output is missing some inline statements
when invoked through `bazel`.

Release note: None
Epic: CRDB-8349

Closes cockroachdb#65485
craig bot pushed a commit that referenced this issue Jun 8, 2023
Merge #103471
18fd3eb 
103471: gcassert: fix issues when running under bazel r=rickystewart a=healthy-pod

This code change fixes issues when running gcassert under bazel by ensuring that we generate go and cgo files, and pass a path to C and C++ compilers to the lint test process.

With these changes,  `gcassert` can run as part of the `Lint` build config and doesn't need its own build config so the gcassert build config scripts are deleted.

Release note: None
Epic: CRDB-8349

Closes #65485

Co-authored-by: healthy-pod <[email protected]>
@craig craig bot closed this as completed in b8311b7 Jun 8, 2023
@healthy-pod healthy-pod mentioned this issue Jun 21, 2023
Merged
healthy-pod pushed a commit to healthy-pod/cockroach that referenced this issue Jun 21, 2023
dev,generate-cgo: externalize cgo files gen into a seperate binary
a865a51 
Previously, we only needed to gen cgo files using `dev` so that
was were that logic lived. Now, we need to gen cgo files outside
dev to fix an issue with gcassert [see cockroachdb#65485].

This code change externalizes the code into a separate binary
and let's `dev gen cgo` point to it.

Release note: None
Epic: None

Informs cockroachdb#65485
healthy-pod pushed a commit to healthy-pod/cockroach that referenced this issue Jun 21, 2023
This code change fixes issues when running gcassert under bazel by
419a220 
ensuring that we generate go and cgo files, and pass a path to C and
C++ compilers to the lint test process.

With these changes, `gcassert` can run as part of the `Lint` build
config and doesn't need its own build config so the gcassert build
config scripts are deleted.

Release note: None
Epic: CRDB-8349

Closes cockroachdb#65485
@healthy-pod healthy-pod mentioned this issue Jun 21, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@healthy-pod healthy-pod

Labels
A-build-system C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-dev-inf
Projects
None yet
Milestone
No milestone
4 participants
@jlinder @srosenberg @rickystewart @healthy-pod