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

Removing "netcoreapp" and "netfx" TargetGroup #457

Merged
merged 38 commits into from
Dec 14, 2019
Merged

Removing "netcoreapp" and "netfx" TargetGroup #457

merged 38 commits into from
Dec 14, 2019

Conversation

Anipik
Copy link
Contributor

@Anipik Anipik commented Dec 2, 2019

Most of the work is done using regex. The commit messages contain the regex used for that commit.
I also introduced a new variable to track the latest version of netcoreapp, making it easier to increment for later version
.sln files will need to be updated with every new major product version.

Regexs used
netcoreapp -> netcoreapp5.0 in .sln
netcoreapp; -> netcoreapp5.0; in .props
netcoreapp- -> netcoreapp5.0- in .props
netcoreapp- -> netcoreapp5.0- in .csproj
'netcoreapp -> 'netcoreapp5.0 in .csproj

netcoreapp5.0 occurences are then replaced using the above regex by TargetFrameworkVNext

Same stuff has been done for netfx

@ViktorHofer
Copy link
Member

This collides with the work being done in #445. Can you either pause this until the other PR is in or use the same TFM name so that we can simply replace the property define here after the other one is in?

@Anipik
Copy link
Contributor Author

Anipik commented Dec 2, 2019

Sure I can wait until the #445 is in

Directory.Build.props Outdated Show resolved Hide resolved
@ViktorHofer
Copy link
Member

We agreed on NetCoreAppCurrentTargetFrameworkMoniker (.NETCoreApp,Version=v5.0) and NetCoreAppCurrent (netcoreapp5.0) in #445.

@ViktorHofer
Copy link
Member

@Anipik this work is now unblocked.

@@ -114,8 +114,8 @@ jobs:
- task: CopyFiles@2
displayName: Prepare framework runtime folder to publish
inputs:
sourceFolder: $(Build.SourcesDirectory)/artifacts/bin/pkg/${{ parameters.framework }}/runtime
targetFolder: $(Build.ArtifactStagingDirectory)/artifacts/bin/pkg/${{ parameters.framework }}/runtime
sourceFolder: $(Build.SourcesDirectory)/artifacts/bin/pkg/netcoreapp5.0/runtime # The hardcoded target framework should be removed when we drop the support for versionless targetframeworks from ci.
Copy link
Member

Choose a reason for hiding this comment

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

eng/packaging.props Outdated Show resolved Hide resolved
@jkoritzinsky
Copy link
Member

I'd prefer if I could get #494 in before this if I can to unblock cross-cutting PRs instead of having to react to this and delaying merging #494 another day or so.

@safern
Copy link
Member

safern commented Dec 12, 2019

I'd prefer if I could get #494 in before this if I can to unblock cross-cutting PRs

I definitely agree #494 has higher priority since that unblocks live live workflow.

@Anipik
Copy link
Contributor Author

Anipik commented Dec 12, 2019

sure i can wait. let me know when i can go ahead and merge this one.

Directory.Build.props Outdated Show resolved Hide resolved
@ViktorHofer
Copy link
Member

Actually I just saw in how many places we hardcode net472 and I'm not really happy about that. As we are already touching so many files as part of this PR I would introduce a property for it NetFrameworkCurrent as well as part of this PR. As this PR is currently blocked by other work we should have some time for it. I can help too.

@Anipik
Copy link
Contributor Author

Anipik commented Dec 13, 2019

@ViktorHofer as we are not gonna increase the netframework version, what will be the advantages of having this new property ?

@Anipik
Copy link
Contributor Author

Anipik commented Dec 13, 2019

@jkoritzinsky @safern @ViktorHofer
Do we need to add build coreclr step for the runtime-libraries enterprise-linux ?

  /repo/eng/liveBuilds.targets(20,5): error : The CoreCLR artifacts path does not exist '/repo/artifacts/bin/coreclr/Linux.x64.Debug/'. The CoreCLR subset category must be built before building this project. [/repo/src/libraries/System.AppContext/src/System.AppContext.csproj]
/repo/eng/liveBuilds.targets(20,5): error : The CoreCLR artifacts path does not exist '/repo/artifacts/bin/coreclr/Linux.x64.Debug/'. The CoreCLR subset category must be built before building this project. [/repo/src/libraries/System.Buffers/src/System.Buffers.csproj]
/repo/eng/liveBuilds.targets(20,5): error : The CoreCLR artifacts path does not exist '/repo/artifacts/bin/coreclr/Linux.x64.Debug/'. The CoreCLR subset category must be built before building this project. [/repo/src/libraries/System.Private.Uri/src/System.Private.Uri.csproj]
/repo/eng/liveBuilds.targets(20,5): error : The CoreCLR artifacts path does not exist '/repo/artifacts/bin/coreclr/Linux.x64.Debug/'. The CoreCLR subset category must be built before building this project. [/repo/src/libraries/System.Diagnostics.Debug/src/System.Diagnostics.Debug.csproj]
/repo/eng/liveBuilds.targets(20,5): error : The CoreCLR artifacts path does not exist '/repo/artifacts/bin/coreclr/Linux.x64.Debug/'. The CoreCLR subset category must be built before building this project. [/repo/src/libraries/System.Diagnostics.Contracts/src/System.Diagnostics.Contracts.csproj]
/rep

@jkoritzinsky
Copy link
Member

Yes. You'll need to add a job to build coreclr for that pipeline now that live-live is enabled.

@Anipik
Copy link
Contributor Author

Anipik commented Dec 14, 2019

@safern @ViktorHofer @ericstj can we go ahead and merge this one ?
linux failure is not related to this one .

@davidsh
Copy link
Contributor

davidsh commented Dec 14, 2019

runtime-libraries enterprise-linux — #20191213.7 failed

This has been fixed already with PR #843 which was merged in the last hour.

@Anipik
Copy link
Contributor Author

Anipik commented Dec 14, 2019

Thanks @davidsh

@Anipik
Copy link
Contributor Author

Anipik commented Dec 14, 2019

This one is ready to go in. the merging is blocked and require an approval

@Anipik Anipik merged commit 3b9abae into dotnet:master Dec 14, 2019
@Anipik Anipik deleted the vnext branch February 24, 2020 23:37
@ghost ghost locked as resolved and limited conversation to collaborators Dec 11, 2020
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.

9 participants