Fix error message when trying to run a nonexistent path

[fschutt]

Fixes #3404.

[Michael-F-Bryan]

I think it's time to refactor the SplitVersion type to be something more closely matched to how we treat SplitVersion in practice.

We're actually reusing SplitVersion for several different concepts, so it'd be better to split them up into their own bespoke types. From what I can tell, depending on the context, SplitVersion might mean one of the following:

• The path to a package folder/file on disk
• A package stored on a registry (where registry URL and namespace are optional)
• A package stored on an arbitrary URL
• A particular command inside a package

Where certain fields are assumed to be None (or ignored) depending on how it is being used.

In my opinion, it'd be better to take a more data-oriented approach. Maybe something like this?

struct PackageSpec {
  location: PackageLocation,
  version: Option<semver::Version>,
}

enum PackageLocation {
  Url(url::Url),
  Registry {
    registry: String,
    namespace: Namespace,
    package_name: String,
  }
}

enum Namespace {
  Global,
  Underscore,
  Organisation(String),
}

struct Command {
  package: PackageSpec,
  command: Option<String>,
}

I know it's a pain, but we shouldn't keep adding extra logic and semantics onto the same struct.

[syrusakbary]

I'd like to have few more tests:

1. Testing that a proper formatted package name actually is searched for in a registry.
2. Assure that anything else that doesn't look like a package is not searched for in a registry and is treated like a file
3. Assure that non-urls (such as wapm.io without the https://) are actually treated like files

[theduke]

I would approach this differently:

• first, try to normalize the argument with Path::canonicalize()

• Then, check if it's a valid file that exists ( and has a .wasm or .wat extension?), and run it if that's the case
  (there is already similar logic above, but only for cases where there is a wapm.toml)

• If that doesn't work, try to parse it as a package identifier

[syrusakbary]

To add on top. If is not "parseable" as a package identifier, it should not fall back into the registry.

Basically, here's the logic:

• Does the file exists? If so, try to run it
• If the file doesn't exist. Does it look like a package name? If so, transform it to a url
• Is the provided path an url? Try to download it
• Is the provided path not an url, show an error mentioning the file path doesn't work

[syrusakbary]

We need to follow this logic

[syrusakbary]

Note for us in the future: we can enforce that namespaces exists always (for now), as a tradeoff towards simplification

[syrusakbary]

If no namespace is given, we assume the namespace as being _

[theduke]

As a general note: we really need a solid parser for package identifiers.
It's quite tricky to parse, and we also need that in deploy.

Should probably go into the Pirita crate.

@Michael-F-Bryan @fschutt

[syrusakbary]

It's quite tricky to parse, and we also need that in deploy.
Should probably go into the Pirita crate.

I disagree with this. It should probably go on the registry crate?

[Michael-F-Bryan]

It should probably go on the registry crate?

Maybe.

On one hand, package naming is something inherent to the way we communicate with WAPM and the wasmer-registry crate is about communicating with WAPM, but on the other hand I don't want projects to start pulling in tokio and reqwests and a GraphQL client just because they want a PackageName struct... and hiding those dependencies behind a #[cfg] is not the answer 😉

On the other hand, the webc and pirita crates are more targeted towards the WEBC format and still have their own non-trivial dependencies.

How about we leave them as a bunch of private types in a dedicated wasmer_cli::package_name module until we can think of a better long-term home?

[fschutt]

@Michael-F-Bryan parsing can currently not be in any other crate than the CLI crate because the parser needs to know which commands are prohibited (for example the command init would not be parseable as a valid command).

If that restriction was lifted (could be done by refactoring), I can put it into a separate crate such as a guid crate that handles all parsing (and then we can use it from wasmer, pirita and deploy at the same time).

[Michael-F-Bryan]

@fschutt, I don't think implementation details from wasmer some-package should be bleeding into the parsing code for package names. That would mean the wasmer CLI effectively gets baked into the definition of a package name because every time we add a new subcommand we need to add another name to the blacklist.

If that restriction was lifted (could be done by refactoring), I can put it into a separate crate such as a guid crate that handles all parsing (and then we can use it from wasmer, pirita and deploy at the same time).

This is my preferred option.

Ideally the command-line parsing and package name parsing would be completely separate, where our command-line parsing would do its normal parsing, then if it gets some sort of "unknown subcommand" error it'll re-parse the command-line arguments as a wasmer_cli::commands::run::Run command.

[fschutt]

Okay:

1. Testing that a proper formatted package name actually is searched for in a registry.

This is done by just running the integration tests, which download the package from the registry.

2. Assure that anything else that doesn't look like a package is not searched for in a registry and is treated like a file

This is done in the test_split_version test, where the errors indicate that it's being treated as a file

3. Assure that non-urls (such as wapm.io without the https://) are actually treated like files

Also done in the test_split_version test.

[theduke]

Why whould we have a separate registry flag if one can provide a URL?

[fschutt]

Because we might want to install a package from wapm.dev / wapm.io instead of going through wasmer login / wapm config set registry.url

[theduke]

But that would just be wasmer run https://wapm.dev/ns/name then?

[theduke]

Downloading a package doesn't require login.

[fschutt]

It is useful if you want to specify the registry for a package, but you don't want to mess with the wapm.config. Ex. by default the default registry in prod is wapm.io, but the test are always locked to wapm.dev, since some packages don't exist on .io.

wasmer run {url} only downloads + runs webc files, not tar.gz packages.

I also don't think you can specify versions yet like /name@version

For now I'd like to leave it as is, at least until we've migrate the website documentation from wapm run to wasmer run.

[theduke]

I don't really think it makes sense to collect multiple errors here.
Should just return something like "InvalidPackageSpecifier" here, that's enough.

[fschutt]

No, this is on purpose, so we can print something like:

wasmer run qjsj.temp
error: Invalid value "qjsj.temp" for '<FILE>': 
    parsing as file: file does not exist: qjsj.temp
    parsing as URL: relative URL without a base
    parsing as package: Invalid package: "qjsj.temp"
    parsing as command: invalid command: qjsj.temp


For more information try --help

This way it's much easier to debug if something fails. There are cases where you can't distinguish between a package and a filepath, so if we error on one case, this might cause problems when debugging such cases.

[theduke]

I don't think that's very helpful for the user, rather just confusing.

I'd just print a generic error message like "error:

Invalid run target "qjsj.temp".
You can specify a local path, run packages from the Wasmer registry with ... or ...
(whatever, a solid explanation)

[fschutt]

Superseded by #3416.