Package management CLI, part 1 #2146

jneem · 2025-01-24T15:25:43Z

This is the first part of the package manager CLI, hidden behind a "experimental-package" feature flag. It initially supports only git and path dependencies.

Allow the git repo realization to check for up-to-date git repos using an existing lock file.
Thoroughly test the manifest deserialization
Test the is-lock-file-up-to-date logic
Integration tests involving the cli
Progress messages during git operations

github-actions · 2025-01-24T15:58:02Z

Bencher Report

Branch	npm-without-index
Testbed	ubuntu-latest

Click to view all benchmark results

Benchmark	Latency	microseconds (µs)
fibonacci 10	📈 view plot 🚷 view threshold	396.97
foldl arrays 50	📈 view plot 🚷 view threshold	1,560.70
foldl arrays 500	📈 view plot 🚷 view threshold	5,897.50
foldr strings 50	📈 view plot 🚷 view threshold	5,893.60
foldr strings 500	📈 view plot 🚷 view threshold	51,366.00
generate normal 250	📈 view plot 🚷 view threshold	45,770.00
generate normal 50	📈 view plot 🚷 view threshold	1,907.50
generate normal unchecked 1000	📈 view plot 🚷 view threshold	2,778.40
generate normal unchecked 200	📈 view plot 🚷 view threshold	665.95
pidigits 100	📈 view plot 🚷 view threshold	2,780.90
pipe normal 20	📈 view plot 🚷 view threshold	1,285.40
pipe normal 200	📈 view plot 🚷 view threshold	8,566.40
product 30	📈 view plot 🚷 view threshold	751.43
scalar 10	📈 view plot 🚷 view threshold	1,318.00
sum 30	📈 view plot 🚷 view threshold	750.19

🐰 View full continuous benchmarking report in Bencher

jneem · 2025-01-30T12:08:42Z

I haven't added a progress bar to the git downloads, but I think this is ready for a first look.

This does auto-updating of the lock-file, like cargo but unlike npm. I haven't yet implemented re-using the standard library across evaluation of multiple manifests, but I think it shouldn't be too hard (certainly easier than the general case of #2140 where you want to re-use work across nickel invocations).

yannham

Just a partial review for now. Note to self: next item on the stack is package/src/lib.rs.

cli/src/input.rs

yannham · 2025-02-06T09:30:41Z

cli/src/package.rs

+    Lock {
+        /// The path at which to write the lock file.
+        ///
+        /// Defaults to the filename `package.ncl.lock` in the same directory


It's a detail, but I think usually package managers don't keep the .ncl equivalent in the lock file: flake.nix -> flake.lock, Cargo.toml -> Cargo.lock, Pipefile -> Pipefile.lock, foo_bar.gemspec -> Gemfile.lock, etc.

Right, but I was worried about colliding with npm's (the other npm 😉) package.lock. Maybe it's better to avoid a collision by having a more nickel-specific name than "package", though...

I see. Yes, maybe having a more specific name than "package" could be helpful in general, if only there are projects with both an NPM manifest an a Nickel manifest (or even imagine that you handle everything with Nickel, so you would like to write your "package.json" as a "package.ncl" instead and generate it).

Ok, I realized while fiddling with the website that actually npm uses "package-lock.json", not "package.lock". So there would be no conflict to go with "package.lock", but maybe a more distinctive name is better anyway. What do you think about Nickel.ncl/Nickel.lock? Or Nickelfile.ncl/Nickelfile.lock?

cli/src/package.rs

yannham · 2025-02-06T09:45:13Z

package/Cargo.toml

+[package]
+name = "nickel-lang-package"
+description = "The Nickel Package Manager (npm)"
+version = "0.1.0"


It's not very important for this PR, but I wonder if we shouldn't make it target either nickel-lang-core or the Nickel version (nickel-lang-cli) (probably the latter, since it does provide new CLI commands), to avoid the proliferation of uncorrelated versions. On the other hand, this removes the possibility of updating those crates independently.

I think it makes some sense to have the version in sync with nickel-lang-cli. It does require a little bit of manual intervention to keep them in sync: I can put version.workspace = true in package/Cargo.toml, but I need to repeat the "1.9.0" in the workspace's dependencies list.

The release scripts is capable of handling that (we already do it for other crates like NLS). Though we should update the script as part of this PR, to make sure everything is consistent.

Right, but none of the other non-"independent" crates are a dependency of other crates in the workspace. Naively adding package to the list of crates gives

-- Updating cross-dependencies... -- Patching cross-dependency lsp-harness in ./Cargo.toml to version 0.1.0 -- Patching cross-dependency nickel-lang-core in ./Cargo.toml to version 0.11.0 -- Patching cross-dependency nickel-lang-package in ./Cargo.toml to version { "workspace": true } sed: -e expression #1, char 87: unterminated `s' command ++ Unexpected exit. Cleaning up...

(Also, I guess we should add git and vector)

package/src/error.rs

yannham

I can't pretend I followed all the details, but it looks good after a pass. The extensive tests are also great and reassuring. I just wonder if all the tests using git commands explicitly wouldn't break tests on Windows ?

package/src/lib.rs

yannham · 2025-02-14T15:08:45Z

package/src/lock.rs

+        if self.id == 0 {
+            write!(f, "{}", self.name)
+        } else {
+            write!(f, "{} {}", self.name, self.id)


Naive question: is it possible that a package name has a space in the name?

You can't use such a package because the import syntax for packages requires the package name to be an identifier ("identifier" in the sense of the nickel lexer). But it's a good point, nickel_lang_package should check this.

I've added this check to std.package.Manifest

package/src/lock.rs

package/src/manifest.rs

package/src/snapshot.rs

package/src/version.rs

yannham · 2025-02-14T15:37:45Z

package/tests/integration.rs

+
+        // The rust stdlib doesn't have anything for recursively copying a directory. There are
+        // some crates for that, but it's easier just to shell out.
+        run(Command::new("cp")


Wouldn't that break the CI on Windows?

Enables package management for git and path packages, when the package-experimental feature is enabled. Co-authored-by: Yann Hamdaoui <[email protected]>

jneem · 2025-02-18T05:42:00Z

Ok, I think the main outstanding thing is to settle on a name for the manifest and lock files.

I'm pretty sure the windows CI is working ok with git and cp -- the CI already caught various small differences between git on windows and linux (e.g. it doesn't auto-detect the username on windows, so you need to provide it), so I'm pretty sure that it's running and passing in CI.

yannham · 2025-02-18T10:36:03Z

core/stdlib/std.ncl

+      in
+      let
+        semver_req_re = "(%{partial_semver_re})|(%{semver_equals_req_re})",
+        IdentKeys =


Should we expose IdentKeys as well? While it feels like a small self-explanatory contract, exposing it makes it discoverable, re-usable, and queri-able (if we add documentation). Though it also makes it part of the backward-compatibility. So I don't have a strong opinion either way.

I've been playing with json schema lately, and it seems pretty common to have a dict with a regex for the key names. What if we exposed, say, std.record.KeysMatch which could get used like

{ foo = 1 } | std.record.KeysMatch "^[a-z]+*"

(basically, I'm worried that IdentKeys is a bit too specific to be worth making public)

Yes, I even think CUE provide built-in syntax for that. Nitpick: should we use FieldMatches instead to follow Nickel's jargon? Although the issue with field is that it designates both the content and the name. Could use FieldNameMatches, but it's longer. That being said, we have std.record.fields which return field names, so FieldMatches might be reasonable.

Fair for IdentKeys, we can keep it local for now.

yannham · 2025-02-18T10:44:09Z

I'm pretty sure the windows CI is working ok with git and cp -- the CI already caught various small differences between git on windows and linux (e.g. it doesn't auto-detect the username on windows, so you need to provide it), so I'm pretty sure that it's running and passing in CI.

I see, this makes our life simpler (it was just a random thought - I haven't used Windows in ages so I wondered if the CLI argument syntax was different, I remember using /).

As per the bikeshedding for the file name:

nickel.ncl/nickel.lock: clear, no risk of conflict with other tools, small name. The only disadvantage is that we tied it to the name of the language, so it might not be 100% clear for a newcomer that this is related to package management? For example if we ever implement file-based configuration for the interpreter itself, maybe it could also be nickel.ncl?
nickelfile.ncl / nickelfile.lock: I'm not sure why Pip and Gem adds the file prefix, because it doesn't seem to add much meaning.

Maybe something with both nickel and another word that indicates it's related to package management would work as well? What do yo think about e.g. nickel-pkg{.ncl,.lock}, nickel-package{.ncl, .lock}, nickel-manifest{.ncl,.lock} or something along those lines? Also, we can post-pone the bikeshedding and merge this first: it's an experimental feature, and 1.10 has just been released, so we have time.

jneem · 2025-02-18T10:54:20Z

nickel-pkg.ncl sounds good to me, but I wonder if we should capitalize it? It seems to be common, and at least cargo explicitly explains their rationale.

yannham · 2025-02-18T11:38:14Z

I initially didn't like the inconsistency of upper casing this specific file but I think their rationale makes sense. Let's start with Nickel-pkg.%{extension} and review later if needed?

jneem force-pushed the npm-without-index branch 2 times, most recently from ca59874 to 19b1cb1 Compare January 28, 2025 06:46

jneem marked this pull request as ready for review January 30, 2025 12:05

jneem requested a review from yannham January 30, 2025 12:08

yannham reviewed Feb 6, 2025

View reviewed changes

jneem force-pushed the npm-without-index branch from 8fdcef9 to 32a68a0 Compare February 11, 2025 10:23

yannham approved these changes Feb 14, 2025

View reviewed changes

jneem and others added 2 commits February 18, 2025 11:40

Add the package crate.

a2309e2

Enables package management for git and path packages, when the package-experimental feature is enabled. Co-authored-by: Yann Hamdaoui <[email protected]>

Add check for valid package names

d4f86e2

jneem force-pushed the npm-without-index branch from 3196a84 to d4f86e2 Compare February 18, 2025 05:35

Less multiline strings

0caee37

yannham reviewed Feb 18, 2025

View reviewed changes

jneem added 2 commits February 18, 2025 20:18

Rename the manifest file

737dcbc

Fix warning

d8a8571

jneem added this pull request to the merge queue Feb 19, 2025

Merged via the queue into master with commit 02439ec Feb 19, 2025
5 checks passed

jneem deleted the npm-without-index branch February 19, 2025 01:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Package management CLI, part 1 #2146

Package management CLI, part 1 #2146

jneem commented Jan 24, 2025 •

edited

Loading

github-actions bot commented Jan 24, 2025 •

edited

Loading

jneem commented Jan 30, 2025

yannham left a comment

yannham Feb 6, 2025

jneem Feb 6, 2025

yannham Feb 11, 2025

jneem Feb 18, 2025

yannham Feb 6, 2025

jneem Feb 11, 2025

yannham Feb 11, 2025

jneem Feb 11, 2025

yannham left a comment

yannham Feb 14, 2025

jneem Feb 18, 2025

jneem Feb 18, 2025

yannham Feb 14, 2025

jneem commented Feb 18, 2025

yannham Feb 18, 2025

jneem Feb 18, 2025

yannham Feb 18, 2025

yannham commented Feb 18, 2025 •

edited

Loading

jneem commented Feb 18, 2025

yannham commented Feb 18, 2025

Package management CLI, part 1 #2146

Package management CLI, part 1 #2146

Conversation

jneem commented Jan 24, 2025 • edited Loading

github-actions bot commented Jan 24, 2025 • edited Loading

jneem commented Jan 30, 2025

yannham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yannham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jneem commented Feb 18, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yannham commented Feb 18, 2025 • edited Loading

jneem commented Feb 18, 2025

yannham commented Feb 18, 2025

jneem commented Jan 24, 2025 •

edited

Loading

github-actions bot commented Jan 24, 2025 •

edited

Loading

yannham commented Feb 18, 2025 •

edited

Loading