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

Log TaskStarted line and column #6399

Merged
merged 1 commit into from
May 28, 2021
Merged

Log TaskStarted line and column #6399

merged 1 commit into from
May 28, 2021

Conversation

KirillOsenkov
Copy link
Member

We currently log the file in which a task is invoked, but we don't log the line and column, so if there are multiple tasks of the same name in a target, there's an ambiguity as to which task it is.

Pass the line and column information to TaskStartedEventArgs.

Fortunately we don't need to increment the binlog file format as we can piggy-back on the existing infrastructure for messages which can already log line and column if present. Reading older binlogs with the new MSBuild will work as the line and column flags won't be set, so no attempt to read anything extra. Reader newer binlogs with old MSBuild (same version but pre-this change) will also work as the line and column fields will be set and the existing infrastructure will read them, but nothing will consume that data.

We currently log the file in which a task is invoked, but we don't log the line and column, so if there are multiple tasks of the same name in a target, there's an ambiguity as to which task it is.

Pass the line and column information to TaskStartedEventArgs.

Fortunately we don't need to increment the binlog file format as we can piggy-back on the existing infrastructure for messages which can already log line and column if present. Reading older binlogs with the new MSBuild will work as the line and column flags won't be set, so no attempt to read anything extra. Reader newer binlogs with old MSBuild (same version but pre-this change) will also work as the line and column fields will be set and the existing infrastructure will read them, but nothing will consume that data.
Copy link
Member

@Forgind Forgind left a comment

Choose a reason for hiding this comment

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

I'm a little confused about your older binlog/newer MSBuild comment. If it tries to read something that isn't there, then even if accessing the line/column variables doesn't throw an error, why isn't the reader off of where it should be?

Comment on lines +110 to +111
LineNumber = reader.Read7BitEncodedInt();
ColumnNumber = reader.Read7BitEncodedInt();
Copy link
Member

Choose a reason for hiding this comment

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

Like @Forgind I also wonder if the change is backward compatible. Shouldn't you be testing for BuildEventArgsFieldFlags.LineNumber and BuildEventArgsFieldFlags.ColumnNumber here before reading the integers?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I think I understand. This code is separate from the binlog logic and has no versioning concerns, is that correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this is unrelated to binlog and used by the node packet translator to send events from worker nodes to central node. Nodes will only talk to each other if they’re the same MSBuild version.

Comment on lines +110 to +111
LineNumber = reader.Read7BitEncodedInt();
ColumnNumber = reader.Read7BitEncodedInt();
Copy link
Member

Choose a reason for hiding this comment

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

Ah, I think I understand. This code is separate from the binlog logic and has no versioning concerns, is that correct?

@Forgind Forgind added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label May 10, 2021
@Forgind Forgind merged commit ffa1a00 into main May 28, 2021
@KirillOsenkov KirillOsenkov deleted the dev/kirillo/logTaskLocation branch May 29, 2021 04:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Debuggability Issues impacting the diagnosability of builds, including logging and clearer error messages. Area: Logging merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants