Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix error message when trying to run a nonexistent path #3407

Closed
wants to merge 34 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
07f5aba
Improve error message when package / path wasn't found
fschutt Dec 5, 2022
c29ee26
Fixed error that led to not being able to run wasmerio/wasmer
fschutt Dec 5, 2022
ea26f95
Fix merge errors
fschutt Dec 6, 2022
a2e9496
Move SplitVersion parsing to separate file
fschutt Dec 6, 2022
0888e5a
Remove split_version from registry
fschutt Dec 6, 2022
6555e5e
Finish moving SplitVersion to own file
fschutt Dec 6, 2022
c357571
Refactor SplitVersion and add unit tests for parsing
fschutt Dec 6, 2022
7de0210
Adjust unit test and handle parsing in clap instead
fschutt Dec 6, 2022
c15adf1
Temporarily remove all the run() code and refactor it to compile
fschutt Dec 6, 2022
b4b84be
Get wasmer to compile again
fschutt Dec 6, 2022
6d3aeb4
Fix CLI parsing of wasmer run command
fschutt Dec 6, 2022
26f4b43
Add code to lookup package information from SplitVersion
fschutt Dec 7, 2022
92cefac
Get wasmer run working for all command types
fschutt Dec 7, 2022
4ee0c97
Fix running a package directory instead of a file
fschutt Dec 7, 2022
8ca5a5b
Fix SplitVersion::parse test
fschutt Dec 7, 2022
8892a61
Fix run-url test
fschutt Dec 7, 2022
de7118d
Fix make lint
fschutt Dec 7, 2022
627285d
Fix registry not being recongnized on Linux
fschutt Dec 7, 2022
6cb47c1
Linux: rework from_binfmt_args_fallible
fschutt Dec 7, 2022
2a4e91a
Disable some tests on Windows (already tested as part of integration)
fschutt Dec 7, 2022
71ff457
Fix make lint
fschutt Dec 7, 2022
9af38d1
Fix test_split_version
fschutt Dec 7, 2022
8cb9fca
Add fetching for webc files back, rework SplitVewrsion -> PackageSource
fschutt Dec 8, 2022
9751fbc
Fix test_split_version
fschutt Dec 8, 2022
f633e61
Debug why redirects aren't working for application/webc
fschutt Dec 8, 2022
ab6f6b9
Add functions for tar.gz fetching back
fschutt Dec 8, 2022
50c75d4
Add caching for installed commands
fschutt Dec 8, 2022
7a48f96
Work on error messages for non-existent file paths
fschutt Dec 8, 2022
7c3b0eb
Update spinner when installing packages
fschutt Dec 8, 2022
a7ff5db
Add support for command immediately executing commands
fschutt Dec 8, 2022
cb312bc
Fix caching for local packages
fschutt Dec 8, 2022
4010316
Fix caching + fix make lint
fschutt Dec 8, 2022
886392f
Fix CLI integration test + add more tests
fschutt Dec 8, 2022
5d40796
split_version -> package_source
fschutt Dec 8, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 60 additions & 17 deletions lib/cli/src/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -344,14 +344,34 @@ fn test_split_version() {
);
}

#[derive(Debug)]
pub(crate) enum SplitVersionError {
InvalidCommandName(anyhow::Error, Option<anyhow::Error>),
fschutt marked this conversation as resolved.
Show resolved Hide resolved
fschutt marked this conversation as resolved.
Show resolved Hide resolved
Other(anyhow::Error),
}

impl std::error::Error for SplitVersionError {}
fschutt marked this conversation as resolved.
Show resolved Hide resolved

impl fmt::Display for SplitVersionError {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
match self {
SplitVersionError::InvalidCommandName(i, _) => {
write!(f, "invalid command name")?;
i.fmt(f)
}
SplitVersionError::Other(i) => i.fmt(f),
}
}
}

