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.

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

Do dependency checking via a validation action #62

Merged
merged 5 commits into from
Nov 13, 2024

Conversation

jadenPete
Copy link

You can read more about validation actions here:
https://bazel.build/extending/rules#validation_actions

This should enable dependency checking to be performed concurrently with phase_singlejar.

@jadenPete jadenPete self-assigned this Oct 30, 2024
@jadenPete jadenPete requested a review from jjudd October 30, 2024 17:16
@DavidANeil
Copy link
Member

Have you tested that --norun_validations correctly skips all of the validations? (i.e. all of the outputs are correctly segregated)

@DavidANeil
Copy link
Member

For when we pull this into our main repo you will want to experiment with --experimental_use_validation_aspect to see if it gives better scheduling.

@jadenPete jadenPete force-pushed the make-dependency-checking-a-validation-action branch from 16a61d6 to 99835eb Compare October 30, 2024 17:23
@jadenPete
Copy link
Author

Have you tested that --norun_validations correctly skips all of the validations? (i.e. all of the outputs are correctly segregated)

Good idea. I just tried building //dependencies/indirect:used_deps_off_c from within tests without --norun_validations, and a buildozer message appeared. I also built it with --norun_validations, and the message didn't appear, so it looks like we're good!

@jadenPete
Copy link
Author

jadenPete commented Oct 30, 2024

For when we pull this into our main repo you will want to experiment with --experimental_use_validation_aspect to see if it gives better scheduling.

Will do.

@jadenPete jadenPete force-pushed the make-dependency-checking-a-validation-action branch from 99835eb to 3b92fcc Compare October 30, 2024 17:32
if deps_configuration.used == "error":
outputs.append(deps_checks["used"])
if getattr(deps_configuration, name) == "error":
outputs.append(deps_check)
Copy link

Choose a reason for hiding this comment

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

If you removed deps_check from line 24, then where is deps_check coming from?

Copy link
Author

Choose a reason for hiding this comment

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

It should be coming from line 28.

Copy link

Choose a reason for hiding this comment

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

Totally missed that. My bad.

Jaden Peterson added 4 commits November 12, 2024 13:38
Previously, it was generated in the "javainfo" phase, which runs before
the "depscheck" phase. "javainfo" can't run after "depscheck" because
"compile" depends on "javainfo", and "depscheck" can't run before
"javainfo" because it depends on "compile".
This shouldn't be necessary, now that we're using validation actions.
@jjudd jjudd force-pushed the make-dependency-checking-a-validation-action branch from 78d0667 to 18aa2de Compare November 12, 2024 20:40
…ing the worker response

There was a race condition where a task could be cancelled with the
worker output streams still set to null, which would cause a null
pointer exception in the worker.

We also weren't always flushing the print stream before responding to
Bazel about the work request. This fixes that.
@jjudd jjudd force-pushed the make-dependency-checking-a-validation-action branch from 18aa2de to ac07080 Compare November 12, 2024 23:08
@jjudd jjudd merged commit aaf77df into lucid-master Nov 13, 2024
1 check failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants