Use ProjectReference items for live built dependencies #1185
Conversation
| // If the dependency doesn't exist, emit a package reference (i.e. for source-build-externals packages like Newtonsoft.Json). Otherwise, emit a project reference. | ||
| if (!File.Exists(Path.Combine(ProjectRoot, dependencyProjectRelativePath))) | ||
| { | ||
| packageReferences += $" <PackageReference Include=\"{packageDependency.ItemSpec}\" Version=\"{dependencyVersion}\" />{Environment.NewLine}"; | ||
| } | ||
| else | ||
| { | ||
| // Make sure that the path always uses forward slashes, even on Windows. | ||
| projectReferences += $" <ProjectReference Include=\"../../{dependencyProjectRelativePath.Replace('\\', '/')}\" />{Environment.NewLine}"; | ||
| } |
There was a problem hiding this comment.
I would like some input here. This behavior is the reason for the currently failing tests in the PR. The idea here is to use P2Ps by default and only if the dependency doesn't exist as a project, fallback to a PackageReference. This works well in the standard case which creates dependencies before generating the dependee, but not when re-generating packages with the --exclude-dependencies / -x flag when the dependency doesn't yet exist on the filesystem.
I was also considering an allow-list but that would have the downside of silently dropping disallowed dependencies. I like that disallowed external dependencies are in the project file but then later filtered out, as that better projects the original package, i.e.
There was a problem hiding this comment.
Hmm, talking this through...
My concern with the current implementation approach is that it could easily allow prebuilts in the system. Depending on how the SB repo legs evolves, these may not be caught until the flow PR into the VMR.
With the allow-list, I agree I don't like the silent dropping. With this approach what would be wrong with always emitting a ProjectReference for anything outside of the allow-list? For exceptions like the Interop case referenced, you could still remove the reference in a customization couldn't you? The benefit of this is it would be guaranteed to cause a build error and force the developer to explicitly address the problem (vs the silent drop or introduction of a possible prebuilt).
There was a problem hiding this comment.
With this approach what would be wrong with always emitting a ProjectReference for anything outside of the allow-list? For exceptions like the Interop case referenced, you could still remove the reference in a customization couldn't you?
It's mostly correctness. It looks wrong to emit a P2P pointing to a project that doesn't exist and shouldn't exist. But thinking more about this, I don't think there's another way. It's impossible to differentiate between an in-source and external dependency without having a hardcoded list or looking at the filesystem (which isn't ideal as mentioned).
|
As part of this change, I think the version.details.xml self-reference should be removed as this is no longer needed and will help validate the change. FYI, Once merged and flowed into the VMR, we can excluded the reference packages from the n-1 artifacts and the special logic handling around |
|
I reverted the Version.Details.xml change (removing the SBRP self-reference) as that introduces new pre-builts around targeting packs and the Microsoft.NETCore.Platforms package. The prebuilts indicate that the live artifacts aren't used in the built. That's unrelated to this change and should get fixed independently. Filed dotnet/source-build#4931 & dotnet/source-build#4932 This PR should be ready now. |
|
@ViktorHofer, I pushed a change to include removing the DependencyPackageProjects references from the readme. Just in case someone new to the process needs to add a project, I didn't want them to be confused. If you are alright with those changes, feel free to merge. |
Fixes dotnet/source-build#1690
Fixes dotnet/source-build#1605
Commit 1: Allows the repository to use P2Ps and update the generate tooling
Commit 2: Update all projects via generate.sh and add a few Customizations.props files