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 references to Exe projects #14488

Merged
merged 4 commits into from
Dec 30, 2020

Conversation

dsplaisted
Copy link
Member

Fix #1675

Include runtimeconfig.json and deps.json in files that are copied from a referenced Exe project.

Note that I also included a refactoring of how you can create test PublishCommands, which touched a lot of files. So it should be easier to review this commit by commit.

Currently, it still fails if a self-contained project references a framework-dependent project, or vice versa. When a self-contained project references a framework-dependent project, running the referenced project fails with the following error:

It was not possible to find any compatible framework version
The framework 'Microsoft.NETCore.App', version '5.0.0' was not found.

  • No frameworks were found.

You can resolve the problem by installing the specified framework and/or SDK.

I suspect this may be because some of the host files (hostfxr or something) are different between self-contained and framework-dependent apps, and are interfering with each other. @vitek-karas @agocke What do you think?

When a framework-dependent project references a self-contained project, it fails because the runtime pack assets aren't copied. However, if we were to fix that I think we might run into the same issues that we get when referencing a framework-dependent project from a self-contained project.

@jaredpar @ericstj @ViktorHofer

@dsplaisted dsplaisted requested review from rainersigwald and a team November 10, 2020 19:59
@vitek-karas
Copy link
Member

The mismatch between two exes - one being SCD and the other FDD won't work. Host is kind of dumb (for historical reasons). When it looks for hostfxr it first looks into the app's directory and if it finds it there, it will behave as SCD until it reads the runtimeconfig: https://github.com/dotnet/runtime/blob/33c9d5afdb3f165599ab0e858652a4d416d34a9e/src/installer/corehost/cli/fxr_resolver.cpp#L60-L65

The result of this is that doesn't look at DOTNET_ROOT because it thinks it's running SCD and thus uses the app's directory as the "dotnet root". Later on when it finds out it's an FDD (by reading runtimeconfig) it will still use the app's directory as "dotnet root" and will try to find shared frameworks in it (this is totally dumb) - which will obviously fail.

On Windows this is even worse because if DOTNET_MULTILEVEL_LOOKUP is enabled (which is the default), it will actually fallback to search global locations and it might find matching framework there. In which case it will run just fine. But it will still ignore DOTNET_ROOT.

Given the challenges of the reverse where the main project is FDD and referenced one is SCD (copying over runtime pack assets), I would probably go with disabling the mixed scenarios explicitly. Let's make SDK fail in those cases. We know it won't work (or it will be unreliable).

If the two match, it should work just fine - assuming SDK does version validation of the frameworks and performs necessary roll-forward and so on. The interesting case would be to try what happens if the main project is net3.1 and the referenced project is net5.0 for example and they're both SCD.

Note that host will not perform any version validation for SCD apps - it will blindly load what's in the .deps.json and on disk. It only performs framework version resolution for FDD apps.

@vitek-karas
Copy link
Member

Related to this - if we're now officially supporting building multiple exes into one output we should also consider what happens with publishing. Do we publish each project separately and then just copy them into one output location? Or do we somehow publish them "together"?

  • ILLink - this could be made to work for SCD - we would pass multiple entry points to linker and it would trim the framework for "both". Getting the right settings might be challenging though.
  • Single-file - not sure what the desirable effect is in this case - but there's no good way to make this work for "merged" publish
  • CrossGen - right now probably relatively easy since it runs per-assembly, but going forward would have similar problems as single-file with large version bubbles.

@dsplaisted dsplaisted force-pushed the exe-project-references branch from 7da3a0d to df2816c Compare November 11, 2020 17:24
@marcpopMSFT marcpopMSFT requested a review from wli3 November 18, 2020 22:19

</Target>

<Target Name="AddDepsJsonAndRuntimeConfigToPublishItemsForReferencingProjects"
Copy link

@wli3 wli3 Nov 19, 2020

Choose a reason for hiding this comment

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

No other way but need to write the same logic twice for publish and build?

And I cannot find the logic of copying the referenced exe, how that worked?

Copy link
Member Author

Choose a reason for hiding this comment

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

The logic is not quite the same. The files can be different between build and publish in some cases.

For copying the referenced exe: The apphost is added as a None item with CopyToOutputDirectory=PreserveNewest in the _ComputeNETCoreBuildOutputFiles, with Link set to the target filename. That gets gathered by the targets that get items to copy to the output directory from referenced projects.

Copy link

@wli3 wli3 left a comment

Choose a reason for hiding this comment

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

Looks good. Had few questions. And update the comments


[Theory]
//[InlineData(true, false, Skip = "Currently not supported (see ReferencedExeFailsToRun)")]
//[InlineData(false, true, Skip = "Currently not supported (see ReferencedExeFailsToRun)")]
Copy link

Choose a reason for hiding this comment

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

update this comment too

[Theory]
// Fails because runtime pack artifacts are not transitively copied
[InlineData(true, false)]
// Fails with the following error (is the self-contained copy of hostfxr or something interfering with the framework dependent app?):
Copy link

Choose a reason for hiding this comment

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

update the comment after Vitek's reply

@dsplaisted
Copy link
Member Author

@vitek-karas We don't have a good way of checking that a FDD isn't referencing an SCD, or vice versa. Also, it's possible that the referencing app could be calling into APIs of the referenced project instead of expecting it to run as a separate app from the same folder.

So for now I'd like to leave it as it is in this PR, where you can reference the Exe project but it won't run correctly if there's an FDD/SCD mismatch. How does that sound?

@vitek-karas
Copy link
Member

As a point in time (to make this PR reasonable) I think it's OK. But I don't think we should ship that way. Or rather, there are already holes in the SDK where some weird combination of inputs creates output which is somehow wrong without SDK warning/failing, or SDK silently ignoring settings (for example building a classlib as self-contained - no warning, ignored; publishing class lib as self-contained - the output is actually SCD which is useless - won't work). I think we should try move to a place where if the SDK doesn't warn/fail, it produces output which should work and it follows the inputs (so if I ask for self-contained I either get a working SCD, or SDK warns/fails)
.
If FDD references SCD we know the output is effectively broken - so SDK should probably fail on this.

@dsplaisted
Copy link
Member Author

I filed #15117 to track generating an error if there's a SelfContained mismatch on an Exe-to-Exe reference.

@vitek-karas Feel free to file issues for other invalid property combinations where you think we should generate an error.

@dsplaisted dsplaisted merged commit 1e54e83 into dotnet:release/5.0.2xx Dec 30, 2020
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.

3 participants