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.

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

[release/6.0] Don't special case "Experimental" projects #60643

Merged
merged 2 commits into from
Oct 19, 2021

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Oct 19, 2021

Backport of #60641 to release/6.0

/cc @ericstj

Infra change to build Experimental projects with correct version.

We only have one experimental project, System.Runtime.Experimental and
we want it to behave like any other package.
@ghost
Copy link

ghost commented Oct 19, 2021

Tagging subscribers to this area: @Anipik, @safern, @ViktorHofer
See info in area-owners.md if you want to be subscribed.

Issue Details

Backport of #60641 to release/6.0

/cc @ericstj

Customer Impact

Testing

Risk

Author: github-actions[bot]
Assignees: -
Labels:

area-Infrastructure-libraries

Milestone: -

@ericstj
Copy link
Member

ericstj commented Oct 19, 2021

Interesting, this is failing only in release/6.0:

/__w/1/s/.packages/microsoft.dotnet.build.tasks.packaging/6.0.0-beta.21514.2/build/Packaging.targets(800,5): error : No VersionSuffix was set. Ensure it is set before targets in packaging are ran. [/__w/1/s/src/coreclr/.nuget/ILCompiler.Reflection.ReadyToRun.Experimental/ILCompiler.Reflection.ReadyToRun.Experimental.pkgproj]
##[error].packages/microsoft.dotnet.build.tasks.packaging/6.0.0-beta.21514.2/build/Packaging.targets(800,5): error : (NETCORE_ENGINEERING_TELEMETRY=Build) No VersionSuffix was set. Ensure it is set before targets in packaging are ran.

Due to

<StableVersion Condition="($(MSBuildProjectName.Contains('Private')) or $(MSBuildProjectName.Contains('Experimental')) or $(MSBuildProjectName.Contains('Transport'))) and '$(MSBuildProjectExtension)' == '.pkgproj'"></StableVersion>

Do we know what the intent is for ILCompiler.Reflection.ReadyToRun.Experimental? @cshung @MichalStrehovsky
Looks like this has never been pushed to nuget.org, so my guess is this should be treated as non-shipping.

@safern
Copy link
Member

safern commented Oct 19, 2021

IMHO I think we should leave the logic for Experimental packages as it was and then if a specific experimental package that we want it to make it to NuGet.org and stabilized, should override these properties on the project file directly.

We have gone back and forth removing this and bringing it back because sometimes we want to ship some and sometimes we don't but the consensus we made for 5.0 was that the experimental packages in dotnet/runtime usually don't ship stable and don't ship to NuGet (of course this was before dotnet/runtimelab, so that might change the perspective).

@ericstj
Copy link
Member

ericstj commented Oct 19, 2021

The fact that we go back and forth so much means that there's probably no grounds for a name-based convention. Why not make folks call it out explicitly? The problem here is that there were two places that were trying to capture this behavior and they got out of sync.

Can we just make the project's set IsShipping to declare their behavior?

@safern
Copy link
Member

safern commented Oct 19, 2021

Can we just make the project's set IsShipping to declare their behavior?

Sure they would need to set IsShippingPackage=false and SuppressFinalPackageVersion=true in the package if we want this to be opt-in rather than opt-out.

But I see that, there is yet another package: ILCompiler.Reflection.ReadyToRun.Experimental that depends on this logic. So we should fix that project by setting these 2 properties and just remove the default behavior for projects that have Experimental on its name.

I believe Experimental packages in runtime should be treated as non shipping by default, and if there is an exception we should treat it as opt-out by setting IsShippingPackage=true and SuppressFinalPackageVersion=false in those projects (in this case System.Runtime.Experimental). cc: @jkotas as he suggested we did what is being reverted here in 5.0 days.

@Anipik
Copy link
Contributor

Anipik commented Oct 19, 2021

I think for 6.0 we should just IsShippingPackage=true and SuppressFinalPackageVersion=false in the ref project of S.R.Experimental.
It will then be pretty limited scope change and reduce the risk at this stage.

@ericstj
Copy link
Member

ericstj commented Oct 19, 2021

I think some of this thinking changed since 5.0, since we added the ability to ship experimental functionality in the product. I'm OK with whatever folks decide to unblock the build. I pushed another commit which I think cleans up the naming conventions in coreclr. Feel free to merge or close this PR. I submitted to try to help unblock 6.0 stabalization.

@safern
Copy link
Member

safern commented Oct 19, 2021

Thanks, @ericstj I was just adding my 2c, but yeah I do agree that the thinking changed after 5.0. I'm fine with how the change is now. We just need to make sure to remember that if or some reason we add an Experimental package that we don't want to ship to NuGet at some point on 7 an onwards, to set the properties accordingly on that project.

@mmitche mmitche merged commit 80ed1bd into release/6.0 Oct 19, 2021
@safern safern deleted the backport/pr-60641-to-release/6.0 branch October 19, 2021 23:34
@MichalStrehovsky
Copy link
Member

Just wanted to make sure - ILCompiler.Reflection.ReadyToRun.Experimental will not end up on NuGet.org, is that correct?

We don't have any versioning/security/etc. guarantees around this package. This package is shipped as-is for ILSpy to consume and the expectations communicated there. See #41379.

@safern
Copy link
Member

safern commented Oct 20, 2021

Just wanted to make sure - ILCompiler.Reflection.ReadyToRun.Experimental will not end up on NuGet.org, is that correct?

Yes that is correct, that is one of the things the false flag indicates.

@ghost ghost locked as resolved and limited conversation to collaborators Nov 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants