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

Deps.json should include project references that aren't present in project.assets.json #28963

Merged
merged 7 commits into from
Nov 17, 2022

Conversation

wrall
Copy link
Contributor

@wrall wrall commented Nov 9, 2022

Addresses #28891.

Although most projects are included in project.assets.json after a restore takes place, there are some (rare) scenarios when this is not the case. When that happens, project references end up being skipped and excluded from deps.json.

One can work around this by adding a binary Reference, that is inconvenient and can lead to issues when one forgets. Instead, it would be good to check whether the project reference is included in project.assets.json before skipping it when generating the list of dependencies.

Also added a functional test (.NET 6.0 project referencing a .NET Framework project) that failed previously and succeeds now.

(Continues #28892, retargeting release/7.0.2xx)

@wrall
Copy link
Contributor Author

wrall commented Nov 9, 2022

(@marcpopMSFT @dsplaisted FYI.)

Copy link
Member

@dsplaisted dsplaisted left a comment

Choose a reason for hiding this comment

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

This is looking great, thanks!

Comment on lines 76 to 79
if (string.IsNullOrEmpty(projectName))
{
return true;
}
Copy link
Member

Choose a reason for hiding this comment

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

Under what conditions would this be null or empty, and should we be returning true here?

@dsplaisted
Copy link
Member

Looks like there are some test failures in CI: GivenThatWeWantToPublishAProjectWithAllFeatures.It_fails_when_nobuild_is_set_and_build_was_not_performed_previously and GivenThatWeWantToPreserveCompilationContextForBuild.It_supports_copylocal_false_references. Let us know if you need help fixing those.

Thanks!

@wrall
Copy link
Contributor Author

wrall commented Nov 10, 2022

@dsplaisted said:

Looks like there are some test failures in CI: GivenThatWeWantToPublishAProjectWithAllFeatures.It_fails_when_nobuild_is_set_and_build_was_not_performed_previously and GivenThatWeWantToPreserveCompilationContextForBuild.It_supports_copylocal_false_references.

These should be fixed now - there was an issue with how I resolved some of the comments from the last PR.

@wrall wrall requested review from AntonLapounov and a team as code owners November 12, 2022 01:50
@wrall
Copy link
Contributor Author

wrall commented Nov 12, 2022

FYI, latest update was to hide the new behavior behind a new MSBuild property. (Note that the test case I added fails without the new switch being provided, but I did not add a negative test).

@StephenBonikowsky
Copy link
Member

@dsplaisted and @marcpopMSFT is this ready to be merged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants