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

Spurious Bazel failures when compiling the graphs back end #4685

Closed
fruffy opened this issue May 27, 2024 · 12 comments
Closed

Spurious Bazel failures when compiling the graphs back end #4685

fruffy opened this issue May 27, 2024 · 12 comments
Assignees
Labels
bug This behavior is unintended and should be fixed. infrastructure Topics related to code style and build and test infrastructure.

Comments

@fruffy
Copy link
Collaborator

fruffy commented May 27, 2024

Bazel has been failing recently with this error message:

 Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging
In file included from external/com_github_p4lang_p4c/backends/graphs/parsers.h:22,
                 from external/com_github_p4lang_p4c/backends/graphs/parsers.cpp:18:
external/com_github_p4lang_p4c/backends/graphs/graphs.h:25:2: error: #error "This backend requires the boost graph headers, which could not be found"
   25 | #error "This backend requires the boost graph headers, which could not be found"

I tried to fix it with
https://github.com/p4lang/p4c/actions/runs/9257567840/job/25466961631?pr=4678 but that did not work it seems.
I am unsure how this failure can happen. @smolkaj any idea?

@fruffy fruffy added the infrastructure Topics related to code style and build and test infrastructure. label May 27, 2024
@fruffy fruffy added the bug This behavior is unintended and should be fixed. label May 27, 2024
@ChrisDodd
Copy link
Contributor

This seems to be intermittent, which indicates that there might be some difference depending on which specific CI machine it ends up running on. Does the bazel build attempt to first install all prereqs (like boost::graphs), or does it rely on some things being preinstalled somehow?

@fruffy
Copy link
Collaborator Author

fruffy commented May 28, 2024

This seems to be intermittent, which indicates that there might be some difference depending on which specific CI machine it ends up running on. Does the bazel build attempt to first install all prereqs (like boost::graphs), or does it rely on some things being preinstalled somehow?

The error in particular might actually be totally unrelated to the boost::graphs installation since they do not perform the same CMake check.
They convert the #cmakedefine in the config file to an actual #define
https://github.com/p4lang/p4c/blob/main/BUILD.bazel#L32
There must be something going wrong here.

@smolkaj
Copy link
Member

smolkaj commented Jun 6, 2024

Ugh, that's not great -- will need to take a look.

Is this actively blocking p4c PRs or are we disregarding this failures during submissions for the time being?

@fruffy
Copy link
Collaborator Author

fruffy commented Jun 6, 2024

Is this actively blocking p4c PRs or are we disregarding this failures during submissions for the time being?

We are disregarding the failures, but the spurious failures cause disruptions in the merge queue, which can be annoying (when the Bazel test fails the PR is dequeued and has to be manually resubmitted to the queue).

@ChrisDodd
Copy link
Contributor

ChrisDodd commented Jun 6, 2024

The failure rate seems to be only 10-30%, so it is not too bad -- when a merge fails, just resubmit and it will succeed (at least I've never had it fail the second time). Maybe a connection to test machine load? When there's a load spike, a bunch of things will fail which will reduce the load, so the resubmit tends to pass.

@smolkaj
Copy link
Member

smolkaj commented Jun 6, 2024

Trying to figure out who added that particular build rule so they can take a look.

@smolkaj
Copy link
Member

smolkaj commented Jun 6, 2024

I see it was added in #2849 by @maemre. Would you be able to take a look, Mehmet?

@smolkaj
Copy link
Member

smolkaj commented Jun 6, 2024

From Google's side, we're not currently using this backend, so we would be okay with commenting out the target for the time being.

@fruffy
Copy link
Collaborator Author

fruffy commented Jun 11, 2024

I will make the Bazel tests optional until we are able to fix the stability issues.

@smolkaj
Copy link
Member

smolkaj commented Jun 11, 2024 via email

@maemre
Copy link
Contributor

maemre commented Jun 11, 2024

I added this when doing debugging for the PR #2898 but that PR is not merged. I left Google before it could be merged and there hasn't been any activity on it in the past 3 years. If Google does not need it, there might not be other users for building the graphs backend using Bazel. So, I think commenting out/removing the graph backend build rule is fine.

@fruffy
Copy link
Collaborator Author

fruffy commented Jun 21, 2024

After #4720 this appears resolved.

@fruffy fruffy closed this as completed Jun 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This behavior is unintended and should be fixed. infrastructure Topics related to code style and build and test infrastructure.
Projects
None yet
Development

No branches or pull requests

4 participants