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

Allow disabling the AssetTargetFallback dependency resolution via an environment variable #4703

Merged
merged 6 commits into from
Jun 28, 2022

Conversation

nkolev92
Copy link
Member

@nkolev92 nkolev92 commented Jun 27, 2022

Bug

Fixes: NuGet/Home#11564

Regression? Last working version:

Description

Some users have been affected by the recent bug fix to start resolving the dependencies via AssetTargetFallback. In particular, https://github.com/NuGet/NuGet.Client/pull/4372/files is the change.

NuGet/Home#11564 has testimonials about it, and as a response we will:

  • Disable the behavior in 17.3/6.3/6.0.400.
  • Remove the fallback in 17.4/6.4/7.0.100.

This should ease the transition for the more severely affected customers.

Normally the most convenient way of disabling this would be a property in the project, but that change creates a bunch of public API changes that we'd need to keep.
Current NuGet does a lot of heavy caching when running restore, to ensure that we don't do multiple assets selections for a package id/version/framework. In order to ensure correctness, we'd have to account for this extra variable as well, whether ATF dependency resolution is enabled or not.
I'd argue this is risky and especially given that we'd want to revert this in 17.4, I think the cost would be too high to do it on a project level.
For reference this is how the other change would look like: https://github.com/NuGet/NuGet.Client/compare/dev-nkolev92-ATFDependenciesBehaviorFallback?expand=1.

The env var name is NUGET_USE_LEGACY_ASSET_TARGET_FALLBACK_DEPENDENCY_RESOLUTION and only the true case ignored value will be considered.

PR Checklist

  • PR has a meaningful title

  • PR has a linked issue.

  • Described changes

  • Tests

    • Automated tests added
    • OR
    • Test exception
    • OR
    • N/A
  • Documentation

    • Documentation PR or issue filled - TBD. Waiting on approval from @aortiz-msft on this.
    • OR
    • N/A

@nkolev92 nkolev92 marked this pull request as ready for review June 27, 2022 22:09
@nkolev92 nkolev92 requested a review from a team as a code owner June 27, 2022 22:09
@nkolev92 nkolev92 requested a review from erdembayar June 28, 2022 01:19
Copy link
Contributor

@jeffkl jeffkl left a comment

Choose a reason for hiding this comment

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

In my opinion, having to set an environment variable is a little rough. Say my entire repository isn't working with ATF yet, I can't expect everyone who clones and builds it to set an environment variable. It would be much better if it was an MSBuild property I could set instead but of course that's a lot more work for us 🤕

@nkolev92
Copy link
Member Author

In my opinion, having to set an environment variable is a little rough. Say my entire repository isn't working with ATF yet, I can't expect everyone who clones and builds it to set an environment variable. It would be much better if it was an MSBuild property I could set instead but of course that's a lot more work for us 🤕

Yeah, it creates maintainability issues down the line if we did properties :(

Fortunately, I don't expect this to be used heavily.

@nkolev92 nkolev92 requested a review from jeffkl June 28, 2022 18:54
Copy link
Contributor

@jeffkl jeffkl left a comment

Choose a reason for hiding this comment

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

An env var is better than nothing but I still think a property would be best

@nkolev92 nkolev92 merged commit df7d995 into dev Jun 28, 2022
@nkolev92 nkolev92 deleted the dev-nkolev92-ATFDependenciesEnvVarFallback branch June 28, 2022 22:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants