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

Embed project.assets.json in binlog #16840

Merged

Conversation

KirillOsenkov
Copy link
Member

We're thinking of adding a way to embed arbitrary files in the binlog.
See dotnet/msbuild#6339

project.assets.json is commonly requested to be embedded. This would fix dotnet/msbuild#3529

Binlog size does increase from 3.5 MB -> 5 MB, so we'll need to think perhaps about making it off by default.

We're thinking of adding a way to embed arbitrary files in the binlog.
See dotnet/msbuild#6339

project.assets.json is commonly requested to be embedded. This would fix dotnet/msbuild#3529

Binlog size does increase from 3.5 MB -> 5 MB, so we'll need to think perhaps about making it off by default.
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@cdmihai
Copy link
Contributor

cdmihai commented Apr 15, 2021

Given that it's super useful to have the project.asset.jsons in the binlog, and given that you've optimized and reduced the binlog size way beyond the extra size added by the jsons, I think it's worth having it on by default.

@KirillOsenkov KirillOsenkov marked this pull request as ready for review April 19, 2021 05:00
@KirillOsenkov KirillOsenkov removed the WIP label Apr 19, 2021
@marcpopMSFT
Copy link
Member

@KirillOsenkov is there anything in the project.assetts.json file that is more risk from a privacy perspective than that's already included in the binlogs? Do we need tests in this area as we don't have existing tests for binlogs in the sdk?

@KirillOsenkov
Copy link
Member Author

Nothing in project.assets.json that could be PII as far as I can think. It does include full paths to .csproj files, but those are already in the binlog.

Don't think we need tests for this case, as it's literally an item group and it's non-essential. There's bigger areas in MSBuild that are in more need of tests so I'd say let's spend efforts there instead.

@agocke
Copy link
Member

agocke commented Apr 6, 2022

This change just helped me work around a race condition that's been plaguing me for days. Great change, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Find a way to embed project.assets.json in .binlog
4 participants