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

Add Cargo Build Sdk #599

Merged
merged 29 commits into from
Jan 17, 2025
Merged

Conversation

novacole
Copy link
Contributor

@novacole novacole commented Jan 9, 2025

This sdk adds support for building rust projects with cargo within MSBuild.

src/CargoBuild/README.md Outdated Show resolved Hide resolved
</PropertyGroup>

<PropertyGroup>
<!-- Copy logic to know if managed targets got imported: https://github.com/dotnet/sdk/blob/49002c14cf91ecd08e79d6184dbd4716c005b509/src/Tasks/Microsoft.NET.Build.Tasks/sdk/Sdk.targets#L25-L27 -->
Copy link
Member

Choose a reason for hiding this comment

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

Can the comment explain why?

So, is it expected that a cargobuild project would be named "csproj"?

src/CargoBuild/README.md Outdated Show resolved Hide resolved
src/CargoBuild/README.md Outdated Show resolved Hide resolved
src/CargoBuild/README.md Outdated Show resolved Hide resolved
src/CargoBuild/README.md Outdated Show resolved Hide resolved
src/CargoBuild/README.md Outdated Show resolved Hide resolved
src/CargoBuild/README.md Outdated Show resolved Hide resolved
src/CargoBuild/README.md Outdated Show resolved Hide resolved
src/CargoBuild/README.md Outdated Show resolved Hide resolved
@CZEMacLeod
Copy link

Might have already been discussed, and I don't really use rust, but it seems odd to have build in the name twice.
`Microsoft.Build.Rust.Cargo' or similar would make it much more obvious what this sdk is about.
Again for the project type extension - given vbproj, csproj, etc. perhaps rsproj would make more sense.
Just some food for thought...

@AndyGerlicher
Copy link
Member

Might have already been discussed, and I don't really use rust, but it seems odd to have build in the name twice. `Microsoft.Build.Rust.Cargo' or similar would make it much more obvious what this sdk is about. Again for the project type extension - given vbproj, csproj, etc. perhaps rsproj would make more sense. Just some food for thought...

Good feedback. We did discuss this a bit. Originally had Rust in the name and rsproj, but the decision was to focus on the integration between MSBuild and Cargo. Having it too focused on Rust (or .rsproj) implies more language integration and that there might be Visual Studio support, which is not what this is about. This is just a bridge between MSBuild and Cargo so you can have a single graph and get the benefits of that (caching, etc.). Make more sense given the additional context?

@CZEMacLeod
Copy link

Good feedback. We did discuss this a bit. Originally had Rust in the name and rsproj, but the decision was to focus on the integration between MSBuild and Cargo. Having it too focused on Rust (or .rsproj) implies more language integration and that there might be Visual Studio support, which is not what this is about. This is just a bridge between MSBuild and Cargo so you can have a single graph and get the benefits of that (caching, etc.). Make more sense given the additional context?

Fair enough. From an outsider's perspective it seems a little odd - given in the future that an LSP might be available, or you might open a solution including this with VSCode, where better rust integration could be available.
I understand that this isn't first party for VisualStudio though so .cargoproj makes some sense.
To me, if you are focusing on that though, the name should be aligned through-out: .cargoproj and Microsoft.Build.Cargo. Repeating the Build just looked weird.

Does this only output Nuget packages for libraries, or can it build executables (such that it could be referenced by Aspire)?

How does this compare to the Android C++/Java Gradle build project build which is about the closest thing I can think of.

Anyway, it looks like you've already thought about this so I'll but out, and leave you to finish this impressive bit of code.

AndyGerlicher
AndyGerlicher previously approved these changes Jan 17, 2025
@AndyGerlicher AndyGerlicher merged commit 1392e8d into microsoft:main Jan 17, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants