Package management CLI, part 1 #2146
Conversation
Copy over the cli changes Fix docs Update tests for the manifest changes Switch to libtest-mimic for package tests Remove an unwrap Knock out some more easy TODOs Remove the index
|
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 | 490.99 |
| foldl arrays 50 | 📈 view plot 🚷 view threshold | 1,704.20 |
| foldl arrays 500 | 📈 view plot 🚷 view threshold | 6,754.60 |
| foldr strings 50 | 📈 view plot 🚷 view threshold | 7,131.70 |
| foldr strings 500 | 📈 view plot 🚷 view threshold | 62,039.00 |
| generate normal 250 | 📈 view plot 🚷 view threshold | 41,002.00 |
| generate normal 50 | 📈 view plot 🚷 view threshold | 1,948.20 |
| generate normal unchecked 1000 | 📈 view plot 🚷 view threshold | 3,324.70 |
| generate normal unchecked 200 | 📈 view plot 🚷 view threshold | 770.48 |
| pidigits 100 | 📈 view plot 🚷 view threshold | 3,185.80 |
| pipe normal 20 | 📈 view plot 🚷 view threshold | 1,507.70 |
| pipe normal 200 | 📈 view plot 🚷 view threshold | 9,925.00 |
| product 30 | 📈 view plot 🚷 view threshold | 848.47 |
| scalar 10 | 📈 view plot 🚷 view threshold | 1,534.60 |
| sum 30 | 📈 view plot 🚷 view threshold | 826.57 |
jneem
force-pushed
the
npm-without-index
branch
from
0cf4b4c to
336b132
Compare
jneem
force-pushed
the
npm-without-index
branch
from
ca59874 to
19b1cb1
Compare
|
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). |
| // If the manifest path isn't given, where should we start looking | ||
| // for it? If there's only one file, we start with its parent directory. |
There was a problem hiding this comment.
When you say the parent directory, this means the directory where this file is located, not one the one above that, right?
| // If there are multiple files, finding a good heuristic | ||
| // is harder. For now, we take the parent directory if it's | ||
| // unique and otherwise we require an explicit manifest | ||
| // path. |
There was a problem hiding this comment.
One possibility would be to look for the common ancestor. But I can see how it might be wrong in some cases (if the files are far apart), so playing safe by explicitly requiring a manifest path sounds good 👍
| Lock { | ||
| /// The path at which to write the lock file. | ||
| /// | ||
| /// Defaults to the filename `package.ncl.lock` in the same directory |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...
| } | ||
| } | ||
|
|
||
| fn print_package_map(map: &PackageMap) { |
There was a problem hiding this comment.
Would that make sense to make this a Display instance instead, or are there other considerations against this idea?
| [package] | ||
| name = "nickel-lang-package" | ||
| description = "The Nickel Package Manager (npm)" | ||
| version = "0.1.0" |
There was a problem hiding this comment.
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.
| } | ||
| } | ||
|
|
||
| impl std::fmt::Display for Error { |
There was a problem hiding this comment.
I haven't tried, but I suppose this means that package management errors are displayed in a way that doesn't follow the other errors look-and-feel? It doesn't have to be done now, but would implementing IntoDiagnostics instead and re-use the Nickel error reporting infrastructure work as well?
| Error::InternalManifestError { path, msg } => { | ||
| write!( | ||
| f, | ||
| "internal error reading the manifest at {}; this is a bug in nickel: {msg}", |
There was a problem hiding this comment.
TODO: reuse the same internal error message that is used in Nickel-lang core. I think there is also a note saying to report the error to the issue tracker.
| } | ||
| Error::NoPackageRoot { path } => write!( | ||
| f, | ||
| "tried to import a relative path ({}), but we have no reference", |
There was a problem hiding this comment.
I'm not sure I would understand this error message without context.
There was a problem hiding this comment.
Addendum: I think "reference" wasn't clear initially. I find "root" better, or "base" maybe, for what it's worth
| Error::LockFileDeserialization { path, error } => { | ||
| write!(f, "lock file {} is invalid: {error}", path.display()) | ||
| } | ||
| Error::NoManifestParent => { |
There was a problem hiding this comment.
What does this error correspond to? Is that possible for a regular file to have no parent?
Co-authored-by: Yann Hamdaoui <[email protected]>
Co-authored-by: Yann Hamdaoui <[email protected]>
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.