Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Reference Microsoft.NETCore.Targets #3621

Merged
merged 2 commits into from
Jan 29, 2018

Conversation

ericstj
Copy link
Member

@ericstj ericstj commented Jan 23, 2018

We want to make sure that Microsoft.NETCore.App has a reference to Microsoft.NETCore.Targets
so that the latest version is used and none of the old pre-2.0 runtime-specific packages get
brought into the package graph.

We were trying to do this before but it was failing because we weren't directly referencing the package.

In the future we could change this target to use the SDK assets file raising task then read the version
that was actually restored instead of forcing us to keep a direct reference to it.

Fixes #2716

@@ -34,6 +34,9 @@
<PackageReference Include="Microsoft.NETCore.Platforms">
<Version>$(MicrosoftNETCorePlatformsPackageVersion)</Version>
</PackageReference>
<PackageReference Include="Microsoft.NETCore.Targets">
<Version>2.0.0</Version>

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

Can we get the property defined for the version to follow the dependency flow pattern? That will help with product construction and source-build builds.

Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

Thanks for following up.

@eerhardt
Copy link
Member

@weshaggard - how is this going to work in source-build? especially if we stop building Microsoft.NETCore.Targets from corefx.

@ericstj ericstj force-pushed the reference.ms.nc.targets branch from 8c82c57 to 15d7ac4 Compare January 26, 2018 18:51
@ericstj
Copy link
Member Author

ericstj commented Jan 26, 2018

FWIW this 2.0.0 dependency already exists on the private NETCoreapp package that corefx produces. Also, this particular usage (emitting the dependency) doesn't require any resolution of the package at build time: it's just downstream folks who try to restore NETCore.App who would need the package.

@eerhardt
Copy link
Member

A mantra with the source-build effort is to not ship something that you didn't build from source. The high-level reasoning is that distro maintainers should be able to change anything and everything in our product for servicing reasons. See dotnet/source-build#187 for a lot of discussion on this topic.

So taking a dependency on a previously shipped package sort of falls into this crack. Of course we have this same issue elsewhere, so I wouldn't block this. But it would be good to know how this affects source-build.

@ericstj
Copy link
Member Author

ericstj commented Jan 26, 2018

We could always just leave it in corefx and build it at the stable version for source builds. Its ugly but it would work. Or I could continue to version it and build it even though it doesn't change.

@ericstj
Copy link
Member Author

ericstj commented Jan 29, 2018

@weshaggard what do you think about @eerhardt's question around versioning of the targets package?

We want to make sure that Microsoft.NETCore.App has a reference to Microsoft.NETCore.Targets
so that the latest version is used and none of the old pre-2.0 runtime-specific packages get
brought into the package graph.

We were trying to do this before but it was failing because we weren't directly referencing the package.

In the future we could change this target to use the SDK assets file raising task then read the version
that was actually restored instead of forcing us to keep a direct reference to it.
To be consistent with other dependencies we'll set this as a property.

We don't need the latest version since its had no changes since 2.0 RTM.  This is a dead-end
package that we're including merely to avoid having the pre-2.0 version included which would
bring in all the pre-2.0 runtime packages.
@ericstj ericstj force-pushed the reference.ms.nc.targets branch from 15d7ac4 to fd5b44a Compare January 29, 2018 22:52
@weshaggard
Copy link
Member

@weshaggard what do you think about @eerhardt's question around versioning of the targets package?

I would prefer we not build it live if it isn't changing any longer. We have a number of pre-built packages that we currently depend on. I think the worst case scenario is we build a dummy package but I don't think there is any way we get out of the business of having a set of pre-built packages, at least not anytime soon.

@eerhardt
Copy link
Member

Agreed. It's a larger issue than just NETCore.Targets. Just something we need to be aware of for source-build.

/cc @dotnet/source-build-contrib

@ericstj ericstj merged commit a2f2dc8 into dotnet:master Jan 29, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants