Rokit panics when running rokit install or rokit add

[kennethloeffler]

Right now, Rokit panics when I run rokit install or rokit add:

ken@crawling witch-game % rokit add rojo
thread 'main' panicked at /Users/ken/projects/programming/rokit/lib/discovery/mod.rs:131:25:
attempt to subtract with overflow
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Here's the code in question:
https://github.com/filiptibell/rokit/blob/d41b102f40cb7d58c083803d540d154a7e4ea645/lib/discovery/mod.rs#L130-L139

cwd will almost always be shallower than the manifest path, in which case this code will panic.

DiscoveredManifest.path is unused except for in this function. I'm not sure what purpose it serves, but it seems like we can safely remove it, or else use a signed integer type.

[filiptibell]

I think the initial idea here was to use DiscoveredManifest for discovery when Rokit is going to run a tool, and to then be able to sort these discovered manifests (by depth) to find the proper tool - discover_tool_spec does exactly this though and is optimized for that one specific use-case so those fields were left unused.

I think keeping path here for any future consumers of the Rokit lib might be nice, and isize for relative depth definitely makes more sense than usize, let's change that.

[kennethloeffler]

Should we keep the same way of computing the depth then (cwd.components().count() - path.components().count())? I think it produces weird results:

• cwd at ~/projects/games/witch-game with manifest at ~/projects/games/witch-game/rokit.tomlhas a depth of -1
• cwd at ~/projects/games/witch-game/some-other-dir with manifest at ~/projects/games/witch-game/rokit.toml has a depth of 0
• cwd at ~/projects/games/witch-game/some-other-dir/another-dirwith manifest at ~/projects/games/witch-game/rokit.toml has a depth of 1

I think we should subtract one from manifest depth and swap the order of subtraction (path.components().count() - 1 - path.components().count()), which IMO gives more reasonable results:

• cwd at ~/projects/games/witch-game with manifest at ~/projects/games/witch-game/rokit.tomlhas a depth of 0
• cwd at ~/projects/games/witch-game/nested-once with manifest at ~/projects/games/witch-game/rokit.toml has a depth of -1
• cwd at ~/projects/games/witch-game/nested-once/nested-twicewith manifest at ~/projects/games/witch-game/rokit.toml has a depth of -2

Alternatively, we could just subtract one from the manifest depth and keep the order the same, if you want to count up instead?

[filiptibell]

Thinking about it some more, yeah, it actually makes sense as a usize, like you mention the weirdness is from comparing number of dir path components with number of file path components, and the - op being swapped. Does path.parent().unwrap().components().count() - cwd.components().count() look right to you?

Rokit should never discover a manifest like ~/projects/games/witch-game/rokit.toml if cwd is at ~/projects/games or some upper directory, and this should be guaranteed.

[kennethloeffler]

I think that makes sense - but if manifest paths will always have fewer or the same number of components as cwd (except for the case where manifest is a direct child of cwd), and we want depth to count up, then we should continue using a usize and keep the same order, but use the manifest path's parent for its depth. Maybe "height" would be a better name?

And should we worry about users running Rokit in directories that are shallower than the Rokit/Aftman/Foreman home directories?

[Dekkonot]

And should we worry about users running Rokit in directories that are shallower than the Rokit/Aftman/Foreman home directories?

I fairly regularly run tools in the root of my drive, but I'm not sure if a toolchain manager should actually work there. Feels like there's arguments in both directions and I cannot imagine people being too mad as long as we clearly document the behavior.

[filiptibell]

And should we worry about users running Rokit in directories that are shallower than the Rokit/Aftman/Foreman home directories?

Hmm. I'm not really sure what depth (or height) value makes sense for the home manifest at all, actually.
The intent was for it to be some kind of "search depth", so:

• Should the home manifest depth be usize::MAX since it's really a kind of fallback?
• Or if you're at the root of the drive, is it.. isize::MIN for negative depth?

Maybe this depth value is best left out after all, and we only keep path. Rokit doesn't use it and any future lib consumer can do their own path comparisons instead.