Enable using package lock files for NuGet restore #12654
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lahma
requested review from
sfmskywalker,
deanmarcussen,
kevinchalet,
hishamco,
Jetski5822,
carlwoodhouse,
jtkech and
sebastienros
as code owners
|
I also enabled |
|
Could we simply add a |
|
I think we would lose the certainty by having some projects having undeterministic dependencies. Of course, separating delivered projects from test/support would make sense. Might be easier with targeted |
Piedone
requested changes
Mar 14, 2024
There was a problem hiding this comment.
I sincerely apologize for us taking so much time here. I checked out the PR, going over old ones, and this would be quite useful.
Is this something you'd like to revisit any time soon or should we close? This would need to be updated with the latest main.
When addressing review feedback, please adhere to the following:
- Apply code suggestions directly so the reviewer doesn't have to eyeball the changes. These resolve themselves after applying them, that's fine.
- Don't resolve other conversations so it's easier to track for the reviewer. Then, the reviewer will resolve them.
- Feel free to mark conversations that you addressed to keep track of them with an emoji or otherwise, just don't resolve them.
- Please keep conversations happening in line comment in those convos, otherwise communication will be a mess. If you have trouble finding them, see this video.
- Please click "Re-request review" in the top-right corner for each reviewer when you're ready for another round of review, so they know that you're done.
| @@ -17,6 +17,10 @@ | |||
| <GenerateAssemblyCompanyAttribute>false</GenerateAssemblyCompanyAttribute> | |||
| <GenerateAssemblyProductAttribute>false</GenerateAssemblyProductAttribute> | |||
|
|
|||
| <EnableLockFiles Condition="$(MSBuildProjectDirectory.Contains('.Tests.')) == false and $(MSBuildProjectDirectory.Contains('.Targets')) == false">true</EnableLockFiles> | |||
There was a problem hiding this comment.
Why not use lock files for these projects?
Comment on lines
+21
to
+22
| <RestorePackagesWithLockFile Condition="$(EnableLockFiles) == 'true'">true</RestorePackagesWithLockFile> | ||
| <RestoreLockedMode Condition="$(EnableLockFiles) == 'true' and '$(ContinuousIntegrationBuild)' == 'true'">true</RestoreLockedMode> |
There was a problem hiding this comment.
Link to some docs about both of these properties, please.
There was a problem hiding this comment.
Are these files automatically managed during a NuGet version update when one updates packages locally in the solution or is there anything special to be done? If the latter, please add docs about it to Dependencies.props.
There was a problem hiding this comment.
Do I understand correctly that these can't be DRYd anymore, not to have hundreds of files?
|
FWIW, most of the projects I've worked on that were using lock files have stopped using them:
IMHO, the very few advantages it offers are not enough to justify the EXTREME noise it generates (not to mention the very high risk of merge conflicts in PRs, specially here with our "one PR per dependency change" policy). |
|
@kevinchalet makes excellent points above so I'm closing this. Maybe better in enterprise setting where we also generate license information reports which are tightly tied to versions. |
|
That's a good perspective! My concern that this fixes is mostly about our dependencies using floating versions. E.g. we use |
This is not the behavior you should be seeing: NuGet always enforces a "lowest version possible" policy by default (and AFAICT, OrchardCore doesn't opt in a different So when referencing |
|
Here's the complete list of dependencies - transitive packages included - I'm getting for |
|
Ah OK, good, thank you. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Originally found this while testing #12589 .
While the current dependencies file states desired versions, it could resolve to different dependency graphs (versions) depending on when restore is done. By enabling repeatable package restores, builds should be locked to given versions and also allow unambiguous caching of NuGet references.
With this PR, proposing to enable lock file usage by default and committing lock files to VCS, just like it's done for Node dependencies.