Skip to content

Respect deps.json part 2#7594

Merged
rokonec merged 9 commits into
dotnet:mainfrom
Forgind:testrainerchange
May 25, 2022
Merged

Respect deps.json part 2#7594
rokonec merged 9 commits into
dotnet:mainfrom
Forgind:testrainerchange

Conversation

@Forgind

@Forgind Forgind commented May 3, 2022

Copy link
Copy Markdown
Contributor

Fixes #4081

Context

See #7520

Changes Made

Added looking for a .deps.json

Testing

Found one case when it seemed to discard an assembly without a .deps.json and one when it seemed to include it. (Not extensive)

@rainersigwald rainersigwald left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Have you validated this code against the NuGet workflow that failed?

cc @baronfel

Comment thread src/Shared/MSBuildLoadContext.cs Outdated
{
_directory = Directory.GetParent(assemblyPath)!.FullName;

_resolver = File.Exists(assemblyPath) && File.Exists(Path.Combine(_directory, Path.GetFileNameWithoutExtension(assemblyPath) + ".deps.json"))

@rainersigwald rainersigwald May 3, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
_resolver = File.Exists(assemblyPath) && File.Exists(Path.Combine(_directory, Path.GetFileNameWithoutExtension(assemblyPath) + ".deps.json"))
_resolver = File.Exists(assemblyPath) && File.Exists(Path.ChangeExtension(assemblyPath, ".deps.json"))

?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Beautiful and much nicer than my ugly thing 👍

Comment thread src/Shared/MSBuildLoadContext.cs Outdated
{
_directory = Directory.GetParent(assemblyPath)!.FullName;

_resolver = File.Exists(assemblyPath) && File.Exists(Path.Combine(_directory, Path.GetFileNameWithoutExtension(assemblyPath) + ".deps.json"))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Comment both parts of this condition, please?

@rainersigwald

Copy link
Copy Markdown
Member

Oh and let's make sure to squash this before merge; the commit history is super confusing with the revert.

@Forgind

Forgind commented May 13, 2022

Copy link
Copy Markdown
Contributor Author

Oh and let's make sure to squash this before merge; the commit history is super confusing with the revert.

Do you mean make sure to use squash and merge when merging, or that I should rebase this into a few interesting commits?

@Forgind Forgind added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label May 16, 2022
@rokonec rokonec merged commit 5c5e583 into dotnet:main May 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Consider adopting ComponentDependencyResolver for tasks on Core

3 participants