Piping diagnostics through BEP #11766
Conversation
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
|
@googlebot I consent. |
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
|
Quick summary of this change: Structured Compiler Diagnostics (design doc) is a way to surface compiler warnings and errors (otherwise known as diagnostics) in a structured manner, eventually leading up to the BEP. This will help the ecosystem create IDE integrations through Build Server Protocol / Language Server Protocol. cc a few people working in affected areas, who can help to make the calls to approve this feature request: @ericfelly for changes to Bazel action infrastructure (I can only add @philwo as reviewer, but not @ericfelly and @alandonovan, not sure why) |
|
cc @ulfjack |
| e.printStackTrace(); | ||
| } | ||
| if(fileSystem != null) | ||
| this.diagnosticsFile = new Artifact.SourceArtifact( |
There was a problem hiding this comment.
I'm quite sure this is not the best place to do this, but since I did not find a better place, I'd love to hear your feedback regarding it
| @@ -0,0 +1,15 @@ | |||
| #!/usr/bin/env bash | |||
There was a problem hiding this comment.
As of right now this only supports OSX, but this is only temporary, I'm working on a generalized solution
|
Adding @comius, who is working on the Java rules now. |
SocksDevil
force-pushed
the
diagnostics-pipe
branch
from
6fa2e2a
to
71f9d2a
Compare
|
I've noticed that @ericfelly left a comment in the design doc saying that he's added a new Would that be sufficient for logging diagnostics, and using a tool to parse from those logs? |
(Idk if it's best to keep the discussion through the thread or the design doc, please let me know.) It is mentioned that the field did not exist, but this is already what is being used for outputting the diagnostics. See here |
I think concretely the change being requested is that in |
Perhaps I didn't express myself correctly, but the action metadata logs are already being populated and it is the means of getting the information I'm using already for the BSP server. I'll delete the field from the build event protocol |
|
@andrefmrocha @illicitonion Thanks, I missed that. And thank you for working on this PR. Before we can go any further on this change, I'd really like to hear the thoughts from @ericfelly for Blaze Core implications, and @cushon for Java rules implications. While I think this is a good idea for Bazel in general, I don't have enough visibility into the technical implications like @ericfelly and @cushon for their respective areas, so I'd request for their approvals as well. This PR adds wiring to the SpawnAction and ActionAnalysisMetadata machinery for a specific language ruleset, which are really hot paths and should require careful review. Can we make this feature opt-in and less intrusive? This PR is also really large, which could make it difficult for reviewers to start from scratch, so I'd recommend reviewers to starting from the design doc. Is there any way you could break this PR down further? Lastly, please ensure that the new and existing tests are passing on Bazel CI, across all platforms, and run google-java-format to format it according to the Google styleguide. |
I believe the feature can be opt-in, this was also something discussed and implemented for The PR can definitely be broken even more: into the introduction of the diagnostics in actions, and then the implementation for java (taking into account the already existing one for java_tools). This would be quite interesting as well since we could parallelise the review of the Regarding the CI, as it is, I cannot make the tests pass, since they break due to the changes introduced to the |
|
The PR is quite invasive indeed. We are paying a large complexity cost
treating these diagnostic files as Artifacts. I'm not sure what this
achieves. In the design discussion, I brought up the question of whether
the diagnostics data was required incrementally. In other words, should a
null build still be guaranteed to emit all transitive diagnostic
information? Whether this is a requirement has significant implications on
the design. If it's not a requirement, I think the implementation can be
significantly simpler and extend the existing "action metadata" mechanism.
If it is a requirement, I'm not seeing how the current PR achieves that.
Eric Fellheimer
felly@google.com
…On Tue, Aug 11, 2020 at 3:51 AM André Rocha ***@***.***> wrote:
@andrefmrocha <https://github.com/andrefmrocha> @illicitonion
<https://github.com/illicitonion> Thanks, I missed that. And thank you
for working on this PR. Before we can go any further on this change, I'd
really like to hear the thoughts from @ericfelly
<https://github.com/ericfelly> for Blaze Core implications, and @cushon
<https://github.com/cushon> for Java rules implications.
While I think this is a good idea for Bazel in general, I don't have
enough visibility into the technical implications like @ericfelly
<https://github.com/ericfelly> and @cushon <https://github.com/cushon>
for their respective areas, so I'd request for their approvals as well.
This PR adds wiring to the SpawnAction and ActionAnalysisMetadata machinery
for a specific language ruleset, which are really hot paths and should
require careful review. Can we make this feature opt-in and less intrusive?
This PR is also really large, which could make it difficult for reviewers
to start from scratch, so I'd recommend reviewers to starting from the design
doc
<https://docs.google.com/document/d/1KCQMvUMNVUBDyDmZJ1DB80tf21MVQdyWYS0Y9D_P0as/edit#heading=h.5mcn15i0e1ch>.
Is there any way you could break this PR down further?
Lastly, please ensure that the new and existing tests are passing on Bazel
CI, across all platforms, and run google-java-format to format it according
to the Google styleguide.
I believe the feature can be opt-in, this was also something discussed and
implemented for rules_scala, where a flag was introduced into the scala
toolchain. Wdyt of that solution?
The PR can definitely be broken even more into the introduction of the
diagnostics in actions, and then the implementation for java (taking into
account the already existing one for java_tools). This would be quite
interesting as well since we could parallelise the review of the
java_tools and the introductions of the diagnostics. Wdyt?
Regarding the CI, as it is, I cannot make the tests pass, since they break
due to the changes introduced to the java_tools that the java actions are
expecting
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#11766 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADALHEXZTVGFJRGC3Q3XD4LSAD2BRANCNFSM4OZO3AQA>
.
|
Thanks for the feedback @ericfelly . I took some time to properly understand the use of Artifacts for this and discussed it with @illicitonion as well, so that I could get his insight. |
|
Thanks for the details André.
In order to be able to obtain diagnostics through a remote execution, the
diagnostics file must be one of the outputs of the action
This means that an action will have to be responsible to add this
Artifact to the outputs nonetheless.
Your 1st statement is true, but the 2nd does not follow from the 1st. The
diagnostic file needs to be known to the remote execution layer, but this
does not mean it needs to be represented as an Artifact. And there are
reasons we don't need or want it to be an Artifact. An Artifact implies
that it could be used by downstream actions (it doesn't), and that we would
need to properly detect external filesystem modifications to the file. In
terms of how this translates to code, I think you'll possibly want to
change the various Spawn abstractions, but not Actions.
Eric Fellheimer
felly@google.com
On Wed, Aug 12, 2020 at 7:48 AM André Rocha <notifications@github.com>
wrote:
… The PR is quite invasive indeed. We are paying a large complexity cost
treating these diagnostic files as Artifacts. I'm not sure what this
achieves. In the design discussion, I brought up the question of whether
the diagnostics data was required incrementally. In other words, should a
null build still be guaranteed to emit all transitive diagnostic
information? Whether this is a requirement has significant implications on
the design. If it's not a requirement, I think the implementation can be
significantly simpler and extend the existing "action metadata" mechanism.
If it is a requirement, I'm not seeing how the current PR achieves that.
Eric Fellheimer ***@***.***
… <#m_2599112781869148066_>
On Tue, Aug 11, 2020 at 3:51 AM André Rocha *@*.***> wrote: @andrefmrocha
<https://github.com/andrefmrocha> https://github.com/andrefmrocha
@illicitonion <https://github.com/illicitonion>
https://github.com/illicitonion Thanks, I missed that. And thank you for
working on this PR. Before we can go any further on this change, I'd really
like to hear the thoughts from @ericfelly <https://github.com/ericfelly>
https://github.com/ericfelly for Blaze Core implications, and @cushon
<https://github.com/cushon> https://github.com/cushon for Java rules
implications. While I think this is a good idea for Bazel in general, I
don't have enough visibility into the technical implications like
@ericfelly <https://github.com/ericfelly> https://github.com/ericfelly
and @cushon <https://github.com/cushon> https://github.com/cushon for
their respective areas, so I'd request for their approvals as well. This PR
adds wiring to the SpawnAction and ActionAnalysisMetadata machinery for a
specific language ruleset, which are really hot paths and should require
careful review. Can we make this feature opt-in and less intrusive? This PR
is also really large, which could make it difficult for reviewers to start
from scratch, so I'd recommend reviewers to starting from the design doc
https://docs.google.com/document/d/1KCQMvUMNVUBDyDmZJ1DB80tf21MVQdyWYS0Y9D_P0as/edit#heading=h.5mcn15i0e1ch.
Is there any way you could break this PR down further? Lastly, please
ensure that the new and existing tests are passing on Bazel CI, across all
platforms, and run google-java-format to format it according to the Google
styleguide. I believe the feature can be opt-in, this was also something
discussed and implemented for rules_scala, where a flag was introduced into
the scala toolchain. Wdyt of that solution? The PR can definitely be broken
even more into the introduction of the diagnostics in actions, and then the
implementation for java (taking into account the already existing one for
java_tools). This would be quite interesting as well since we could
parallelise the review of the java_tools and the introductions of the
diagnostics. Wdyt? Regarding the CI, as it is, I cannot make the tests
pass, since they break due to the changes introduced to the java_tools that
the java actions are expecting — You are receiving this because you were
mentioned. Reply to this email directly, view it on GitHub <#11766
(comment)
<#11766 (comment)>>,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/ADALHEXZTVGFJRGC3Q3XD4LSAD2BRANCNFSM4OZO3AQA
.
Thanks for the feedback @ericfelly <https://github.com/ericfelly> . I
took some time to properly understand the use of Artifacts for this and
discussed it with @illicitonion <https://github.com/illicitonion> as
well, so that I could get his insight.
Regarding null builds, the BSP client is expected to cache the diagnostics
on its side, so an incremental build is not necessarily required to do so.
Moreover, the server can also help by caching them on its side. There can
be some issues with this, but both me and @jastice
<https://github.com/jastice> agree that it makes more sense to follow
this approach for now. So that is not something we have to worry about
being emitted.
Regarding your idea to reuse the action metadata logs, I can understand
your point of view and I do agree with it, however maintaining a small
concern. In order to be able to obtain diagnostics through a remote
execution, the diagnostics file must be one of the outputs of the action,
according to @illicitonion <https://github.com/illicitonion> . This means
that an action will have to be responsible to add this Artifact to the
outputs nonetheless. Wdyt? For rules_scala this is already being done,
thus this change wouldn't be that significant. However, from what I know
right now, for java this would still mean that I would still have to
manually instantiate an Artifact. Perhaps there would be a better way to do
it, but it's still out of my reach (I even left a comment on where I
instatied it, asking for feedback). Other than that, I don't see any
problems with using the action metadata to our advantage.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#11766 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADALHEULQNPCIEDMUZITM43SAJ6O5ANCNFSM4OZO3AQA>
.
|
Ah, I was under the expression that outputs had to be artifacts, but I've come to understand that it's in fact However I'm not sure if I follow on how it's possible to implement this modifying only the Spawns. The only way to properly inform the wrappers around the compilers, is through the actions themselves, no? In the case of java, I understand that I could handle this on the side of its spawn specific spawn if I wanted to, however, I'm not following how I would get the information for the other rules, which are created through starlark. Are you suggest I obtain it through the context? |
|
I think my previous comment may have been a bit too strong. You'll have to
change some Action code. My point was just that you don't need to change
_every_ Action, like the change to AbstractAction.
…On Wed, Aug 12, 2020 at 1:07 PM André Rocha ***@***.***> wrote:
Your 1st statement is true, but the 2nd does not follow from the 1st. The
diagnostic file needs to be known to the remote execution layer, but this
does not mean it needs to be represented as an Artifact. And there are
reasons we don't need or want it to be an Artifact. An Artifact implies
that it could be used by downstream actions (it doesn't), and that we would
need to properly detect external filesystem modifications to the file. In
terms of how this translates to code, I think you'll possibly want to
change the various Spawn abstractions, but not Actions. Eric Fellheimer
***@***.***
Ah, I was under the expression that outputs had to be artifacts, but I've
come to understand that it's in fact ActionInputs so that is not the
case, thank you for clarifying it for me.
However I'm not sure if I follow on how it's possible to implement this
modifying only the Spawns. The only way to properly inform the wrappers
around the compilers, is through the actions themselves, no? In the case of
java, I understand that I could handle this on the side of its spawn
specific spawn if I wanted to, however, I'm not following how I would get
the information for the other rules, which are created through starlark.
Are you suggest I obtain it through the context?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#11766 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADALHEXNDCG3PIRTMVC5CILSALD4HANCNFSM4OZO3AQA>
.
|
Ah alright. Then I believe I should have an idea on how to implement this, at least for the starlark rules, I'll try and work on an implementation tomorrow. I'm just not sure which API should I use to specify the diagnostic file to the outputs of a Java compile action. Do you have a recommendation as to what extension of ActionInput is more fitting for this matter? |
|
In addition to the question I made above, I'd like to ask if you could explain when can the result of an action be null. I believe this to have been the reason that the action was was also made to have the diagnostics file, so that if a spawn does not return a result, the diagnostics is still passed down to the event. I'm wondering if this is a relevant case though |
|
@michaeledgar can you follow up here? |
|
@michaeledgar is point for BEP, but @anakanemison is doing a lot of work with IDEs, might also be interested. I'm not sure if this PR is still active, though? |
janakdr
added
the
P2
|
Assigning a priority to stop some bot nags. |
|
Please still consider this PR active. We can update it if somebody actually wants to deal with it :) |
|
Hello @andrefmrocha , Above PR has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days, if no further activity occurs. Thank you! |
|
it looks like parsing the compiler output is sufficient, so we don't need this PR for now, even though it would be good to have. |
|
Closing as stale. Please reopen if you'd like to work on this further. |
Pipes structured diagnostics information through BEP, information being used to develop a bazel extension to support BSP, which can be found here.
Starting the work to generalise to implement diagnostics for the
javacwrapper.Related to the work done in rules_scala, in PR #1068