-
Notifications
You must be signed in to change notification settings - Fork 81
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 orphaned VersionOverrides (opt-in) #180
Disallow orphaned VersionOverrides (opt-in) #180
Conversation
Notes and concerns:
|
How to implement this yourself in a repo: First, put this at the end of Packages.props, after all the version lines:
Second, put this target in a global build file:
|
Sorry I was out of town last week. I'm looking at this now. The unit tests weren't running as part of the build because of something else which I've fixed via a commit. So you'll need to rebase and push when you get a chance
If that doesn't work, I can update the branch in the UI here and you can just pull. |
…ersionOverrides but no central Version
e80db71
to
292c79d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, just change the name of the property.
My item/property names are verbose, I'd really appreciate more concise ideas
Other than the comment left, they look fine to me
Does DisableVersionOverridesWithoutCentralVersions clearly indicate that by turning this on you're disallowing a certain kind of ? ("turn this on in order to turn this other thing off" can be confusing).
DisableVersionOverridesWithoutCentralVersions must be 'true' for this feature to run, and the SDK doesn't set this anywhere, so by default it's the empty string and thus treated as false. This seems OK to me but it's inconsistent with most other properties where empty string is true because we check for != 'false'.
This is why I suggest naming it EnablePackageVersionOverrideWithoutCentralVersion
instead
I didn't do anything about implicitly defined packages so I think the feature should apply to them too. Should I handle these specially? Do implicitly defined packages like FSharp.Core use VersionOverride or just Version? I don't know the pattern here.
Implicitly defined packages should only use Version
I didn't do anything about GlobalPackageReference either. My items are collected right after GlobalPackageReferences are copied to PackageReferences (an arbitrary choice). But GlobalPackageReferences are not allowed to have VersionOverrides (only Versions) so this feature would never apply to them
Although possible, it would be very unusual to use VersionOverride
for global package references so I wouldn't worry about it.
Currently, this gives you one error at a time - so if there are multiple violations in a project, you'd have to build -> fix that one PackageReference -> build again -> fix the next PackageReference -> etc. We could, alternatively, use ContinueOnError="ErrorAndContinue" on the task, to spit out all the violations at once - but then the build wouldn't stop here. I prefer this alternative personally, but other task invocations above, don't do this, so I kept my new invocaton in line with the existing invocations for now.
Let's keep it consistent for now. If you want to send a separate PR to log all of the errors and still fail the build, I'd be more than happy to accept it 😄
This doesn't have an error code so projects can't suppress it. I think this is probably fine. If developers want to have exceptions to this rule, then they should just keep this feature turned off.
You can only suppress warnings anyway. The main point of error codes is so you can search the web for possible solutions. I think the error message is verbose enough to not need more documentation.
…some Windows line endings I accidentally put in the unit test file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the property default value and I'll merge, thank you for the contribution!
@@ -28,6 +28,8 @@ | |||
<EnableCentralPackageVersions | |||
Condition="'$(EnableCentralPackageVersions)' == '' And (!Exists('$(CentralPackagesFile)') Or Exists('$(MSBuildProjectDirectory)\packages.config') Or '$(MSBuildProjectExtension)' == '.vcxproj' Or '$(MSBuildProjectExtension)' == '.ccproj' Or '$(MSBuildProjectExtension)' == '.nuproj')">false</EnableCentralPackageVersions> | |||
|
|||
<EnablePackageVersionOverrideWithoutCentralVersion Condition="'$(EnablePackageVersionOverrideWithoutCentralVersion)' == '' And '$(EnableCentralPackageVersions)' != 'false'">true</EnablePackageVersionOverrideWithoutCentralVersion> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no need to default this property to any value as we'll only look for the user to turn it off by setting it to false
. Let's remove this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I wasn't sure whether checking for false and setting true by default was the right choice. I removed this, thanks for the guidance here.
…tralVersion. In practice, it's true by default because we only run the Error if it's explicitly set to "false'. So an explicitly set default is doing too much. This is more in line with how other overridable props handle their defaults.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution!
See #178 for a description of the problem.
It's a small diff, but it's my first time contributing to this repo so I have some notes and concerns, listed in a comment below. I also have a way you could implement this in a code base with including this feature in the SDK; this should help us understand whether this PR is worth merging.