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

Fix #1435 - calculation of ResolvedPath for project references #1442

Merged
merged 2 commits into from
Oct 30, 2017

Conversation

BrunoJuchli
Copy link
Contributor

@BrunoJuchli BrunoJuchli commented Jul 22, 2017

See #479 and #1435 for error description and analysis of the problem. In short:

The old implementation combined the relative <ProjectReference Include="..." /> path with the directory of the ProjectAssetFile.
Corrected this to combine the relative project reference with the current's project path.

Questions:

  • Is it actually the desired outcome that ResolvedPath points to the csproj file? Or should it point to the output directory of that project?
  • Is ProjectPath always passed in as full path? (otherwise it needs to be enclosed by a Path.GetFullPath)

The old implementation combined the relative `<ProjectReference Include="..." />` path with the directory of the `ProjectAssetFile`.
Corrected this to combine the relative project reference with the current's project path.
@nguerrera
Copy link
Contributor

@natidea

@nguerrera
Copy link
Contributor

The code before the change does look wrong. I'm trying to understand why this wasn't reported sooner. The repro steps in #1435 don't seem out of the ordinary. @dsplaisted @livarcocc

@natidea
Copy link
Contributor

natidea commented Jul 24, 2017

@nguerrera this was being tracked with #479
Is this incorrect metadata being used anywhere, or is this usage something custom for the reported project?

@nguerrera
Copy link
Contributor

@natidea #1435 says the build is failing due to this.

@BrunoJuchli Do you have anything custom reading this ResolvedPath ?

@nguerrera
Copy link
Contributor

Is #1435 actually an instance of #922? Is there a project.json project involved?

@BrunoJuchli
Copy link
Contributor Author

BrunoJuchli commented Jul 26, 2017

@nguerrera In my case there's no project.json involved.
The solutions contains classic *.csproj + "modern" (VS2017,..) .csproj. And some csproj target .net full framework and others netstandard - if that's of any relevance.

This pertains a direct-reference, not a transitive one. The csproj of the project being built contains a <ProjectReference Include='...' /> pointing the the project who's ResolvedPath is calculated incorrectly.

Full source code here: https://github.com/BrunoJuchli/StaticProxy.Fody/tree/7d8b924d90759cb3248803514ba631463358494a/

note: there's two solutions. The relevant is StaticProxy.Fody.sln

And yes, there's something custom reading the resolved path: Fody. The ResolvedPaths is one of the ways its looking for weaver-dll's.

(Not that the reason for bringing this issue up is not to make Fody work, or work any better, it's just how I stumbled upon the issue of there being an invalid path).

I also doubt that this change is correct. As stated in the entry-post I'm unsure whether ResolvedPath pointing to a *.csproj is valid. Shouldn't it point to where the (built) assemblies of of the <ProjectReference...> are located? All the other ResolvedPaths do.

@BrunoJuchli
Copy link
Contributor Author

BrunoJuchli commented Jul 26, 2017

@natidea
I concur that #1435 is a duplicate of #479. Sorry for posting the duplicate.
I've updated the issue and this PR accordingly.

@nguerrera
#479 states

The resolved path for project references is not actually used anywhere currently, so this bug does not cause any problems, but should be fixed anyway.

Hence only the consumers will "fail", but a standard build-config without any custom code won't "fail" - it just produces invalid output for external consumers.

@nguerrera nguerrera merged commit 634680a into dotnet:master Oct 30, 2017
@BrunoJuchli BrunoJuchli deleted the patch-2 branch March 12, 2018 11:32
mmitche pushed a commit to mmitche/sdk that referenced this pull request Jun 5, 2020
…0200508.1 (dotnet#1442)

- Microsoft.AspNetCore.Analyzers: 5.0.0-preview.5.20256.9 -> 5.0.0-preview.5.20258.1
- Microsoft.AspNetCore.Mvc.Analyzers: 5.0.0-preview.5.20256.9 -> 5.0.0-preview.5.20258.1
- Microsoft.AspNetCore.Components.Analyzers: 5.0.0-preview.5.20256.9 -> 5.0.0-preview.5.20258.1
- Microsoft.AspNetCore.Mvc.Api.Analyzers: 5.0.0-preview.5.20256.9 -> 5.0.0-preview.5.20258.1

Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants