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

Share custom package infra among tools projects #46098

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ellahathaway
Copy link
Member

Needed for new tooling that will be added as part of dotnet/source-build#4736

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Infrastructure untriaged Request triage from a team member labels Jan 17, 2025
@ellahathaway ellahathaway marked this pull request as ready for review January 17, 2025 19:29
@ellahathaway ellahathaway requested review from a team as code owners January 17, 2025 19:29
@@ -1,6 +1,7 @@
<Project>

<Import Project="$([MSBuild]::GetPathOfFileAbove(Directory.Build.props, $(MSBuildThisFileDirectory)..))" />
<Import Project="$(CustomPackageVersionsProps)" Condition="'$(CustomPackageVersionsProps)' != ''" />
Copy link
Member

Choose a reason for hiding this comment

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

There are other existing tools in this directory besides BinaryToolKit. It would be good to not add more complexity to those tools (especially the msbuild tasks). Can you describe why you can't copy the Import in BinaryToolKit into the new project that will get added?

Aside, I see some benefit in having only a single entry point project for these different kind of permutations. Would it make sense to rename BinaryToolKit to something more generic and make it possible to invoke it for the different scenarios? If you use System.CommandLine it has support for subcommands which might be appropriate here.

Copy link
Member Author

@ellahathaway ellahathaway Jan 17, 2025

Choose a reason for hiding this comment

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

Can you describe why you can't copy the Import in BinaryToolKit into the new project that will get added?

I want to be able to invoke the standalone CLI tools in eng/tools via a single script which takes --tool <tool> and optionally --with-packages and --with-sdk as arguments. It felt complex to try to condition the --with-packages arg to only be allowed when the tool supports the CustomPackageVersionsProps prop. So, rather than condition the script's --with-packages arg, I felt that it would be best to allow the projects in this directory to have the ability to optionally use the prop + infra.

I see some benefit in having only a single entry point project for these different kind of permutations

I see your point in this. I'm open to moving the current standalone tools (CreateBaselineUpdatePR, BinaryToolKit, LicenseRemover) into a new directory with shared targets and props, but I'm hesitant to move them all into a single project because the tools aren't all directly related in their actions/purposes.

Copy link
Member

Choose a reason for hiding this comment

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

but I'm hesitant to move them all into a single project because the tools aren't all directly related in their actions/purposes.

Agreed, the CreateBaselineUpdatePR one doesn't make sense to merge with the others. In general see value in having one managed "prep build tool". That would mean merging BinaryToolKit and LicenseRemover together. I assume that they already share some basic knowledge on how to traverse the source graph and exclusions handling. Just a suggestion as I think a common managed project would make things more scalable in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Infrastructure untriaged Request triage from a team member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants