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

gcassert: fix issues when running under bazel #103471

Merged
merged 1 commit into from
Jun 8, 2023
Merged

gcassert: fix issues when running under bazel #103471

merged 1 commit into from
Jun 8, 2023

Conversation

healthy-pod
Copy link
Contributor

@healthy-pod healthy-pod commented May 16, 2023 •
edited
Loading

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

@blathers-crl
Copy link

blathers-crl bot commented May 16, 2023

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@healthy-pod healthy-pod force-pushed the update-gcassert branch 4 times, most recently from bb3cbba to dcfd840 Compare May 16, 2023 22:00
rickystewart
rickystewart reviewed May 16, 2023
View reviewed changes
Copy link
Collaborator

@rickystewart rickystewart left a comment

Choose a reason for hiding this comment

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

What the hell is going on with the update to dev??

@healthy-pod
Copy link
Contributor Author

What the hell is going on with the update to dev??

just testing 😄

@healthy-pod healthy-pod force-pushed the update-gcassert branch 18 times, most recently from ea9eeaf to e23960d Compare June 5, 2023 22:23
@blathers-crl
Copy link

blathers-crl bot commented Jun 5, 2023

Thank you for updating your pull request.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@blathers-crl blathers-crl bot added the O-community Originated from the community label Jun 5, 2023
@healthy-pod healthy-pod force-pushed the update-gcassert branch 2 times, most recently from fbf3d98 to bb27891 Compare June 5, 2023 23:02
@healthy-pod healthy-pod requested a review from rickystewart June 6, 2023 23:31
@healthy-pod
Copy link
Contributor Author

This is ready for review. Generated code and GitHub CI failures should be fixed when I merge jordanlewis/gcassert#12 and point to it instead of my fork.

@healthy-pod healthy-pod force-pushed the update-gcassert branch 3 times, most recently from beade4d to 78a1348 Compare June 7, 2023 03:31
@healthy-pod healthy-pod requested a review from a team as a code owner June 7, 2023 03:31
@healthy-pod healthy-pod requested a review from jbowens June 7, 2023 03:31
@healthy-pod healthy-pod mentioned this pull request Jun 7, 2023
Merged
@healthy-pod healthy-pod force-pushed the update-gcassert branch 7 times, most recently from f8052df to 03b8ee6 Compare June 8, 2023 17:42
rickystewart
rickystewart reviewed Jun 8, 2023
View reviewed changes
build/teamcity/cockroach/ci/tests/lint_impl.sh Outdated
@@ -2,8 +2,14 @@

set -xeuo pipefail

# GCAssert requirements -- start
PATH="$(dirname $(bazel run @go_sdk//:bin/go --run_under=realpath)):$PATH"
Copy link
Collaborator

Choose a reason for hiding this comment

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

export PATH=

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

rickystewart
rickystewart requested changes Jun 8, 2023
View reviewed changes
Copy link
Collaborator

@rickystewart rickystewart left a comment

Choose a reason for hiding this comment

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

Commit message and PR description are no longer accurate

go.mod Outdated
@@ -159,7 +159,7 @@ require (
github.com/jackc/pgx/v5 v5.3.1
github.com/jaegertracing/jaeger v1.18.1
github.com/jordan-wright/email v4.0.1-0.20210109023952-943e75fe5223+incompatible
github.com/jordanlewis/gcassert v0.0.0-20221027203946-81f097ad35a0
github.com/jordanlewis/gcassert v0.0.0-20230607150025-9012d8e73bdd
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't need to/shouldn't be updating gcassert any more, right? The bazel patches that were applied are all faulty and we don't seem to be using them anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We still need to update gcassert to pull changes where we get the logs and can run in debug mode. In the current version we do not capture build logs.

pkg/cmd/generate-cgo/main.go Outdated
cppFlags := fmt.Sprintf("-I%s", filepath.Join(jemallocDir, "include"))
ldFlags := fmt.Sprintf("-L%s -L%s", filepath.Join(jemallocDir, "lib"), filepath.Join(projDir, "lib"))

tempToPersistentDirs := make(map[string]string, 3)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: srcToPersistentDirs seems more appopriate as a name since these files don't come from "temporary" directories.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@healthy-pod healthy-pod force-pushed the update-gcassert branch 2 times, most recently from c2b9f08 to ab125ed Compare June 8, 2023 19:43
@healthy-pod
Copy link
Contributor Author

healthy-pod commented Jun 8, 2023 •
edited
Loading

Commit message and PR description are no longer accurate

Updated

@healthy-pod healthy-pod requested a review from rickystewart June 8, 2023 19:45
This code change fixes issues when running gcassert under bazel by
b8311b7 
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
Copy link
Contributor Author

TFTR!

bors r=rickystewart

@craig
Copy link
Contributor

craig bot commented Jun 8, 2023

Build succeeded:

@craig craig bot merged commit 18fd3eb into cockroachdb:master Jun 8, 2023
@yuzefovich
Copy link
Member

Nice work figuring this out!

@healthy-pod healthy-pod mentioned this pull request Jun 21, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rickystewart rickystewart rickystewart approved these changes

@jbowens jbowens Awaiting requested review from jbowens jbowens was automatically assigned from cockroachdb/storage

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

bazel: TestLint/TestGCAssert fails when run under Bazel
4 participants
@healthy-pod @cockroach-teamcity @yuzefovich @rickystewart