Skip to content

Don't cross compile GetAssemblyVersion #1215

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

Closed
wants to merge 1 commit into from

Conversation

ericstj
Copy link
Member

@ericstj ericstj commented May 15, 2017

Previously we cross-compiled the GetAssemblyVersion method to avoid
a dependency on MetadataReader on desktop. This had the downside of
causing first chance exceptions when run on desktop.

This has the tradeoff of introducing a Reflection.Metadata dependency on
desktop which will increase the layout by ~700KB (for SRM and SCI).

What are folks thoughts on that tradeoff? /cc @dsplaisted @KirillOsenkov @eerhardt

Previously we cross-compiled the GetAssemblyVersion method to avoid
a dependency on MetadataReader on desktop. This had the downside of
causing first chance exceptions when run on desktop.

This has the tradeoff of introducing a Reflection.Metadata dependency on
desktop which will increase the layout by ~700KB (for SRM and SCI).
@KirillOsenkov
Copy link
Member

I have a feeling that SRM will be used more and more in the future so might as well start taking advantage of it earlier.

@eerhardt
Copy link
Member

eerhardt commented May 16, 2017

What about doing this the opposite way? If we target netstandard2.0 in our build task, we can use the desktop code on both desktop and .net core.

Nevermind, I see we are trying to fix the first chance exception problem on desktop.

@ericstj
Copy link
Member Author

ericstj commented May 16, 2017

Tests are failing because S.C.I is missing: error MSB4018: System.IO.FileNotFoundException: Could not load file or assembly 'System.Collections.Immutable, Version=1.2.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a' or one of its dependencies. The system cannot find the file specified. I must have missed one of the places where I need to provide this... Looking

@ericstj
Copy link
Member Author

ericstj commented May 16, 2017

This is failing because of lack of bindingRedirects in SDK:

LOG: Attempting download of new URL file:///F:/dotnet/sdk/bin/Debug/Sdks/Microsoft.NET.Sdk/tools/net46/System.Collections.Immutable.DLL.
WRN: Comparing the assembly name resulted in the mismatch: Build Number

@ericstj
Copy link
Member Author

ericstj commented May 16, 2017

FWIW this is exactly why I was trying to avoid dependencies in the desktop build.

@nguerrera
Copy link
Contributor

dotnet/msbuild#1309

@nguerrera
Copy link
Contributor

I'm inclined to close this until msbuild provides a simple mechanism for binding redirects in tasks. I don't think 800kb + a custom assembly resolve implementation is worth it in the meantime.

@ericstj
Copy link
Member Author

ericstj commented May 17, 2017

Agreed. I suppose another alternative would be to implement our own early check for managed assembly using the code sample @KirillOsenkov shared: https://github.com/KirillOsenkov/MetadataTools/blob/master/PEFile/IsManagedAssembly.cs

But then I'd question if a second file open/read is worth getting rid of the first chance exception.

@nguerrera nguerrera closed this May 17, 2017
mmitche pushed a commit to mmitche/sdk that referenced this pull request Jun 5, 2020
…115.5 (dotnet#1215)

- Microsoft.DotNet.Arcade.Sdk - 5.0.0-beta.20065.5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants