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

Nuget CPM Lock file Proposal (#12409) #13288

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

CEbbinghaus
Copy link

@CEbbinghaus CEbbinghaus commented Mar 4, 2024

After some Internal deliberation I have come up with a fairly straightforward design as to how a Central lock file could be implemented in Nuget. This addresses #12409 which is currently the 3rd highest rated issue.

Rendered Proposal

Appreciate any feedback <3

@CEbbinghaus CEbbinghaus requested a review from a team as a code owner March 4, 2024 01:07
@dotnet-policy-service dotnet-policy-service bot added the Community PRs (and linked Issues) created by someone not in the NuGet team label Mar 4, 2024
@CEbbinghaus
Copy link
Author

@dotnet-policy-service agree company="WiseTech Global"

@DrNeil
Copy link

DrNeil commented Mar 13, 2024

This would be super useful.

accepted/2024/repo-level-lock-file.md Outdated Show resolved Hide resolved
accepted/2024/repo-level-lock-file.md Outdated Show resolved Hide resolved
accepted/2024/repo-level-lock-file.md Outdated Show resolved Hide resolved
accepted/2024/repo-level-lock-file.md Outdated Show resolved Hide resolved
accepted/2024/repo-level-lock-file.md Show resolved Hide resolved
accepted/2024/repo-level-lock-file.md Outdated Show resolved Hide resolved
accepted/2024/repo-level-lock-file.md Outdated Show resolved Hide resolved
CEbbinghaus and others added 2 commits March 20, 2024 13:48
Co-authored-by: Yaakov <yaakov-h@users.noreply.github.com>
@dotnet-policy-service dotnet-policy-service bot added the Status:No recent activity No recent activity. label Apr 19, 2024
@nkolev92 nkolev92 removed the Status:No recent activity No recent activity. label Apr 19, 2024
@NuGet NuGet deleted a comment from dotnet-policy-service bot Apr 19, 2024
@CEbbinghaus
Copy link
Author

What can we do to further progress this proposal?

@JonDouglas JonDouglas self-requested a review May 8, 2024 20:33
@JonDouglas
Copy link
Contributor

@CEbbinghaus Thank you kindly for starting a discussion on this proposal! Give me some time to look through it and get the team together to review it. I didn't see the notification earlier as I've been terribly busy with some other things. I apologize in advance for the ~2 month delay.

@CEbbinghaus
Copy link
Author

I appreciate you taking a look at it now. Please do let us know if anything needs addressing

<Project>
<PropertyGroup>
<ManagePackageVersionsCentrally>true</ManagePackageVersionsCentrally>
<NuGetLockFilePath>lockfile.json</NuGetLockFilePath>

Choose a reason for hiding this comment

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

i love JSON as much as the next guy. but if Directory.Packages.props, NuGet.props and most of the files are XML based, wouldn't it be better to not mix stacks? historically lock files use .json format in various programming platforms, so that might be one defense for it (but not a very compelling one).

Choose a reason for hiding this comment

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

The existing related files are already JSON - packages.lock.json and project.assets.json.

Copy link
Author

Choose a reason for hiding this comment

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

Personally my preference is also xml but the nuget/dotnet team have already settled on a json based lock file and this change is supposed to be as small and simple as possible.

<!-- Explain the proposal as if it were already implemented, and you're teaching it to another person. -->
<!-- Introduce new concepts, functional designs with real-life examples, and low-fidelity mockups or pseudocode to show how this proposal would look. -->

The `Directory.packages.props` now uses the`<NuGetLockFilePath>[path]</NuGetLockFilePath>` property within a `<PropertyGroup>` to specify its lock file path. It becomes the source of truth for all builds. Project level lock files are no longer required unless a package version is overridden.

Choose a reason for hiding this comment

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

Is there a reason to make the lock file path configurable in the project? AFAIK, most package managers leave it unconfigured, and that way, the package manager wouldn't have to first read the configuration file first in order to install packages, only the lock file(which makes it easier to separate into steps).

Copy link
Author

Choose a reason for hiding this comment

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

The package manager would need to read the Directory.packages.props file anyways as it is what enables CentralPackageManagement in the first place? (relevant docs)

We could get away with setting a property like <RestorePackagesWithLockFile>true</RestorePackagesWithLockFile> to enable CPM Lock files and implicitly assume the <NuGetLockFilePath> to be packages.lock.json but we can also achieve the same with a single property and a check $(NuGetLockFilePath) = ''. At the end it will be up to the maintainers to give feedback which way they prefer but the lock file path will need to be configurable in any case.

Copy link

@purepani purepani May 23, 2024

Choose a reason for hiding this comment

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

The package manager would need to read the Directory.packages.props
Yeah, but you can separate that into steps right? I'm imagining a use case of being able to separate it into layers:
You read the configuration file and can generate the lock file in one step, and then you can read the lock file and get the packages in a different step. The reason this could be useful is that you can write a completely separate program to read the lock file if you wanted to(one that assumes that you're using CPM), and you wouldn't have to read the configuration file to figure out where the lock file is.

I personally can't think of a great reason to not have the lock file at the top level of a repo(or at some fixed path relative to the top level), and I can think of more reasons to avoid being able to configure it(Easier to make external tools to consume the lock files and process them separately, avoiding conflicts between, say, a git submodule with it's own lock file that's configured to generate this lock file, etc.).

Given that, as far as I'm aware, most package managers(mostly python and node ones) that I've used have a fixed path for their global lock files, I thought I'd at least bring it up.

Choose a reason for hiding this comment

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

You would need a way to define the "top level" location. This allows for that.

Keep in mind that not every repo-project relationship is 1:1, particularly in a monorepo situation where you might want a small handful of "top levels" rather than only one.

Choose a reason for hiding this comment

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

It should just be in the same folder as the Directory.Packages.props file, the same way that the "top level" of a nodejs project is where the package.json is.

Copy link
Author

Choose a reason for hiding this comment

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

Sorry i feel you might have misunderstood. This is exactly the case @purepani the <NuGetLockFilePath> specifies the lock file for the CPM rather than each individual project. This will be located relative to the Directory.Packages.props at the root of the project.

Now that you mention it we will likely need a property like RestorePackagesWithLockFile but specifically to enable CPM lock files but the lockfile will be located at the root NOT under individual projects

Copy link

@purepani purepani May 23, 2024

Choose a reason for hiding this comment

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

I see so this property is meant for each individual project rather than just the top level of the projects; that makes way more sense.
The way pnpm solves this is by symlinking the dependencies into each workspace after doing a pnpm install at the top level. If you only wanted some dependencies, you could just install it for that group. In this case, it is just the dependencies and not the lock file, but you could certainly symlink the lock file if you wanted(though the only advantage of that approach would be that you wouldn't have to move to the top level every time you wanted to install a package).

(note: I only mentioned pnpm specifically because I know how it works a bit better. Presumably other node package managers work similarly)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community PRs (and linked Issues) created by someone not in the NuGet team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants