Conversation
Signed-off-by: Erik Sundell <erik.sundell@sensmetry.com>
Why not use something like Cargo publish protocol?
What about license? Should require it to be an SPDX expression. |
sysand/src/cli.rs
Outdated
| /// configured default index URL, otherwise the first configured | ||
| /// index URL, or https://beta.sysand.org | ||
| #[arg(long, verbatim_doc_comment)] | ||
| index: Option<String>, |
There was a problem hiding this comment.
| index: Option<String>, | |
| index: Option<Url>, |
sysand/src/cli.rs
Outdated
| #[arg(long, short, default_value_t = false, verbatim_doc_comment)] | ||
| allow_path_usage: bool, | ||
| }, | ||
| /// Publish a KPAR to the sysand package index |
There was a problem hiding this comment.
| /// Publish a KPAR to the sysand package index | |
| /// Publish a KPAR to a sysand package index |
Any index, not necessarily "the" index (beta.sysand.org)
sysand/src/lib.rs
Outdated
| let basic_or_bearer_auth_policy = Arc::new(auths_builder.build()?); | ||
| let bearer_only_auth_policy = Arc::new(publish_auths_builder.build()?); |
There was a problem hiding this comment.
Let's avoid doing additional work for every command. Publish is very rare. Better add a conversion method from the former to the latter, and use that for the publish command.
Alaternatively, why not be even simpler: pass the PURL in some header (e.g. |
core/src/commands/publish.rs
Outdated
| MissingPublisher, | ||
|
|
||
| #[error( | ||
| "publisher field `{0}` is invalid for modern project IDs: must be 3-50 characters, use only letters and numbers, may include single spaces or hyphens between words, and must start and end with a letter or number" |
There was a problem hiding this comment.
| "publisher field `{0}` is invalid for modern project IDs: must be 3-50 characters, use only letters and numbers, may include single spaces or hyphens between words, and must start and end with a letter or number" | |
| "publisher field `{0}` is invalid for modern project IDs: must be 3-50 characters, use only ASCII letters and numbers, may include single spaces or hyphens between words, and must start and end with a letter or number" |
core/src/commands/publish.rs
Outdated
| pub is_new_project: bool, | ||
| } | ||
|
|
||
| fn build_upload_url(index_url: &str) -> Result<Url, PublishError> { |
There was a problem hiding this comment.
Take a parsed URL.
| url: index_url.into(), | ||
| reason: "URL cannot be used as a hierarchical base URL".to_string(), | ||
| })?; | ||
| segments.pop_if_empty(); |
There was a problem hiding this comment.
Require the URL to be HTTP(S), then unwrap() can be used here, and the errors will be better.
core/src/commands/publish.rs
Outdated
| // Keep publish endpoint path stable even if user provided query/fragment in base URL. | ||
| upload_url.set_query(None); | ||
| upload_url.set_fragment(None); |
There was a problem hiding this comment.
IMO it would be better to error out if query/fragment is present, instead of silently dropping them.
There was a problem hiding this comment.
Or log::warn
core/src/commands/publish.rs
Outdated
| let version = &info.version; | ||
| if !is_canonicalizable_publisher_field_value(publisher) { | ||
| return Err(PublishError::NonCanonicalizablePublisher( | ||
| publisher.to_owned().into_boxed_str(), |
There was a problem hiding this comment.
| publisher.to_owned().into_boxed_str(), | |
| publisher.as_str().into_boxed_str(), |
core/src/commands/publish.rs
Outdated
| } | ||
| if !is_canonicalizable_name_field_value(name) { | ||
| return Err(PublishError::NonCanonicalizableName( | ||
| name.to_owned().into_boxed_str(), |
There was a problem hiding this comment.
| name.to_owned().into_boxed_str(), | |
| name.as_str().into_boxed_str(), |
core/src/commands/publish.rs
Outdated
| )); | ||
| } | ||
| semver::Version::parse(version).map_err(|source| PublishError::InvalidVersion { | ||
| version: version.to_owned().into_boxed_str(), |
There was a problem hiding this comment.
| version: version.to_owned().into_boxed_str(), | |
| version: version.as_str().into_boxed_str(), |
| let request_builder = move |c: &reqwest_middleware::ClientWithMiddleware| { | ||
| let file_part = reqwest::multipart::Part::stream(file_bytes.clone()) | ||
| .file_name(file_name.clone()) | ||
| .mime_str("application/octet-stream") |
There was a problem hiding this comment.
Why not application/zip?
core/src/commands/publish.rs
Outdated
| let file_part = reqwest::multipart::Part::stream(file_bytes.clone()) | ||
| .file_name(file_name.clone()) | ||
| .mime_str("application/octet-stream") | ||
| .expect("valid mime type"); |
There was a problem hiding this comment.
| .expect("valid mime type"); | |
| .unwrap(); |
core/src/commands/publish.rs
Outdated
| .text("purl", purl.clone()) | ||
| .part("file", file_part); | ||
|
|
||
| c.post(upload_url.as_str()).multipart(form) |
There was a problem hiding this comment.
| c.post(upload_url.as_str()).multipart(form) | |
| c.post(upload_url).multipart(form) |
There was a problem hiding this comment.
Oh, this won't work. Then clone the URL to avoid very expensive reparse.
core/src/commands/publish.rs
Outdated
| pub fn do_publish_kpar<P: AsRef<Utf8Path>>( | ||
| kpar_path: P, | ||
| index_url: &str, | ||
| auth_policy: Arc<StandardHTTPAuthentication>, |
There was a problem hiding this comment.
We always use bearer auth and no registry will allow unauthenticated publishing, so use ForceBearerAuth.
core/src/commands/publish.rs
Outdated
|
|
||
| pub fn do_publish_kpar<P: AsRef<Utf8Path>>( | ||
| kpar_path: P, | ||
| index_url: &str, |
There was a problem hiding this comment.
Take a URL.
| let is_separator = b == b'-' || b == b' ' || (allow_dot && b == b'.'); | ||
| if !is_separator { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
This allows non-ASCII to pass through.
| })?; | ||
|
|
||
| let status = response.status().as_u16(); | ||
| let body = runtime.block_on(response.text()).unwrap_or_default(); |
There was a problem hiding this comment.
Don't eat errors.
|
|
||
| #[test] | ||
| fn publisher_field_canonicalizability() { | ||
| assert!(is_canonicalizable_publisher_field_value("Acme Labs")); |
There was a problem hiding this comment.
Add a case for dot in the middle.
There was a problem hiding this comment.
Also a case for - (space and punctuation), and one or more cases of non-ASCII.
| @@ -0,0 +1,77 @@ | |||
| # `sysand publish` | |||
|
|
|||
| Publish a KPAR to the sysand package index. | |||
There was a problem hiding this comment.
| Publish a KPAR to the sysand package index. | |
| Publish a KPAR to a sysand package index. |
|
|
||
| - `version`: must be a valid Semantic Versioning 2.0 version. | ||
|
|
||
| - `publisher`: 3-50 characters, letters and numbers only, with optional single |
There was a problem hiding this comment.
| - `publisher`: 3-50 characters, letters and numbers only, with optional single | |
| - `publisher`: 3-50 characters, ASCII letters and numbers only, with optional single |
| - `publisher`: 3-50 characters, letters and numbers only, with optional single | ||
| spaces or hyphens between words, and must start and end with a letter or | ||
| number. | ||
| - `name`: 3-50 characters, letters and numbers only, with optional single |
There was a problem hiding this comment.
| - `name`: 3-50 characters, letters and numbers only, with optional single | |
| - `name`: 3-50 characters, ASCII letters and numbers only, with optional single |
| ## Options | ||
|
|
||
| - `--index <URL>`: URL of the package index to publish to. Defaults to the | ||
| configured default index URL, otherwise the first configured index URL, or |
There was a problem hiding this comment.
Link to index config
| if let Some(index_url) = config | ||
| .indexes | ||
| .iter() | ||
| .find(|index| index.default.unwrap_or(false)) | ||
| .map(|index| index.url.as_str()) | ||
| .or_else(|| config.indexes.first().map(|index| index.url.as_str())) | ||
| { | ||
| Url::parse(index_url).map_err(|e| anyhow!("invalid index URL in configuration: {e}")) | ||
| } else { | ||
| Ok(Url::parse(DEFAULT_INDEX_URL).expect("default publish index URL must be valid")) | ||
| } |
There was a problem hiding this comment.
To avoid all this parsing, use Url in config struct. If this requires many changes, just add a TODO there.
| ) -> Result<()> { | ||
| let kpar_path = resolve_publish_kpar_path(path, ctx)?; | ||
| if !kpar_path.is_file() { | ||
| bail!("kpar file not found at `{kpar_path}`, run `sysand build` first"); |
There was a problem hiding this comment.
| bail!("kpar file not found at `{kpar_path}`, run `sysand build` first"); | |
| bail!("KPAR file not found at `{kpar_path}`, run `sysand build` first"); |
| runtime: Arc<tokio::runtime::Runtime>, | ||
| ) -> Result<()> { | ||
| let kpar_path = resolve_publish_kpar_path(path, ctx)?; | ||
| if !kpar_path.is_file() { |
There was a problem hiding this comment.
wrapfs::is_file
| { | ||
| Url::parse(index_url).map_err(|e| anyhow!("invalid index URL in configuration: {e}")) | ||
| } else { | ||
| Ok(Url::parse(DEFAULT_INDEX_URL).expect("default publish index URL must be valid")) |
There was a problem hiding this comment.
| Ok(Url::parse(DEFAULT_INDEX_URL).expect("default publish index URL must be valid")) | |
| Ok(Url::parse(DEFAULT_INDEX_URL).unwrap()) |
We know it's valid.
| let config = Config::default(); | ||
| let url = resolve_publish_index_url(None, &config).unwrap(); | ||
|
|
||
| assert_eq!(url.as_str(), format!("{DEFAULT_INDEX_URL}/")); |
There was a problem hiding this comment.
use concat!:
| assert_eq!(url.as_str(), format!("{DEFAULT_INDEX_URL}/")); | |
| assert_eq!(url.as_str(), concat!(DEFAULT_INDEX_URL, '/')); |
| ) | ||
| .match_body(Matcher::AllOf(vec![ | ||
| Matcher::Regex(r#"name="purl""#.to_string()), | ||
| Matcher::Regex("pkg:sysand/acme-labs/my\\.project-alpha@1\\.0\\.0".to_string()), |
There was a problem hiding this comment.
Use raw strings to avoid \\ and similar.
Signed-off-by: Erik Sundell <erik.sundell+2025@sensmetry.com>
Summary
Adds a new
sysand publishcommand to upload.kparpackages to a Sysand-compatible index, including CLI wiring, core publish logic, docs, and test coverage.What’s included
New CLI command:
sysand publishsysand publish [OPTIONS] [PATH]PATHis omitted, resolves to default build output for the current project.--index <URL>to explicitly select publish target index.Publish target resolution
When
--indexis not provided, publish target is resolved in this order:default = truehttps://beta.sysand.orgCore publish flow
.kpar:publisherpresent and canonicalizablenamecanonicalizableversionvalid SemVer 2.0pkg:sysand/<publisher>/<name>@<version>):-namepreservedpurl+file) to/api/v1/upload.publish).basic-or-bearerfor general commands andbearer-onlyfor publish.Docs and CLI help
sysand publishcommand docs.Tests
sysand/tests/cli_publish.rs:--indexpublisher,name,version)