Skip to content

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

Merged
forki merged 3 commits into
fsprojects:masterfrom
enricosada:netcoreapp21_and_20
Aug 16, 2018
Merged

netcoreapp2.0 packages are compatibile with netcoreapp2.1#3336
forki merged 3 commits into
fsprojects:masterfrom
enricosada:netcoreapp21_and_20

Conversation

@enricosada

@enricosada enricosada commented Aug 15, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

the netcoreapp2.1 framework support netcoreapp2.0 packages

matthid
matthid reviewed Aug 15, 2018
View reviewed changes
Comment thread src/Paket.Core/Versioning/FrameworkHandling.fs Outdated
| 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 ]

@matthid matthid Aug 15, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

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

@enricosada enricosada Aug 15, 2018

Copy link
Copy Markdown
Contributor 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

@matthid matthid left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nice catch

@matthid

matthid commented Aug 15, 2018

Copy link
Copy Markdown
Member

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

enricosada commented Aug 15, 2018

Copy link
Copy Markdown
Contributor 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

matthid commented Aug 15, 2018

Copy link
Copy Markdown
Member

Looks good to me. Thanks for the tests!

@enricosada enricosada mentioned this pull request Aug 15, 2018
Merged
@enricosada

enricosada commented Aug 15, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

@matthid

matthid commented Aug 15, 2018

Copy link
Copy Markdown
Member

I'll merge so when build is green.

Good luck with that ;)

@forki

forki commented Aug 16, 2018

Copy link
Copy Markdown
Member

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

@matthid

matthid commented Aug 16, 2018

Copy link
Copy Markdown
Member

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

1 more reviewer
@matthid matthid matthid approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@enricosada @matthid @forki