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

Accept TaskOutput when filtering events for EmbedInBinlog #7869

Merged
merged 3 commits into from
Aug 16, 2022
Merged

Accept TaskOutput when filtering events for EmbedInBinlog #7869

merged 3 commits into from
Aug 16, 2022

Conversation

MeikTranel
Copy link
Contributor

Before we only interpreted plain AddItem messages as directives for file embedding.

Resolves #7665

Changes Made

Added TaskParameterEvents with kind TaskOutput to the event filter for embedding files.

Testing

I started out writing tests to catch a dedicated FileImported, because i thought the file was embedded as a payload inside a BuildEvent somehow, but if i get this correctly the archive is just a secondary payload to the binlog - meant for readers to use as a dictionary to look for files without any guarantees that files are actually there.
I'm not actually digging inside the .binlog file, but i found reading the external *.ProjectImports.zip to be an acceptable alternative to get at least some coverage regarding EmbedInBinlog resulting in embedded files.

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.

Looks like the unit tests are failing at line 165. I haven't looked too closely, but I'd guess either the string equality is failing (try Contains to check?) or the file is somewhere unexpected. Not sure how to check the latter.

Comment on lines 155 to 156
<WriteLinesToFile File=""./testtaskoutputfile.txt"" Lines=""abc;def;ghi""/>
<CreateItem Include=""./testtaskoutputfile.txt"">
Copy link
Member

Choose a reason for hiding this comment

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

I imagine these slashes don't work on windows?

Copy link
Member

Choose a reason for hiding this comment

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

I think the convention is to use the \ on all OSs (and MSBuild will convert correctly where needed).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will look at this later today - don't want to use build infrastructure to potentially back and forth on test success.

ObjectModelHelpers.BuildProjectExpectSuccess(testProject, binaryLogger);
var projectImportsZipPath = Path.ChangeExtension(_logFile, ".ProjectImports.zip");
using var fileStream = new FileStream(projectImportsZipPath, FileMode.Open);
using var zipArchive = new ZipArchive(fileStream, ZipArchiveMode.Read);
Copy link
Member

Choose a reason for hiding this comment

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

Consider using TestEnvironment and TransientZipArchive?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TransientZipArchive is only for creating a zip archive not the other way around and i don't think TestEnvironment would help here really - ObjectModelHelpers.BuildProjectExpectSuccess already uses it internally, the _logFile path is attached to it implicitly - pretty much the only path i produce is the .ProjectImports.zip, which is just conventionally attached to the binlog path.

@MeikTranel
Copy link
Contributor Author

MeikTranel commented Aug 9, 2022

I can just drop the explicit path - that fixes building on both windows and linux.
More importantly now that the feature itself works - i just tested via WSL and it fails on the zip assertion because the Name is the full path instead of just the file name + extension. So i'm asking myself here - is this normal behavior with the internal ZipArchive on linux/debian/wsl ?
image
image

…r EmbedInBinlog

Before we only interpreted plain AddItem messages as directives for file embedding.

Resolves #7665
@Forgind
Copy link
Member

Forgind commented Aug 9, 2022

I looked at documentation for ZipArchive.

It explicitly states that it looks after the last path separator character (\). I'm guessing it hasn't been made unix-friendly and doesn't realize that there are other slashes 😄

@MeikTranel
Copy link
Contributor Author

MeikTranel commented Aug 9, 2022

Oh my lord - 😂😂 - well imma have to work around that now 🥳🥳 i guess .EndsWith works.

@KirillOsenkov i assume this doesn't show up on your end because you are working with the fully qualified path anyways?

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.

LGTM, thanks @MeikTranel!

Copy link
Member

@benvillalobos benvillalobos left a comment

Choose a reason for hiding this comment

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

LGTM

…rmance

Co-authored-by: Forgind <Forgind@users.noreply.github.com>
@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 Aug 15, 2022
Copy link
Member

@KirillOsenkov KirillOsenkov left a comment

Choose a reason for hiding this comment

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

👍

@benvillalobos benvillalobos changed the title Accept TaskParameterMessageKind.TaskOutput when filtering events for EmbedInBinlog Accept TaskOutput when filtering events for EmbedInBinlog Aug 16, 2022
@benvillalobos benvillalobos merged commit 5d102ae into dotnet:main Aug 16, 2022
@benvillalobos
Copy link
Member

Thanks for the contribution!

@MeikTranel MeikTranel deleted the wip/fix-EmbedInBinlog-for-taskoutputs branch August 17, 2022 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

Items returned by tasks not considered for EmbedInBinlog
4 participants