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

Add option to enable floating versions in CPM #5528

Merged
merged 4 commits into from
Dec 12, 2023

Conversation

jeffkl
Copy link
Contributor

@jeffkl jeffkl commented Dec 4, 2023

Bug

Fixes: NuGet/Home#10432

Regression? Last working version:

Description

By default, central package management (CPM) does not allow users to specify floating version (i.e. 1.*). CPM is considered an enterprise-level feature and since floating versions can introduce non-deterministic restores we want users to not be able to get situations where restores in different environments could not be identical.

However, the community has left a good amount of feedback that such a feature should be allowed with more details available in NuGet/Home#10432. This change allows a user to enable floating versions by setting an MSBuild property which remains off by default.

The only place the MSBuild property will be documented is at https://learn.microsoft.com/en-us/nuget/reference/errors-and-warnings/nu1011 where users will be encouraged to use non floating versions and educated on the potential issues they could encounter.

Original pull request: #5440
This pull request is a revert of a revert plus a change to set the property so that it won't fail DDRITs this time.

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
    • OR
    • N/A

@jeffkl jeffkl self-assigned this Dec 4, 2023
@jeffkl jeffkl requested a review from a team as a code owner December 4, 2023 21:04
nkolev92
nkolev92 previously approved these changes Dec 4, 2023
Copy link
Member

@nkolev92 nkolev92 left a comment

Choose a reason for hiding this comment

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

I just reviewed: 6809e89 :D

donnie-msft
donnie-msft previously approved these changes Dec 5, 2023
Copy link
Member

@zivkan zivkan left a comment

Choose a reason for hiding this comment

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

GetPropertyValueWithDteFallback should not be used by any new MSBuild properties. As a team, we need to find a way to stop people blindly copy/pasting "old code" that uses the DTE fallback, so we can slowly migrate towards a no-DTE future.

@jeffkl jeffkl dismissed stale reviews from donnie-msft and nkolev92 via d203139 December 7, 2023 18:04
Copy link
Member

@zivkan zivkan left a comment

Choose a reason for hiding this comment

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

If you have any ideas on how we can reduce the risk of someone using GetPropertySafe or GetPropertyWithDteFallback the next time anyone adds a new property, then the whole team will benefit.

Unfortunately I don't have any ideas, other then hoping that someone knowledgeable notices during pull requests, which is at high risk of human error.

@jeffkl
Copy link
Contributor Author

jeffkl commented Dec 11, 2023

If you have any ideas on how we can reduce the risk of someone using GetPropertySafe or GetPropertyWithDteFallback the next time anyone adds a new property, then the whole team will benefit.

Unfortunately I don't have any ideas, other then hoping that someone knowledgeable notices during pull requests, which is at high risk of human error.

Could we mark the implementation as obsolete and add an exception for existing calls? We could put it in an #if DEBUG so it would only fail our local build if someone tried to use it going forward.

@jeffkl jeffkl force-pushed the dev-jeffkl-bring-back-cpm-floating-versions branch from d203139 to 7f3be7c Compare December 11, 2023 23:31
@zivkan
Copy link
Member

zivkan commented Dec 12, 2023

Could we mark the implementation as obsolete and add an exception for existing calls? We could put it in an #if DEBUG so it would only fail our local build if someone tried to use it going forward.

I don't see how that would work. Consider we already had a block of:

#pragma disable ObsoleteWarning
var featureThing = GetPropertySafe("FeatureThing");
var featureStuff = GetPropertySafe("FeatureStuff");
var featureWhatever = GetPropertySafe("FeatureWhatever");
#pragma enable ObsoleteWarning

Now someone wants to add a property featureAnotherOne. I think they're likely to just copy paste one of the existing lines, where the obsolete warning has already been disabled.

Especially if you consider that the legacy PR project class has dozens of lines getting properties. Unless we have the obsolete warning suppression/re-enable every few lines, then someone modifying code right in the middle of the block is unlikely to even see the obsolete warning being suppressed.

On the other hand, maybe doing it anyway at least has a chance that someone will notice that GetPropertySafe and GetPropertyValueWithDteFallback are marked obsolete. Especially if we also make sure XMLDoc says to use the other method, so mouse hover intellisense is also a way this information can be discovered.

Previously I was letting perfect be the enemy of good. Apologies to @kartheekp-ms who brought up the same idea earlier, and I similarly said at the time I didn't think it would work. It's still an improvement, so better than doing nothing.

I'm happy to do this in another PR if you like.

@jeffkl jeffkl force-pushed the dev-jeffkl-bring-back-cpm-floating-versions branch from 7f3be7c to 7184c61 Compare December 12, 2023 19:36
@jeffkl jeffkl merged commit 683afcc into dev Dec 12, 2023
15 of 16 checks passed
@jeffkl jeffkl deleted the dev-jeffkl-bring-back-cpm-floating-versions branch December 12, 2023 21:44
@naiksujit
Copy link

Hi @jeffkl : Can we use this with projects that are compiled with netSDK6?

@jeffkl
Copy link
Contributor Author

jeffkl commented Jan 18, 2024

As long as you're building with the latest .NET SDK or Visual Studio, you can still compile applications and libraries for .NET 6 and use this functionality. If you're using an older version of .NET SDK or Visual Studio, this functionality will not exist and can not be used.

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.

[DCR]: Allow floating versions with Central Package Management (CPM)
5 participants