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

Adds fallback folder for full framework and multi-tfm projects. #1399

Closed

Conversation

livarcocc
Copy link
Contributor

Customer scenario

Fallback folders were not being set for full framework projects nor multi-tfm projects. Adds fallback folder for full framework projects by removing the condition on the project being netstandard or netcore. Also, sets the properties for multi-tfm projects. We did that by evaluating all the TFMs in the project and if any of them target a 1.x project, we default to PackageSource, as we need to use the lowest common denominator here, so that the project will work independently of the TFM.

Bugs this fixes:

Fixes #1389

Workarounds, if any

Don't use the offline cache. Instead, we will hit the web during restore for Multi-tfm projects.

Risk

Low

Performance impact

Low. We are running another set of inner-loop evaluations.

Is this a regression from a previous update?

Yes.

Root cause analysis:

We were only setting the property for NuGet in the inner-loop.

How was the bug found?

Partner team.

@dotnet/dotnet-cli for review

@MattGertz for approval

…erty for multi-tfm projects. We did that by evaluating all the TFMs in the project and if any of them target a 1.x project, we default to PackageSource, as we need to use the lowest common denominator here, so that the project will work independently of the TFM.
@livarcocc livarcocc added this to the 2.0.0 milestone Jul 7, 2017
@livarcocc livarcocc changed the title Adds fallback folder for full framework projects. Also, sets the prop… Adds fallback folder for full framework and multi-tfm projects. Jul 7, 2017
@livarcocc
Copy link
Contributor Author

@emgarten

Copy link
Member

@emgarten emgarten left a comment

Choose a reason for hiding this comment

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

This looks good to me, Restore should read the properties after this target runs.

@livarcocc
Copy link
Contributor Author

@emgarten does restore in va use the same target?

@emgarten
Copy link
Member

emgarten commented Jul 11, 2017

VS restore is nominated from the project system, it wouldn't be the same.

@natidea is there a target that runs before restore properties are checked? Does CollectPackageReferences run first?

@livarcocc
Copy link
Contributor Author

We use _GenerateRestoreProjectSpec in another target, should we use the same here? We want the SDK NuGet properties to be set for both CLI and VS.

@livarcocc
Copy link
Contributor Author

@emgarten _GenerateRestoreProjectSpec does not work, I just tried it.

@emgarten
Copy link
Member

_GenerateRestoreProjectSpec would be called later than _GetRestoreSettings

@natidea
Copy link
Contributor

natidea commented Jul 11, 2017

The nomination in VS runs after the Design Time build completes and all targets have run. So yes, CollectPackageReferences is always run before hand, unless the DT build fails before the target has run.

@nguerrera
Copy link
Contributor

@natidea Aren't all design-time builds done on the inner/configured project?

@livarcocc
Copy link
Contributor Author

Closing this for now. Will re-activate when we have a solution that also works in VS.

@livarcocc livarcocc closed this Jul 12, 2017
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.

6 participants