search

dotnet / msbuild

The Microsoft Build Engine (MSBuild) is the build platform for .NET and Visual Studio.
https://docs.microsoft.com/visualstudio/msbuild/msbuild
MIT License
5.22k stars 1.35k forks source link

Sdk attribute on project causes import to fail #1451

Open natemcmaster opened 7 years ago

natemcmaster commented 7 years ago

Importing a project that has the Sdk attribute causes Import to fail.

Repro:

<!-- main.proj -->
<Project>
   <Import Project="otherproject.csproj" />
</Project>
<!-- otherproject.csproj -->
<Project Sdk="Microsoft.NET.Sdk">
</Project>

Run dotnet msbuild main.proj

Error:

C:\dev\dotnet\sdk\1.0.0-preview4-004215\Sdks\Microsoft.NET.Sdk\Sdk\Sdk.targets(35,3): error MSB4102: The value "" of the "Project" attribute in element is invalid. Parameter "path" cannot have zero length. [C:\dev\proj\main.proj]

natemcmaster commented 7 years ago

cc @AndyGerlicher @mlorbetske

rainersigwald commented 7 years ago

This should be forbidden but I didn't have time to build a nice error message for it for RC2.

rainersigwald commented 7 years ago

Although actually that failure is an SDK bug that arises when the project extension is neither .csproj nor .vbproj.

Rename main.proj -> main.csproj and you'll at least get a different error!

natemcmaster commented 7 years ago

This should be forbidden

Can you explain why it should be? Is there some reason it could not be made to work?

rainersigwald commented 7 years ago

I think it's really confusing to reason about where the implicit imports should be from imported files: do they unify, as other attributes like DefaultTargets do, and thus get imported at the top/bottom of the initial proj file, or do they just bracket the contents of the imported file (that's the current behavior, since there's no error message).

We built the Sdk attribute to simplify end-user project files. If you're writing build logic to be imported, you should use an explicit import.

natemcmaster commented 7 years ago

bracket the contents of the imported file

Seems most reasonable. The Sdk attribute is intended only for the scope of the file where its defined.

If you're writing build logic to be imported, you should use an explicit import.

For context, I'm writing a command line tool which needs to find property values in a csproj without using MSBuild APIs directly (because of https://github.com/Microsoft/msbuild/issues/1097) and without requiring an additional PackageReference to work.

rainersigwald commented 7 years ago

Seems most reasonable. The Sdk attribute is intended only for the scope of the file where its defined.

See, I'd go the other way and say if it were supported it should be unified. So I think it should just be banned, to avoid the confusion.