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

[RikkiGibson]

See microsoft/vscode-dotnettools#2443 (comment)

I think when a Directory.Build.props specifies BaseOutputPath, BaseIntermediateOutputPath, or ArtifactsPath, it should basically be ignored in virtual projects. The reason is that we strongly want to avoid putting the artifacts for those projects in the source tree with a file-based app.

Right now the test is failing, I need to come back and investigate in more details. I also think the <ArtifactsPath> in D.B.props case should be tested, I didn't see an existing test.

Opening PR now to try and share understanding of the issue and get buy-in on proposed solution.

[RikkiGibson]

@jjonescz I am currently trying to debug the new test and understand what is happening with the virtual project.. but, it looks like the test works by spawning a subprocess, which debugger is not attached to. Do you have any suggestions for how to debug efficiently when working in this area?

[jjonescz]

I debug by adding a new launch profile to src\Cli\dotnet\Properties\launchSettings.json, something like:

{
  "profiles": {
    "dotnet": {
      "commandName": "Project"
    },
    "redist": {
      "commandName": "Executable",
      "executablePath": "dotnet",
      "commandLineArgs": "exec $(SolutionDir)artifacts\\bin\\redist\\Debug\\dotnet\\sdk\\10.0.200-dev\\dotnet.dll run Program.cs",
      "workingDirectory": "$(SolutionDir)artifacts\\tmp\\Debug\\testing\\ProjectRefere---C5C43CCB",
      "environmentVariables": {
        "DOTNET_CLI_CONTEXT_VERBOSE": "true",
        "MSBUILDTERMINALLOGGER": "off",
        "CI": "true"
      }
    }
  }
}

The working directory comes from the test which sets up the scenario. Of course you also need to set the command line args correctly. And then just debug with VS.

[baronfel]

I do similar with a vscode launch.json :D

[jjonescz]

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.

[jjonescz]

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

[jjonescz]

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.

[jjonescz]

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.

[RikkiGibson]

It seems fine to allow users to override these properties.

I do see where you're coming from on this. It’s also not clear to me yet whether this PR is the right thing to do.

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.

I think it’s more clear that for ordinary projects, the BaseOutputPath should win over an ArtifactsPath. I think that’s why the logic in UseArtifactsOutputPath.props is written the way it is.

The thing that I am finding concerning about the status quo is: if user only specifies <UseArtifactsOutput>true</UseArtifactsOutput> in D.B.props, then ordinary projects which lack <ArtifactsPath> explicitly will usually output into the default location artifacts/, but FBPs will continue to output into the temp directory.

However if you use <ArtifactsPath>$(MSBuildThisFileDirectory)artifacts</ArtifactsPath> in D.B.props, now it will control both ordinary projects and file-based apps outputs.

Because of the behavior of the former case, I find the behavior of the latter case to be unexpected. It feels like users of these output path controlling properties, would usually not expect file-based programs to also be controlled by those properties, and to have to take some additional step in order to put those artifacts into a custom path. I could be wrong about this. In either case, it seems good to have the discussion, and confirm the reasons for our decision one way or another, then ensure the test coverage is present.