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.

Sign up for GitHub

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

Jump to bottom

Remove bootstrap.proj #5070

Closed
wants to merge 5 commits into from
Closed

Remove bootstrap.proj #5070

wants to merge 5 commits into from

Conversation

jeffkl
Copy link
Contributor

@jeffkl jeffkl commented Feb 28, 2023 •
edited
Loading

Bug

Fixes: https://github.com/NuGet/Client.Engineering/issues/2186

Regression? Last working version:

Description

This removes bootstrap.proj and replaces it with more retail MSBuild logic and allows a user to clone our repository and build it as long as they have Visual Studio and .NET SDK installed.

PR Checklist

  • PR has a meaningful title

  • PR has a linked issue.

  • Described changes

  • Tests

    • Automated tests added
    • OR
    • Test exception
    • OR
    • N/A

  • Documentation

    • Documentation PR or issue filled
    • OR
    • N/A

@jeffkl jeffkl self-assigned this Feb 28, 2023
@jeffkl jeffkl force-pushed the dev-jeffkl-reduce-bootstrap branch from fc50b25 to 1095f99 Compare February 28, 2023 19:16
zivkan
zivkan reviewed Mar 1, 2023
View reviewed changes
build.ps1 Show resolved Hide resolved
@jeffkl jeffkl force-pushed the dev-jeffkl-reduce-bootstrap branch from 1095f99 to 3a53e3d Compare March 7, 2023 20:43
@jeffkl jeffkl changed the title [Draft]: Remove bootstrap.proj Remove bootstrap.proj Mar 7, 2023
@jeffkl jeffkl marked this pull request as ready for review March 7, 2023 20:58
@jeffkl jeffkl requested a review from a team as a code owner March 7, 2023 20:58
nkolev92
nkolev92 reviewed Mar 7, 2023
View reviewed changes
Copy link
Member

@nkolev92 nkolev92 left a comment

Choose a reason for hiding this comment

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

Love the overall intention.

Would it be easier to have 1 PR to minimize the risk of the changes?
All the changes are easy to follow individually, but there's a lot :D

It would be helpful to summarize the changes in the PR description as well.

Have you verified that the outputs generated by this changes are equivalent before/after?

build/BuildValidator.proj
@@ -0,0 +1,63 @@
<Project Sdk="Microsoft.Build.NoTargets">
Copy link
Member

Choose a reason for hiding this comment

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

Can we name this LocalizationValidator or something like that/

I know the package is not named super well, but we can do better with the project.

build/loc.proj
@@ -99,7 +106,4 @@
<Copy SourceFiles="@(_EnglishBinaries)" DestinationFiles="@(_EnglishBinaries->'%(DestinationPath)')" />
<Copy SourceFiles="@(_LocalizeFiles)" DestinationFiles="@(_LocalizeFiles->'%(DestinationPath)')" />
</Target>

<Target Name="AfterBuild" DependsOnTargets="LocalizeNonProjectFiles;BatchLocalize"/>
<Import Project="$(MicroBuildDirectory)Microsoft.VisualStudioEng.MicroBuild.Core.targets" />
Copy link
Member

Choose a reason for hiding this comment

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

How are these targets included now?

setup/After.Microsoft.Common.targets
@@ -0,0 +1,6 @@
<Project>
<PropertyGroup Condition="'$(MSBuildProjectExtension)' == '.vsmanproj'">
<OutputType />
Copy link
Member

Choose a reason for hiding this comment

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

I'm imaging it's getting overwritten if we put it in the project file itself?

@ghost ghost added the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Mar 15, 2023
@ghost
Copy link

ghost commented Mar 15, 2023

This PR has been automatically marked as stale because it has no activity for 7 days. It will be closed if no further activity occurs within another 7 days of this comment. If it is closed, you may reopen it anytime when you're ready again, as long as you don't delete the branch.

@jeffkl jeffkl removed the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Mar 20, 2023
@jeffkl jeffkl force-pushed the dev-jeffkl-reduce-bootstrap branch from 3a53e3d to b583ead Compare March 20, 2023 18:38
@ghost ghost added the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Mar 27, 2023
@ghost
Copy link

ghost commented Mar 27, 2023

This PR has been automatically marked as stale because it has no activity for 7 days. It will be closed if no further activity occurs within another 7 days of this comment. If it is closed, you may reopen it anytime when you're ready again, as long as you don't delete the branch.

@ghost ghost closed this Apr 3, 2023
@jeffkl jeffkl removed the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Apr 20, 2023
@jeffkl jeffkl mentioned this pull request Apr 20, 2023
Merged
8 tasks
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zivkan zivkan zivkan left review comments

@nkolev92 nkolev92 nkolev92 left review comments

Assignees

@jeffkl jeffkl

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jeffkl @nkolev92 @zivkan