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

Delete task code that has been moved to the shared framework SDK in Arcade #35405

Merged
merged 4 commits into from
May 8, 2020

Conversation

v-chayan
Copy link
Contributor

Deleted below files that were moved to Arcade.
BuildFPMToolPreReqs.cs
CopyNupkgAndChangeVersion.cs
CreateFrameworkListFile.cs
ExecWithRetries.cs
GenerateDebRepoUploadJsonFile.cs
GenerateJsonObjectString.cs
ProcessSharedFrameworkDeps.cs
RuntimeGraphManager.cs
RuntimeReference.cs
StabilizeWixFileId.cs
ZipFileExtractToDirectory.cs
ZipFileGetEntries.cs

@ghost
Copy link

ghost commented Apr 24, 2020

Tagging subscribers to this area: @ViktorHofer
Notify danmosemsft if you want to be subscribed.

@ViktorHofer
Copy link
Member

The regenerate readme files are also obsolete and can be removed :)

@v-chayan
Copy link
Contributor Author

The regenerate readme files are also obsolete and can be removed :)

I didn't find a copy of RegenerateReadmeTable.cs in Arcade repo. So I didn't remove that file according to the instruction from #34907
I am trying to build again after removed this file to ensure it was really not needed. Will update later.

@ViktorHofer
Copy link
Member

I didn't find a copy of RegenerateReadmeTable.cs in Arcade repo.

Right, because we are now following a different approach where we are dynamically generating/updating aka.ms links so there's no need for the regeneration of the readme file anymore. We still haven't added the build table back to the README.md though. It should be safe to delete this.

@dagood
Copy link
Member

dagood commented Apr 24, 2020

Right, because we are now following a different approach where we are dynamically generating/updating aka.ms links so there's no need for the regeneration of the readme file anymore. We still haven't added the build table back to the README.md though. It should be safe to delete this.

RegenerateReadmeTable is unrelated to aka.ms link generation, it's just a way to help humans manually update the table when the list of platforms or branches changes, through basic templating. Arcade may help generate readme tables at some point, but I don't think we should remove RegenerateReadmeTable and wait for that. #32055 is delayed enough as it is.

@dagood
Copy link
Member

dagood commented Apr 24, 2020

The regenerate readme files are also obsolete and can be removed :)

I didn't find a copy of RegenerateReadmeTable.cs in Arcade repo. So I didn't remove that file according to the instruction from #34907
I am trying to build again after removed this file to ensure it was really not needed. Will update later.

I recommend not doing this--at the very least, not as part of this PR. The task isn't exercised as part of the builds, and this is intentional. It also looks like the normal entrypoint disappeared in 42183b1#diff-f1921dfe4ae191cbfc80c608aa17dd4a.

@dagood
Copy link
Member

dagood commented Apr 24, 2020

Right, because we are now following a different approach where we are dynamically generating/updating aka.ms links so there's no need for the regeneration of the readme file anymore.

I see, it sounds like you're under the impression that the readme was updated for every official build? This actually hasn't changed with aka.ms link generation: the links were always being dynamically updated and the table text was always static. There's nothing new here with aka.ms links.

Copy link
Member

@dagood dagood left a comment

Choose a reason for hiding this comment

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

@ViktorHofer
Copy link
Member

I see, it sounds like you're under the impression that the readme was updated for every official build?

Right, I thought that was the case and the regenerate-readme-table.proj was used to update it and flow it back into the public repo somehow. Sounds like the proj simply avoids the hand-editing of the build table.

@v-chayan
Copy link
Contributor Author

The regenerate readme files are also obsolete and can be removed :)

I didn't find a copy of RegenerateReadmeTable.cs in Arcade repo. So I didn't remove that file according to the instruction from #34907
I am trying to build again after removed this file to ensure it was really not needed. Will update later.

I recommend not doing this--at the very least, not as part of this PR. The task isn't exercised as part of the builds, and this is intentional. It also looks like the normal entrypoint disappeared in 42183b1#diff-f1921dfe4ae191cbfc80c608aa17dd4a.

I've reverted the change. Thanks for pointing this out!

@v-chayan v-chayan requested review from ViktorHofer and dagood April 24, 2020 14:45
@ViktorHofer
Copy link
Member

What about GetTargetMachineInfo?

@v-chayan
Copy link
Contributor Author

What about GetTargetMachineInfo?

This file is still used. Removing it will cause below build error:
F:\GitHub\runtime\tools-local\tasks\installer.tasks\installer.tasks.csproj(52,5): error MSB4062: The "GetTargetMachineInfo" task could not be loaded from the assembly F:\GitHub\runtime\artifacts\bin\installer.tasks\Debug\netstandard2.0\installer.tasks.dll. Confirm that the declaration is correct, that the assembly and all its dependencies are available, and that the task contains a public class that implements Microsoft.Build.Framework.ITask.

@ViktorHofer
Copy link
Member

Ok, I'll deal with it in #35538.

@v-chayan v-chayan merged commit 4804b8d into dotnet:master May 8, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 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.

5 participants