Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

Adding type-forwards for ValueTuple to desktop 4.7 #18519

Merged
merged 2 commits into from
Apr 20, 2017
Merged

Conversation

jcouv
Copy link
Member

@jcouv jcouv commented Apr 17, 2017

@weshaggard @ericstj @AlexGhiondea for review.

Fixes https://github.com/dotnet/corefx/issues/15175

I filed a follow-up issue (https://github.com/dotnet/corefx/issues/18518) to reference the shipped targeting pack instead of the pre-release one from myget.

Do you have any tips on validation?
So far, I inspected the produced package, and verified that it adds a "net47" folder under "lib" and "ref", and that it contains assemblies with type-forwards, and only referencing mscorlib.

Ref folder before:
image
And after change:
image

Lib folder before:
image
And after change;
image

@jcouv jcouv added this to the 2.0.0 milestone Apr 17, 2017
@jcouv jcouv self-assigned this Apr 17, 2017
@jcouv
Copy link
Member Author

jcouv commented Apr 18, 2017

@dotnet-bot test Innerloop PortableLinux Debug x64 Build and Test please

@ViktorHofer
Copy link
Member

@dotnet-bot test Innerloop PortableLinux Debug x64 Build and Test please

@weshaggard
Copy link
Member

Do you have any tips on validation?

I would suggest trying to add a reference to this local package from a .NET 4.7 console application or if we don't have the tooling for that yet, try at least taking the binary and consuming it on .NET 4.7.

@weshaggard
Copy link
Member

@ericstj how do you want to handle this in the desktop support package?

@ericstj
Copy link
Member

ericstj commented Apr 18, 2017

@weshaggard we need a separate build of both ref (shims.proj so that it unifies with ValueTuple in mscorlib) and lib to support net47. Without that we'd have type-conflicts at compile-time. Can you open an issue to track this?

I can imagine we'll have cases where folks build against 4.6.1 and run on 4.7: @jcouv can you confirm that'd work as expected?

@jcouv
Copy link
Member Author

jcouv commented Apr 19, 2017

So I couldn't create a project for a .NET 4.7 console app, because that framework isn't available in VS yet (although strangely it is already listed on this web page, which opens when you click "Install other frameworks…").

So instead, I extracted the ValueTuple ref dll from the package and built a simple console app referencing it (and also the .NET 4.7 mscorlib from GAC, which I got from down-level install). This worked fine and the resulting exe doesn't even reference the ValueTuple assembly (since it only had type-forwards).
Copying the ValueTuple lib dll next to my exe didn't interfere with execution either.

"C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\MSBuild\15.0\Bin\Roslyn\csc.exe" /noconfig  /nostdlib+  /reference:"C:\Windows\Microsoft.Net\assembly\GAC_32\mscorlib\v4.0_4.0.0.0__b77a5c561934e089\mscorlib.dll" /reference:"C:\Users\jcouv\Desktop\System.ValueTuple.4.4.0-preview1-25217-0\ref\net47\System.ValueTuple.dll" /target:exe Program.cs 

@weshaggard
Copy link
Member

One additional test you should do is to consume a library in your 4.7 console application that was built against System.ValueTuple and make sure it works as expected. I believe it will but just one extra layer of verification.

@ericstj
Copy link
Member

ericstj commented Apr 20, 2017

We need to add this to src project:

    <!-- We've explicitly authored a reference facade so we do not need to use the desktop implementation as ref -->
    <PackageDesktopAsRef>false</PackageDesktopAsRef>

@jcouv
Copy link
Member Author

jcouv commented Apr 20, 2017

@weshaggard The second scenario worked too, but I had to use assembly binding redirect so that the library (compiled against 4.0.1.0 VT.dll) would load the new 4.0.2.0 VT.dll.

Let me know if there is anything else. Otherwise, I'll go ahead and merge later today.

@weshaggard
Copy link
Member

Nothing else from my side. Thanks for testing.

@jcouv
Copy link
Member Author

jcouv commented Apr 20, 2017

I'll go ahead and merge. Thanks!

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

Successfully merging this pull request may close these issues.

6 participants