Skip to content

Conversation

@dsyme
Copy link
Contributor

@dsyme dsyme commented Nov 14, 2016

This is our oldest Urgency-Soon bug #99. There is simply no need for this restriction any more after previous fixes in this area. I've verified by hand that it fixes the problem and the resulting DLL is verifiable.

Also includes a bunch of further simplifications to the "fsharp" test suite

Copy link
Contributor

@KevinRansom KevinRansom left a comment

Choose a reason for hiding this comment

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

Don,

what is the behavior if the statically linked assembly uses an API that is not supported by the .net profile targeted by the compilation.

I would expect this will cause method and type missing exceptions to occur in that case. Which is probably not desirable.

Kevin

@dsyme
Copy link
Contributor Author

dsyme commented Nov 14, 2016

@KevinRansom Good question.

The specific case we're aiming to enable is static linking say a Profile 7 DLL into the compilation of a .NET 4.x component. That will work fine after this change (things like System.String are emitted as ELEMENT_TYPE_STRING, and new dependencies are injected on things like System.IO.dll and System.Threading.Tasks.dll). I can add a test for this case.

Now you're worried about the other way around: linking a .NET 4.x component into a Profile 7 component . However (when using an F# project file), MSBuild won't allow you to reference the .NET 4.x component from a Profile 7 component, so this should never happen.

So I think the compatibility checks basically are up to MSBuild, not the F# compiler.

@dsyme
Copy link
Contributor Author

dsyme commented Nov 15, 2016

@KevinRansom I've added a test case here: ca264b7#diff-2dc1b4dc7af7a030af46349856047de0R1122

Copy link
Contributor

@KevinRansom KevinRansom left a comment

Choose a reason for hiding this comment

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

If you are only interested in enabling importing netcore assemblies into desktop apps, then validate for that case rather than allowing both and hoping that the build environment will not allow it to happen.

Kevin

@dsyme
Copy link
Contributor Author

dsyme commented Nov 16, 2016

@KevinRansom OK will add that check, makes sense that way around

@dsyme
Copy link
Contributor Author

dsyme commented Nov 16, 2016

OK, check added, thanks.

@KevinRansom
Copy link
Contributor

This is great ... thanks

@dsyme
Copy link
Contributor Author

dsyme commented Nov 16, 2016

Just to mention that the CI run got this unrelated error during testing:

+++ ClrFx\PseudoCustomAttributes\AssemblyVersion (AssemblyVersion_001.fs) +++
Failed to Find Any Target:  
FAIL

@KevinRansom KevinRansom merged commit 34690eb into dotnet:master Nov 16, 2016
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.

3 participants