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

Add support for embedding arbitrary files into binlog #6339

Merged
merged 3 commits into from Apr 21, 2021
Merged

Add support for embedding arbitrary files into binlog #6339

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1d97b48
Add support for embedding arbitrary files into binlog
KirillOsenkov Apr 10, 2021
92632a9
Add ItemTypeNames.EmbedInBinlog
KirillOsenkov Apr 15, 2021
efec80c
Don't subscribe to EmbedFile if not embedding any files
KirillOsenkov Apr 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 13 additions & 0 deletions src/Build/Logging/BinaryLogger/BinaryLogger.cs
View file
Open in desktop
Expand Up @@ -161,13 +161,26 @@ public void Initialize(IEventSource eventSource)
binaryWriter = new BinaryWriter(stream);
eventArgsWriter = new BuildEventArgsWriter(binaryWriter);

if (projectImportsCollector != null)
{
eventArgsWriter.EmbedFile += EventArgsWriter_EmbedFile;
}

binaryWriter.Write(FileFormatVersion);

LogInitialInfo();

eventSource.AnyEventRaised += EventSource_AnyEventRaised;
}

private void EventArgsWriter_EmbedFile(string filePath)
{
if (projectImportsCollector != null)
{
projectImportsCollector.AddFile(filePath);
}
}

private void LogInitialInfo()
{
LogMessage("BinLogFilePath=" + FilePath);
Expand Down
31 changes: 31 additions & 0 deletions src/Build/Logging/BinaryLogger/BuildEventArgsWriter.cs
View file
Open in desktop
Expand Up @@ -104,6 +104,11 @@ internal class BuildEventArgsWriter
/// </summary>
private readonly List<KeyValuePair<int, int>> nameValueIndexListBuffer = new List<KeyValuePair<int, int>>(1024);

/// <summary>
/// Raised when an item is encountered with a hint to embed a file into the binlog.
/// </summary>
public event Action<string> EmbedFile;

/// <summary>
/// Initializes a new instance of BuildEventArgsWriter with a BinaryWriter
/// </summary>
Expand Down Expand Up @@ -764,6 +769,7 @@ private void WriteProjectItems(IEnumerable items)
{
WriteDeduplicatedString(itemType);
WriteTaskItemList(itemList);
CheckForFilesToEmbed(itemType, itemList);
});

// signal the end
Expand All @@ -776,6 +782,7 @@ private void WriteProjectItems(IEnumerable items)
{
WriteDeduplicatedString(itemType);
WriteTaskItemList(itemList);
CheckForFilesToEmbed(itemType, itemList);
});

// signal the end
Expand Down Expand Up @@ -803,6 +810,7 @@ private void WriteProjectItems(IEnumerable items)
{
WriteDeduplicatedString(currentItemType);
WriteTaskItemList(reusableProjectItemList);
CheckForFilesToEmbed(currentItemType, reusableProjectItemList);
reusableProjectItemList.Clear();
}

Expand All @@ -815,6 +823,7 @@ private void WriteProjectItems(IEnumerable items)
{
WriteDeduplicatedString(currentItemType);
WriteTaskItemList(reusableProjectItemList);
CheckForFilesToEmbed(currentItemType, reusableProjectItemList);
reusableProjectItemList.Clear();
}

Expand All @@ -823,6 +832,28 @@ private void WriteProjectItems(IEnumerable items)
}
}

private void CheckForFilesToEmbed(string itemType, object itemList)
{
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to check the EmbedFile event for null here and bail right away?

Copy link
Member Author

Choose a reason for hiding this comment

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

In theory, but in our scenario we know it’s not null, so probably doesn’t matter either way. I was just using the ?.Invoke pattern out of habit.

Copy link
Member

Choose a reason for hiding this comment

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

Apologies for scrutinizing and hope I'm not misreading the code. EmbedFile is subscribed to always, yes, but the handler EventArgsWriter_EmbedFile is a conditional no-op. And that condition looks real, i.e. projectImportsCollector does not always get created. Are you sure we can't hoist the check and avoid running the loop here in some scenarios?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I see what you're saying. Sounds good.

Copy link
Member Author

Choose a reason for hiding this comment

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

How's this?

Copy link
Member

Choose a reason for hiding this comment

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

Awesome, thank you!

if (EmbedFile == null ||
!string.Equals(itemType, ItemTypeNames.EmbedInBinlog, StringComparison.OrdinalIgnoreCase) ||
itemList is not IEnumerable list)
{
return;
}

foreach (var item in list)
cdmihai marked this conversation as resolved.
Show resolved Hide resolved
{
if (item is ITaskItem taskItem && !string.IsNullOrEmpty(taskItem.ItemSpec))
cdmihai marked this conversation as resolved.
Show resolved Hide resolved
{
EmbedFile.Invoke(taskItem.ItemSpec);
}
else if (item is string itemSpec && !string.IsNullOrEmpty(itemSpec))
{
EmbedFile.Invoke(itemSpec);
}
}
}

private void Write(ITaskItem item, bool writeMetadata = true)
{
WriteDeduplicatedString(item.ItemSpec);
Expand Down
5 changes: 5 additions & 0 deletions src/Shared/Constants.cs
View file
Open in desktop
Expand Up @@ -141,6 +141,11 @@ internal static class ItemTypeNames
/// Declares a project cache plugin and its configuration.
/// </summary>
internal const string ProjectCachePlugin = nameof(ProjectCachePlugin);

/// <summary>
/// Embed specified files in the binary log
/// </summary>
internal const string EmbedInBinlog = nameof(EmbedInBinlog);
}

/// <summary>
Expand Down