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

There is no warning when using code from indirect package reference leading to potential runtime errors #5715

Open
mungojam opened this issue Aug 7, 2017 · 7 comments
Labels
Priority:3 Issues under consideration. With enough upvotes, will be reconsidered to be added to the backlog. Status:Excluded from icebox cleanup Status:Inactive Icebox issues not updated for a specific long time

Comments

@mungojam
Copy link

mungojam commented Aug 7, 2017

There is a fairly simple case in the new PackageReference world where a runtime error can occur where it could be prevented or flagged by maybe msbuild or the compiler. It comes about because we can happily call code that is in indirectly referenced packages, not just in the top level ones.

Detailed Scenario:

3 packages: PackA, PackB and PackC (all .net standard 1.4 in my case)

One app: App1 (.net framework or .net core)

  1. PackA has no dependencies

  2. PackB v1.0.0 has PackageReference to PackA (doesn't matter if it actually uses it or not)

  3. PackC has PackageReference to PackB (v1.0.0) but directly uses method in PackA. It is unfortunately able to do this even though it doesn't explicitly list PackA as a PackageReference. Maybe the developer uses PackB so adds that and doesn't realise they were then also using PackA (easy to do with internal company packages as you tend to remember what functions exist more than what packages they live in).

  4. App1 has PackageReference to PackC and PackB (v.1.0.0).

  5. Everything works nicely but then...

  6. PackB v1.0.1 is released, which removes the PackageReference to PackA.

  7. App1 updates PackB to v1.0.1

  8. App1 compiles but when it calls PackC which then tries to call PackA, it fails with FileNotFoundException because PackA is not known to be a dependency so does not get included in the build.

My thought is that the ideal way to fix this would be a compiler error or warning at step 3, but open to other suggestions. This could be done through some secondary type of DLL reference that couldn't be directly bound to.

I'm concerned we will hit this a lot with our internal packages as we try to remove indirect packages when we convert from packages.config to PackageReference.

VS version (if appropriate): 2017.2 and 2017.3

Sample project attached in broken state having updated PackB to 1.0.1

IndirectPackageReferences.zip

@mungojam
Copy link
Author

Moved to dotnet/roslyn#22095 as I think it should be fixed at compiler level

@dotMorten
Copy link

dotMorten commented Mar 3, 2018

I think some of this is not just compiler but also nuget. For instance if the indirectly referenced package has a .targets file that xcopy deploys native binaries, that target file isn't run, and thus you get a runtime error because the p/invoke call fails with MethodNotFoundException (typical usecase is sub-pacakages for each platform that contain the native libs, so for instance a desktop dev doesn't have to download android, ios and uwp native libs when he references the package, but also so you don't blow past the 250mb NuGet.org package size limit).
You can currently work around that by specifying PrivateAssets="None", but our customers keeps hitting this because they don't know that's what they manually need to do.
I need a way in my nuspec to specify that the dependency is super duper important, and it should run the .targets file, deploy content etc etc., so referencing my package "just works".

@mungojam
Copy link
Author

mungojam commented Mar 3, 2018

Yeah, I think it's nuget too now. Or improved VS tooling in my case could help. Your issue sounds a bit different to mine, and I think I have seen yours targeted in another issue where they concluded that the default behaviour should be different but it was too late to change it now.

@krispenner
Copy link

Would this not affect bundle package references like Microsoft.AspNetCore.All where their intended purpose is to provide no code but just a bundle of packages so you don't need to go reference 100 other packages directly? You reference sub packages indirectly through the parent bundle package and this is intentional.
I'm not familiar with PrivateAssets and how that would maybe work around this though.

@mungojam
Copy link
Author

@krispenner yeah, those meta packages should be the exception rather than the rule IMO. For those, you would specifically add the packages and tell them to export to consuming packages somehow. That's how R packages work and I think npm node packages too.

@krispenner
Copy link

krispenner commented Jun 18, 2018

Right, great point, I do like the idea of this being explicitly set in the nuget package config by the author indicating which packages are to be exported for consumption or maybe if the package has no dll of its own and only references other packages it could somehow be inferred that it's a bundled package of sorts. Cool, interesting to see what happens here. I also didn't realize this issue was closed when I initially commented but thanks for the reply!

@mungojam mungojam reopened this Jun 18, 2018
@mungojam
Copy link
Author

Thanks for the support, reopened now, I thought it was a compiler issue, but now I think more a nuget tooling thing

@ghost ghost added the Status:Inactive Icebox issues not updated for a specific long time label Sep 1, 2022
@jeffkl jeffkl added Priority:3 Issues under consideration. With enough upvotes, will be reconsidered to be added to the backlog. and removed Pipeline:Icebox labels Apr 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority:3 Issues under consideration. With enough upvotes, will be reconsidered to be added to the backlog. Status:Excluded from icebox cleanup Status:Inactive Icebox issues not updated for a specific long time
Projects
None yet
Development

No branches or pull requests

6 participants