Skip to content
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.

Sign up for GitHub

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

Jump to bottom

netcoreapp2.0 packages are compatibile with netcoreapp2.1 #3336

Merged
merged 3 commits into from
Aug 16, 2018
Merged

netcoreapp2.0 packages are compatibile with netcoreapp2.1 #3336

merged 3 commits into from
Aug 16, 2018

Conversation

enricosada
Copy link
Collaborator

@enricosada enricosada commented Aug 15, 2018 •
edited
Loading

the netcoreapp2.1 framework support netcoreapp2.0 packages

matthid
matthid reviewed Aug 15, 2018
View reviewed changes
src/Paket.Core/Versioning/FrameworkHandling.fs Outdated
@@ -612,7 612,7 @@ type FrameworkIdentifier =
| DotNetCoreApp DotNetCoreAppVersion.V1_0 -> [ DotNetStandard DotNetStandardVersion.V1_6 ]
| DotNetCoreApp DotNetCoreAppVersion.V1_1 -> [ DotNetCoreApp DotNetCoreAppVersion.V1_0 ]
| DotNetCoreApp DotNetCoreAppVersion.V2_0 -> [ DotNetCoreApp DotNetCoreAppVersion.V1_1; DotNetStandard DotNetStandardVersion.V2_0 ]
| DotNetCoreApp DotNetCoreAppVersion.V2_1 -> [ DotNetCoreApp DotNetCoreAppVersion.V1_1; DotNetStandard DotNetStandardVersion.V2_0 ]
| DotNetCoreApp DotNetCoreAppVersion.V2_1 -> [ DotNetCoreApp DotNetCoreAppVersion.V2_0; DotNetStandard DotNetStandardVersion.V2_0 ]
Copy link
Member

@matthid matthid Aug 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I guess we can remove netstandard20 version here as netcoreapp2.0 already supports that

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sure, let me add a test netcoreapp2.1 netstandard2.0 and i'll remove it

matthid
matthid approved these changes Aug 15, 2018
View reviewed changes
Copy link
Member

@matthid matthid left a comment

Choose a reason for hiding this comment

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

nice catch

@matthid
Copy link
Member

matthid commented Aug 15, 2018

I'm curious: Did you notice an impact of this? It looks like something incredible hard to debug, but I can't think of what issues would arise from this...

@enricosada
Copy link
Collaborator Author

@matthid added cleaned up test

i found it working on my next PR, i just splitted it so is easier to review.
The issue was a target framework condition was not populated correctly, but yes, is really small impact (really few netcoreapp2.0 packages out there)

@matthid
Copy link
Member

matthid commented Aug 15, 2018

Looks good to me. Thanks for the tests!

@enricosada enricosada mentioned this pull request Aug 15, 2018
Merged
@enricosada
Copy link
Collaborator Author

enricosada commented Aug 15, 2018 •
edited
Loading

I'll merge so when build is green.
Yes, was really fun to debug 😄

@matthid
Copy link
Member

matthid commented Aug 15, 2018

I'll merge so when build is green.

Good luck with that ;)

@forki
Copy link
Member

forki commented Aug 16, 2018

it's rather important fix. I'll release. Thanks!

@matthid
Copy link
Member

matthid commented Aug 16, 2018

Honestly I'm a bit worried that our reviews didn't catch this, but good that it is solved now.

@forki forki merged commit 1ff1776 into fsprojects:master Aug 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthid matthid matthid approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@enricosada @matthid @forki