impl SplitVersion {
pub fn parse(s: &str) -> Result<SplitVersion, anyhow::Error> {
pub fn parse(s: &str) -> Result<SplitVersion, SplitVersionError> {
s.parse()
}
}

impl FromStr for SplitVersion {
type Err = anyhow::Error;
type Err = SplitVersionError;

fn from_str(s: &str) -> Result<Self, Self::Err> {
let command = WasmerCLIOptions::command();
Expand All @@ -364,6 +384,13 @@ impl FromStr for SplitVersion {

let mut no_version = false;

if s.starts_with('/') || s.contains("..") {
fschutt marked this conversation as resolved.
Show resolved Hide resolved
return Err(SplitVersionError::InvalidCommandName(
anyhow!("package {s:?} seems to be a path"),
None,
));
}

let captures = if re1.is_match(s) {
re1.captures(s)
.map(|c| {
Expand Down Expand Up @@ -402,34 +429,48 @@ impl FromStr for SplitVersion {
})
.unwrap_or_default()
} else {
return Err(anyhow::anyhow!("Invalid package version: {s:?}"));
// maybe a command
return Err(SplitVersionError::Other(anyhow::anyhow!(
"Invalid package version: {s:?}"
)));
};

let mut namespace = match captures.get(1).cloned() {
Some(s) => s,
None => {
return Err(anyhow::anyhow!(
return Err(SplitVersionError::Other(anyhow::anyhow!(
"Invalid package version: {s:?}: no namespace"
))
)))
}
};

let name = match captures.get(2).cloned() {
Some(s) => s,
None => return Err(anyhow::anyhow!("Invalid package version: {s:?}: no name")),
None => {
return Err(SplitVersionError::Other(anyhow::anyhow!(
"Invalid package version: {s:?}: no name"
)))
}
};

let mut registry = None;
if namespace.contains('/') {
let (r, n) = namespace.rsplit_once('/').unwrap();
let mut real_registry = r.to_string();
if !real_registry.ends_with("graphql") {
real_registry = format!("{real_registry}/graphql");
let real_registry = r.to_string();

if !real_registry.contains('.') {
let err = anyhow::anyhow!(
"expected a dot if using a URL shorthand (e.g. {real_registry}.com)"
);
return Err(SplitVersionError::InvalidCommandName(
err,
Some(anyhow::anyhow!(
"invalid registry or namespace {real_registry:?}"
)),
));
}
if !real_registry.contains("://") {
real_registry = format!("https://{real_registry}");
}
registry = Some(real_registry);

registry = Some(wasmer_registry::format_graphql(&real_registry));
namespace = n.to_string();
}

Expand All @@ -446,10 +487,12 @@ impl FromStr for SplitVersion {
};

let svp = sv.package.clone();
anyhow::ensure!(
!prohibited_package_names.any(|s| s == sv.package.trim()),
"Invalid package name {svp:?}"
);
if prohibited_package_names.any(|s| s == sv.package.trim()) {
return Err(SplitVersionError::InvalidCommandName(
anyhow::anyhow!("Invalid package name {svp:?}"),
None,
));
}

Ok(sv)
}
Expand Down
19 changes: 16 additions & 3 deletions lib/cli/src/commands/run.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use crate::cli::SplitVersion;
use crate::cli::{SplitVersion, SplitVersionError};
use crate::common::get_cache_dir;
#[cfg(feature = "debug")]
use crate::logging;
Expand Down Expand Up @@ -859,9 +859,22 @@ pub(crate) fn try_run_package_or_file(
let package = format!("{}", r.path.display());

let mut is_fake_sv = false;
let mut sv = match SplitVersion::parse(&package) {
Copy link
Contributor

@theduke theduke Dec 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to follow this logic

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

let new_split_version = SplitVersion::parse(&package);
let mut sv = match new_split_version {
Ok(o) => o,
Err(_) => {
Err(SplitVersionError::InvalidCommandName(e, context)) => {
let mut e = Err(anyhow::anyhow!(
"Invalid command {package:?}, file {package:?} not found either"
)
.context(e));

if let Some(c) = context {
e = e.context(c);
}

return e.context(anyhow::anyhow!("{}", r.path.display()));
}
Err(SplitVersionError::Other(_)) => {
let mut fake_sv = SplitVersion {
original: package.to_string(),
registry: None,
Expand Down
28 changes: 28 additions & 0 deletions tests/integration/cli/tests/run.rs
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,34 @@ fn run_whoami_works() -> anyhow::Result<()> {
Ok(())
}

#[test]
fn run_wasi_works_non_existent() -> anyhow::Result<()> {
let output = Command::new(get_wasmer_path())
.arg("run")
.arg("does/not/exist")
fschutt marked this conversation as resolved.
Show resolved Hide resolved
.output()?;

let stderr = std::str::from_utf8(&output.stderr).unwrap();

let stderr_lines = stderr
.lines()
.map(|s| s.trim().to_string())
.collect::<Vec<_>>();

assert_eq!(
stderr_lines,
vec![
"error: does/not/exist".to_string(),
"│ 1: invalid registry or namespace \"does\"".to_string(),
"│ 2: expected a dot if using a URL shorthand (e.g. does.com)".to_string(),
"╰─▶ 3: Invalid command \"does/not/exist\", file \"does/not/exist\" not found either"
.to_string(),
]
);

Ok(())
}

#[test]
fn run_wasi_works() -> anyhow::Result<()> {
let output = Command::new(get_wasmer_path())
Expand Down