-
Notifications
You must be signed in to change notification settings - Fork 842
Fix 99: remove restriction on static linking #1741
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
Conversation
KevinRansom
left a comment
There was a problem hiding this 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
|
@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. |
|
@KevinRansom I've added a test case here: ca264b7#diff-2dc1b4dc7af7a030af46349856047de0R1122 |
KevinRansom
left a comment
There was a problem hiding this 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
|
@KevinRansom OK will add that check, makes sense that way around |
|
OK, check added, thanks. |
|
This is great ... thanks |
|
Just to mention that the CI run got this unrelated error during testing: |
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