-
Notifications
You must be signed in to change notification settings - Fork 1
fix: Support libraries using TargetFrameworks property #12
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
Hello, Thanks for the PR could you please add tests too to check that everything works as expected for I don't think we need the full suit of test, the following should be enough I think:
|
Creating a test project like the others in the repo will work but won't demonstrate the fix. Using explicit The way I tested it manually was:
<configuration>
<packageSources>
<!-- The <clear/> line below removes the global NuGet package sources. -->
<clear />
<add key="nuget" value="https://api.nuget.org/v3/index.json" />
<add key="local" value="C:\Users\Robert\GitHub\sdk-test\packages" />
</packageSources>
<packageSourceMapping>
<!-- key value for <packageSource> should match key values from <packageSources> element -->
<packageSource key="nuget">
<package pattern="*" />
</packageSource>
<packageSource key="local">
<package pattern="Fable.Package.SDK" />
</packageSource>
</packageSourceMapping>
</configuration>
I'm not sure if there's a good way to automate this for a test? |
Even, by changing
We could automate all this setup, but this is probably more work for something which is not too worth. If the issue keep happening because we release regression then it could be worth considering in the future. Thanks for the PR, I will review it a little later |
Yes, I've published XParsec by using direct |
I've added some basic tests which should identify any regressions that would make the targets multi-tfm incompatible. I didn't go the whole way as I described in the manual testing. |
…abled the fix for this PR the test would still pass)
3515902
to
a9e4d65
Compare
Thanks for the PR, it has been released in 1.3.1 |
Looks like you went to the effort of the full test simulation. Nice work! |
Yes, I needed to do that because otherwise the test was succeeding even without your fix. |
Copies the
build
directory tobuildCrossTargeting
when creating packages so the SDK will work as expected with libraries usingTargetFrameworks
dotnet/msbuild#1860