Skip to content

Ensure that a virtual project doesn't pick up BaseOutputPath/BaseIntermediateOutputPath from Directory.Build.props #51600

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

Open
wants to merge 1 commit into
base: release/10.0.2xx
Choose a base branch
from
+38 −0
Open

Ensure that a virtual project doesn't pick up BaseOutputPath/BaseIntermediateOutputPath from Directory.Build.props #51600

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/Cli/dotnet/Commands/Run/VirtualProjectBuildingCommand.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1180,11 +1180,16 @@ public static void WriteProjectFile(
// Note that ArtifactsPath needs to be specified before Sdk.props
// (usually it's recommended to specify it in Directory.Build.props
// but importing Sdk.props manually afterwards also works).
//
// An empty value is always given for BaseIntermediateOutputPath and BaseOutputPath, to ensure that
// the SDK targets will initialize them based on ArtifactsPath, and not use values for those paths coming in from Directory.Build.props.
writer.WriteLine($"""
<Project>

<PropertyGroup>
<IncludeProjectNameInArtifactsPaths>false</IncludeProjectNameInArtifactsPaths>
<BaseIntermediateOutputPath></BaseIntermediateOutputPath>
<BaseOutputPath></BaseOutputPath>
Copy link
Member

@jjonescz jjonescz Nov 10, 2025

Choose a reason for hiding this comment

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

I think this won't work (and the failing test seems to agree with me). Implicit build files are imported through SDKs below (see where we emit <Import) so that will overwrite any values here.

For properties that cannot be overwritten, see the block where we write <RestoreUseStaticGraphEvaluation>false</RestoreUseStaticGraphEvaluation> for example.

Copy link
Member

@jjonescz jjonescz Nov 10, 2025

Choose a reason for hiding this comment

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

What if the user explicitly sets these via a #:property directive? I think we have a special error for RestoreUseStaticGraphEvaluation which is also a non-overridable property, perhaps we should report an error for these too.

<ArtifactsPath>{EscapeValue(artifactsPath)}</ArtifactsPath>
<PublishDir>artifacts/$(MSBuildProjectName)</PublishDir>
<PackageOutputPath>artifacts/$(MSBuildProjectName)</PackageOutputPath>
Expand Down
33 changes: 33 additions & 0 deletions test/dotnet.Tests/CommandTests/Run/RunFileTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -919,6 +919,39 @@ public void DefaultProps_DirectoryBuildProps()
.And.HaveStdOutContaining("error CS0103");
}

/// <summary>
/// 'BaseIntermediateOutputPath'/'BaseOutputPath' specified in Directory.Build.props should effectively be ignored.
Copy link
Member

@jjonescz jjonescz Nov 10, 2025

Choose a reason for hiding this comment

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

We should document this in https://github.com/dotnet/sdk/blob/release/10.0.2xx/documentation/general/dotnet-run-file.md#implicit-project-file.

/// We want the standard logic for determining these from an 'ArtifactsPath' to always be used.
/// See also https://github.com/dotnet/sdk/blob/2b9fc02a265c735f2132e4e3626e94962e48bdf5/src/Tasks/Microsoft.NET.Build.Tasks/sdk/UseArtifactsOutputPath.props#L25.
Copy link
Member

@jjonescz jjonescz Nov 10, 2025

Choose a reason for hiding this comment

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

I'm not totally sold on this fix. It seems fine to allow users to override these properties. Is this basically a workaround for an IDE issue and should we fix the underlying issue instead (wrongly recognizing files as file-based apps)?

Consider that this is reproable with project-based apps too - if I specify these properties and then have a artifacts-layout-enabled project somewhere, it would also not place artifacts to the artifacts folder. I guess changing that would be more breaking than scoping the change to just file-based apps though.

/// </summary>
[Fact]
public void BaseOutputPathProps_FromDirectoryBuildProps_NotUsed()
{
var testInstance = _testAssetsManager.CreateTestDirectory();
File.WriteAllText(Path.Join(testInstance.Path, "Program.cs"), """
Console.WriteLine("Hi");
""");

var dbPropsObjPath = Path.Join(testInstance.Path, "MyOutput", "obj");
var dbPropsBinPath = Path.Join(testInstance.Path, "MyOutput", "bin");
File.WriteAllText(Path.Join(testInstance.Path, "Directory.Build.props"), $"""
<Project>
<PropertyGroup>
<BaseIntermediateOutputPath>{dbPropsObjPath}</BaseIntermediateOutputPath>
<BaseOutputPath>{dbPropsBinPath}</BaseOutputPath>
</PropertyGroup>
</Project>
""");

new DotnetCommand(Log, "run", "Program.cs")
.WithWorkingDirectory(testInstance.Path)
.Execute()
.Should().Pass();

Assert.False(Directory.Exists(dbPropsObjPath));
Assert.False(Directory.Exists(dbPropsBinPath));
}

/// <summary>
/// Overriding default (implicit) properties of file-based apps from custom SDKs.
/// </summary>
Expand Down
Loading