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

Disallow VersionOverrides without a central version #178

Closed
erikness opened this issue May 19, 2020 · 6 comments
Closed

Disallow VersionOverrides without a central version #178

erikness opened this issue May 19, 2020 · 6 comments
Labels
Feature Request New feature or request

Comments

@erikness
Copy link
Contributor

erikness commented May 19, 2020

Right now, if you make a PackageReference with no version and no entry in Packages.props, you'll get an error. But this SDK does not actually force every PackageReference to have an entry in Packages.props: if the PackageReference uses a VersionOverride, it can be missing from Packages.props.

It's not a conceptual problem - no need to check Packages.props when you already know the exact version you want, right? - but nevertheless I think it can be a hygiene problem. So I'd like to propose throwing errors for these as well.

Reasons

First, some teams probably use Packages.props as a big list of NuGet packages that are used in their repo. If there are PackageReferences that are not recorded in Packages.props, you lose the easy ability to see your entire group of package dependencies. (Though, on the other hand, it's also possible for you to have a Packages.props global version without any project actually using that package, so maybe teams shouldn't assume this).

Second, VersionOverride is an awkward name if it's not actually overriding anything.

Third, I don't see an easy way to implement this restriction for just my team; if it were easy to make a target that did this, it would be one thing, but the only implementation I can think of involves editing Sdk.targets. So it would be helpful if this feature were provided by the SDK itself. (Alternative: maybe add a new extensibility property, CustomAfterCentralPackagesImport?)

This feature would be team-specific (I'm just going off my own experiences and needs- it's not like I've done a user study) and it would be a breaking change (though not in a particularly disruptive way) so of course it can be behind a feature flag.

Sketch of implementation

In Sdk.targets, right after importing Packages.props, you have a bunch of PackageReference items with both VersionOverride and Version metadata. But you also might have PackageReference items with just VersionOverride metadata and no Version - these are the "illegal" ones with respect to this feature.

<_PackageReferenceWithVersionOverrideButNoCentralVersion 
    Include="@(PackageReference->HasMetadata('VersionOverride'))"
    Exclude="@(PackageReference->HasMetadata('Version'))"
 />   

Then, you'd just place an task at the end of the CheckPackageReferences target if there are any of these items. I don't think you'd need to change any other behavior of CentralPackageVersions.

@jeffkl jeffkl added the Feature Request New feature or request label May 19, 2020
@jeffkl
Copy link
Contributor

jeffkl commented May 19, 2020

Sounds like a good feature, do you want to send a pull request? If not, I can work on it next week as I'm on vacation at the moment.

@erikness
Copy link
Contributor Author

I'll make a pull request in the next few days. Though I think I found a good way to do this in my own team's repo, and this might be a reason to reject the PR:

  1. Make another ItemGroup at the end of your Packages.props file that collects the PackageReferences that have a VersionOverride but no Version.
  2. In Directory.Build.targets, make a target that throws an error

@jeffkl
Copy link
Contributor

jeffkl commented May 27, 2020

That implementation sounds right. I'd rather have it built into the product so you don't have to manage it yourself and others can use the functionality. We'd just need to add it behind an opt-in property like "DisallowVersionOverride` so people can turn it on and we can document it.

I can work on it tomorrow probably if you don't have time.

@erikness
Copy link
Contributor Author

Can you give me until the end of the week to make a PR? I've almost got an implementation that works. You might want to implement this feature differently of course, but my first attempt should flush out most issues that are worth discussing.

@jeffkl
Copy link
Contributor

jeffkl commented May 27, 2020

Yes of course, no hurry. I'll only work on it if you let me know that you don't have time to do it.

@jeffkl
Copy link
Contributor

jeffkl commented Jun 4, 2020

Closed via #180 and available in version 2.0.79

@jeffkl jeffkl closed this as completed Jun 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Request New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants