Support for index packages #2175

jneem · 2025-02-19T10:35:56Z

This adds support for a package index, but requires that all the index packages have exact version constraints. As a result, we don't yet need to do version resolution.

The main points of this PR are

to define the index format
to decide when/how to refresh the index, and how to download index packages
to add primitive index-manipulation tooling, so that we can start hosting (and using) index packages

github-actions · 2025-02-19T14:29:16Z

Bencher Report

Branch	npm-index
Testbed	ubuntu-latest

Click to view all benchmark results

Benchmark	Latency	microseconds (µs)
fibonacci 10	📈 view plot 🚷 view threshold	385.66 µs
foldl arrays 50	📈 view plot 🚷 view threshold	1,542.80 µs
foldl arrays 500	📈 view plot 🚷 view threshold	5,700.30 µs
foldr strings 50	📈 view plot 🚷 view threshold	6,541.00 µs
foldr strings 500	📈 view plot 🚷 view threshold	56,608.00 µs
generate normal 250	📈 view plot 🚷 view threshold	41,402.00 µs
generate normal 50	📈 view plot 🚷 view threshold	1,862.20 µs
generate normal unchecked 1000	📈 view plot 🚷 view threshold	2,732.80 µs
generate normal unchecked 200	📈 view plot 🚷 view threshold	633.06 µs
pidigits 100	📈 view plot 🚷 view threshold	2,716.60 µs
pipe normal 20	📈 view plot 🚷 view threshold	1,271.40 µs
pipe normal 200	📈 view plot 🚷 view threshold	8,273.00 µs
product 30	📈 view plot 🚷 view threshold	740.17 µs
scalar 10	📈 view plot 🚷 view threshold	1,301.30 µs
sum 30	📈 view plot 🚷 view threshold	731.57 µs

🐰 View full continuous benchmarking report in Bencher

yannham

Partial review. Stopped just before package/index/mod.rs, will continue later.

Cargo.toml

yannham · 2025-03-03T15:29:41Z

cli/src/package.rs

+    ///    names *must* match the package id in the index, and you must ensure that
+    ///    the version you push to github matches the SHA-1 hash in the index.
+    /// 7. Open a pull request to `github.com/nickel-lang/nickel-mind` to make your index
+    ///    modifications public.


I wonder if the whole description of the process really belongs to the CLI help message. What do you think about putting it somewhere (manual maybe?) and linking to the page here?

Sure, I've added a manual page about package management in general and included this there.

yannham · 2025-03-03T15:30:34Z

cli/src/package.rs

+                let snap = manifest.snapshot_dependencies(config.clone())?;
+                let index = PackageIndex::shared(config.clone())?;
+                let resolution = resolve::resolve(&manifest, snap, index, config)?;
+                let package_map = resolution.package_map(&manifest)?;
                eprintln!("{package_map}");


left-over debug statement?

No, printing the package map is the main job of this command. (It maybe be time to remove the command, though. It was mainly just to help debug things while implementing...)

yannham · 2025-03-03T15:31:20Z

cli/src/package.rs

@@ -92,7 +125,23 @@ impl PackageCommand {
                    ..config
                };

-                manifest.snapshot_dependencies(config)?;
+                manifest.snapshot_dependencies(config.clone())?;


Out of curiosity, why do we need to take a config as an owned value?

Hm, I think maybe I can get rid of this one. In some other places it's just convenient to store copies of the config so that we can figure out where various files go.

Indeed, we now no longer store Config in Snapshot.

yannham · 2025-03-03T15:32:50Z

cli/src/package.rs

+    ///    the version you push to github matches the SHA-1 hash in the index.
+    /// 7. Open a pull request to `github.com/nickel-lang/nickel-mind` to make your index
+    ///    modifications public.
+    PublishToLocalIndex {


Nitpick: this sounds a bit long for a command name.

Renamed to "publish".

flock/Cargo.toml

git/src/lib.rs

yannham · 2025-03-03T15:41:50Z

package/src/error.rs

+                write!(f, "package {id}@{version} is already present in the index")
+            }
+            Error::PackageIndexSerialization { error, pkg } => {
+                write!(f, "error serializing package. Failed package {pkg:?}, caused by {error}\n{INTERNAL_ERROR_MSG}")


There is some duplication in the error message. Maybe it's a mix of two different versions?

I shorted the message. For some reason, rustfmt took that as a cue to spread the code over more lines 🤔

package/src/index/lock.rs

yannham

Second chunk: I stopped just before resolve.rs.

package/src/index/mod.rs

yannham · 2025-03-06T16:45:11Z

package/src/index/mod.rs

+
+    fn ensure_downloaded_to(&self, index_id: &PreciseId, target_dir: &Path) -> Result<(), Error> {
+        let PreciseId::Github { org, name, commit } = index_id;
+        let url = format!("https://github.com/{org}/{name}.git");


Can we have any injection/url encoding issue here, or are org and name sanitized as an invariant?

The manifest format ensures that org and name are nickel idents (in the lexer sense, not the "interned string" sense). I'll add a comment.

Actually, I take it back: the FromStr implementation checks the names, but the manifest doesn't go through that. I'll fix this...

yannham · 2025-03-06T16:56:37Z

package/src/index/mod.rs

+                return Ok(());
+            }
+
+            eprintln!(


random thought: maybe we will want to have separate macros for outputting status update and errors (so that we can style them a bit, like putting 🫠 in front of errors (or not))

I added info and warn macros that just wrap eprintln for now.

package/src/index/mod.rs

yannham · 2025-03-06T17:01:21Z

package/src/index/mod.rs

+    }
+}
+
+static ID_REGEX: LazyLock<Regex> =


I didn't know something like LazyLock existed in the standard library. I wonder if this deprecates the usage we make of lazy_static elsewhere in Nickel.

Yes, I think so. It's only been in the stdlib since 1.80

package/src/index/serialize.rs

yannham · 2025-03-06T17:12:23Z

package/src/lib.rs

+    }
+}
+
+/// The same as [`Dependency`], but only for the packages that have fixed, unresolvable, versions.


What does "unresolvable" means in this context? Just we don't get to pick a different one, like rigid version constraints, or something like a precise git ref?

It means that the resolver doesn't care about (or get to pick) the version. Or in other words, a path or git dependency.

Git deps don't need precise versions to be "unresolvable": if a git dep points at HEAD, then we just fetch HEAD and that's the package we depend on. The resolver won't consider whether it should take a different revision.

On the other hand, rigid version constraints on an index package are still "resolvable" in this sense, because their presence affects the resolver's decisions.

I see. Maybe it would be clearer to mention it explicitly ("git ref or path"), because "version" is overloaded quite a bit in our context (I initially though version would only correspond to version bounds, which is why "unresolvable version" sounded a bit paradoxical).

yannham · 2025-03-06T17:15:37Z

package/src/manifest.rs

+/// This is currently identical to `IndexDependency`, but we keep
+/// them separate so it's clear which things are part of our public
+/// serialization API.
+struct IndexDependencyFormat {


isn't this struct defined two times, here in manifest but also in index::serialize?

Right, but one of them defines how to deserialize the manifest (going through the RichTerm deserializer) and the other defines how to serialize/deserialize the index through serde_json. The current differences are just cosmetic for now (for some reason I decided to have "github" lower case in the index, but it needs to be upper case for the manifest to match the 'Github enum), but I think there's some value in allowing them to be different. For example, if we add support for other forges then an old nickel version reading 'Codeberg from a manifest is allowed to error out immediately (because it won't know how to fetch that dependency), but an old nickel version shouldn't fail to deserialize an index that contains a package hosted on codeberg. (It can fail if the dependency tree contains such a package, though)

package/src/manifest.rs

Co-authored-by: Yann Hamdaoui <[email protected]>

jneem force-pushed the npm-index branch from f96d589 to 687627a Compare February 19, 2025 14:00

jneem marked this pull request as ready for review February 27, 2025 13:55

jneem requested a review from yannham February 27, 2025 13:59

jneem mentioned this pull request Mar 3, 2025

Support for index packages with version resolution. #2183

Draft

yannham reviewed Mar 3, 2025

View reviewed changes

yannham reviewed Mar 6, 2025

View reviewed changes

jneem added 23 commits March 11, 2025 13:19

Add the index, make it compile

e596c2a

Enforce no compatible versions for now

bc92313

Add a package id validation contract

bb01476

Use the manifest validation for package ids

1ede5d8

Add flock crate

613326f

Fix serialization issue

566c19e

Add a lock to the index

f791a4e

Get it building with resolutions

adb1c45

Try adding windows deps properly

6994179

clippy

309451b

maybe fix ci

e2688fc

fix build

ab82b40

Fix one test

3053971

Fix the other test

0870c98

Add LockFile::walk_tree, but I think we don't need it

d1bf647

Refactor to reduce duplication

8c323aa

Fix index writing

640f602

Comments and refactor

77b65b9

checkpoint

59e1eb7

Fix the test by changing it

3b7b00e

Simplify the type

1e1fc02

Add first index tests

cdcd863

clean up some unwraps

fc26ef6

jneem and others added 19 commits March 11, 2025 13:20

Add back the missing package tests

1622226

Add integration tests for index deps

e9640ec

Forgot to cargo insta it

18aa735

Missing snapshort

dd7484e

Clean up a few todos

8671e87

paths on windows

1fb5889

fix build

548f029

Move package docs to the manual, address some comments.

d7d626b

Remove ignored tests

d09ecdd

Review comments

3da9e1b

Update package/src/index/lock.rs

c0c09aa

Co-authored-by: Yann Hamdaoui <[email protected]>

Don't store Config in Snapshot.

1170de1

Update package/src/index/mod.rs

9d4295d

Co-authored-by: Yann Hamdaoui <[email protected]>

Update package/src/index/mod.rs

d92afcf

Co-authored-by: Yann Hamdaoui <[email protected]>

Update package/src/index/scrape.rs

c2be180

Co-authored-by: Yann Hamdaoui <[email protected]>

Update package/src/index/serialize.rs

c038e5f

Co-authored-by: Yann Hamdaoui <[email protected]>

Review comments

54f0b44

Change from slash to colon

9acf198

Update comment

f9371ee

jneem force-pushed the npm-index branch from 109482e to f9371ee Compare March 11, 2025 06:20

Fix build and tests

b2c3def

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for index packages #2175

Support for index packages #2175

jneem commented Feb 19, 2025 •

edited

Loading

github-actions bot commented Feb 19, 2025 •

edited

Loading

yannham left a comment

yannham Mar 3, 2025

jneem Mar 6, 2025

yannham Mar 3, 2025

jneem Mar 6, 2025

yannham Mar 3, 2025

jneem Mar 6, 2025

jneem Mar 7, 2025

yannham Mar 3, 2025

jneem Mar 6, 2025

yannham Mar 3, 2025

jneem Mar 6, 2025

yannham left a comment

yannham Mar 6, 2025

jneem Mar 7, 2025

jneem Mar 7, 2025

yannham Mar 6, 2025

jneem Mar 7, 2025

yannham Mar 6, 2025

jneem Mar 7, 2025

yannham Mar 6, 2025 •

edited

Loading

jneem Mar 7, 2025

yannham Mar 7, 2025

yannham Mar 6, 2025

jneem Mar 7, 2025

Support for index packages #2175

Are you sure you want to change the base?

Support for index packages #2175

Conversation

jneem commented Feb 19, 2025 • edited Loading

github-actions bot commented Feb 19, 2025 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yannham Mar 6, 2025 • edited Loading

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 19, 2025 •

edited

Loading

github-actions bot commented Feb 19, 2025 •

edited

Loading

yannham Mar 6, 2025 •

edited

Loading