-
Notifications
You must be signed in to change notification settings - Fork 11.4k
Guard npm update readiness #19389
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
Merged
Merged
Guard npm update readiness #19389
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -651,11 +651,59 @@ jobs: | |
| prefix="${NPM_TAG}-" | ||
| fi | ||
|
|
||
| root_tarball="dist/npm/codex-npm-${VERSION}.tgz" | ||
| sdk_tarball="dist/npm/codex-sdk-npm-${VERSION}.tgz" | ||
| # Keep this list in sync with CODEX_PLATFORM_PACKAGES in | ||
| # codex-cli/scripts/build_npm_package.py. The root wrapper advances | ||
| # @openai/codex@latest as soon as it publishes, so every platform | ||
| # package it aliases must already exist in the registry first. | ||
| platform_tarballs=( | ||
| "dist/npm/codex-npm-linux-x64-${VERSION}.tgz" | ||
| "dist/npm/codex-npm-linux-arm64-${VERSION}.tgz" | ||
| "dist/npm/codex-npm-darwin-x64-${VERSION}.tgz" | ||
| "dist/npm/codex-npm-darwin-arm64-${VERSION}.tgz" | ||
| "dist/npm/codex-npm-win32-x64-${VERSION}.tgz" | ||
| "dist/npm/codex-npm-win32-arm64-${VERSION}.tgz" | ||
| ) | ||
|
|
||
| for required_tarball in "${platform_tarballs[@]}" "${root_tarball}"; do | ||
| if [[ ! -f "${required_tarball}" ]]; then | ||
| echo "Missing npm tarball: ${required_tarball}" | ||
| exit 1 | ||
| fi | ||
| done | ||
|
|
||
| shopt -s nullglob | ||
| tarballs=(dist/npm/*-"${VERSION}".tgz) | ||
| if [[ ${#tarballs[@]} -eq 0 ]]; then | ||
| echo "No npm tarballs found in dist/npm for version ${VERSION}" | ||
| exit 1 | ||
| other_tarballs=() | ||
| for tarball in dist/npm/*-"${VERSION}".tgz; do | ||
| if [[ "${tarball}" == "${root_tarball}" || "${tarball}" == "${sdk_tarball}" ]]; then | ||
| continue | ||
| fi | ||
|
|
||
| is_platform_tarball=false | ||
| for platform_tarball in "${platform_tarballs[@]}"; do | ||
| if [[ "${tarball}" == "${platform_tarball}" ]]; then | ||
| is_platform_tarball=true | ||
| break | ||
| fi | ||
| done | ||
| if [[ "${is_platform_tarball}" == true ]]; then | ||
| continue | ||
| fi | ||
|
|
||
| other_tarballs+=("${tarball}") | ||
| done | ||
|
|
||
| # Publish the platform packages before the root CLI wrapper. The root | ||
| # wrapper advances @openai/codex@latest, so it should only publish | ||
| # after the optional dependency versions it references exist. | ||
| tarballs=( | ||
| "${platform_tarballs[@]}" | ||
| "${other_tarballs[@]}" | ||
| "${root_tarball}" | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Root tarball is what determines when npm says a version is ready. So even with the cross validation logic below against github latest, we can still run into user wants to upgrade yet it is actually no 100% ready. |
||
| ) | ||
| if [[ -f "${sdk_tarball}" ]]; then | ||
| tarballs+=("${sdk_tarball}") | ||
| fi | ||
|
|
||
| for tarball in "${tarballs[@]}"; do | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,130 @@ | ||
| use serde::Deserialize; | ||
| use std::collections::HashMap; | ||
|
|
||
| #[cfg(not(debug_assertions))] | ||
| pub(crate) const PACKAGE_URL: &str = "https://registry.npmjs.org/@openai%2fcodex"; | ||
|
|
||
| #[derive(Deserialize, Debug, Clone)] | ||
| pub(crate) struct NpmPackageInfo { | ||
| #[serde(rename = "dist-tags")] | ||
| dist_tags: HashMap<String, String>, | ||
| versions: HashMap<String, NpmPackageVersionInfo>, | ||
| } | ||
|
|
||
| #[derive(Deserialize, Debug, Clone)] | ||
| struct NpmPackageVersionInfo { | ||
| dist: Option<NpmPackageDist>, | ||
| } | ||
|
|
||
| #[derive(Deserialize, Debug, Clone)] | ||
| struct NpmPackageDist { | ||
| tarball: Option<String>, | ||
| integrity: Option<String>, | ||
| } | ||
|
|
||
| pub(crate) fn ensure_version_ready( | ||
| package_info: &NpmPackageInfo, | ||
| version: &str, | ||
| ) -> anyhow::Result<()> { | ||
| let version = version.trim(); | ||
|
|
||
| match package_info.dist_tags.get("latest").map(String::as_str) { | ||
| Some(latest) if latest == version => {} | ||
| Some(latest) => anyhow::bail!( | ||
| "npm latest dist-tag points to {latest}, expected GitHub release {version}" | ||
| ), | ||
| None => anyhow::bail!("npm package is missing latest dist-tag"), | ||
| } | ||
|
|
||
| version_info_with_dist(package_info, version)?; | ||
| Ok(()) | ||
| } | ||
|
|
||
| fn version_info_with_dist<'a>( | ||
| package_info: &'a NpmPackageInfo, | ||
| version: &str, | ||
| ) -> anyhow::Result<&'a NpmPackageVersionInfo> { | ||
| let info = package_info | ||
| .versions | ||
| .get(version) | ||
| .ok_or_else(|| anyhow::anyhow!("npm package version {version} is missing"))?; | ||
| let Some(dist) = info.dist.as_ref() else { | ||
| anyhow::bail!("npm package version {version} is missing dist metadata"); | ||
| }; | ||
| let has_tarball = dist | ||
| .tarball | ||
| .as_deref() | ||
| .is_some_and(|tarball| !tarball.is_empty()); | ||
| if !has_tarball { | ||
| anyhow::bail!("npm package version {version} is missing dist.tarball"); | ||
| } | ||
| let has_integrity = dist | ||
| .integrity | ||
| .as_ref() | ||
| .is_some_and(|integrity| !integrity.is_empty()); | ||
| if !has_integrity { | ||
| anyhow::bail!("npm package version {version} is missing dist.integrity"); | ||
| } | ||
| Ok(info) | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
|
|
||
| fn version_json(version: &str) -> serde_json::Value { | ||
| serde_json::json!({ | ||
| "dist": { | ||
| "integrity": format!("sha512-{version}"), | ||
| "tarball": format!("https://registry.npmjs.org/@openai/codex/-/codex-{version}.tgz"), | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| fn package_info(github_latest: &str, npm_latest: &str) -> NpmPackageInfo { | ||
| let mut versions = serde_json::Map::new(); | ||
| versions.insert(github_latest.to_string(), version_json(github_latest)); | ||
|
|
||
| serde_json::from_value(serde_json::json!({ | ||
| "dist-tags": { "latest": npm_latest }, | ||
| "versions": serde_json::Value::Object(versions), | ||
| })) | ||
| .expect("valid npm package metadata") | ||
| } | ||
|
|
||
| #[test] | ||
| fn ready_version_requires_latest_dist_tag_and_root_dist() { | ||
| let latest = "1.2.3"; | ||
| let package_info = package_info(latest, latest); | ||
|
|
||
| ensure_version_ready(&package_info, latest).expect("npm package is ready"); | ||
| } | ||
|
|
||
| #[test] | ||
| fn ready_version_rejects_stale_latest_dist_tag() { | ||
| let package_info = package_info("1.2.3", "1.2.2"); | ||
|
|
||
| let err = ensure_version_ready(&package_info, "1.2.3") | ||
| .expect_err("npm latest dist-tag must match GitHub latest"); | ||
| assert!( | ||
| err.to_string().contains("latest dist-tag"), | ||
| "error should name stale latest dist-tag: {err}" | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn ready_version_rejects_missing_root_dist() { | ||
| let package_info: NpmPackageInfo = serde_json::from_value(serde_json::json!({ | ||
| "dist-tags": { "latest": "1.2.3" }, | ||
| "versions": { "1.2.3": {} }, | ||
| })) | ||
| .expect("valid npm package metadata"); | ||
|
|
||
| let err = ensure_version_ready(&package_info, "1.2.3") | ||
| .expect_err("root package must have dist metadata"); | ||
| assert!( | ||
| err.to_string().contains("missing dist metadata"), | ||
| "error should name missing dist metadata: {err}" | ||
| ); | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,70 @@ | ||
| pub(crate) fn is_newer(latest: &str, current: &str) -> Option<bool> { | ||
| match (parse_version(latest), parse_version(current)) { | ||
| (Some(l), Some(c)) => Some(l > c), | ||
| _ => None, | ||
| } | ||
| } | ||
|
|
||
| pub(crate) fn extract_version_from_latest_tag(latest_tag_name: &str) -> anyhow::Result<String> { | ||
| latest_tag_name | ||
| .strip_prefix("rust-v") | ||
| .map(str::to_owned) | ||
| .ok_or_else(|| anyhow::anyhow!("Failed to parse latest tag name '{latest_tag_name}'")) | ||
| } | ||
|
|
||
| pub(crate) fn is_source_build_version(version: &str) -> bool { | ||
| parse_version(version) == Some((0, 0, 0)) | ||
| } | ||
|
|
||
| fn parse_version(v: &str) -> Option<(u64, u64, u64)> { | ||
| let mut iter = v.trim().split('.'); | ||
| let maj = iter.next()?.parse::<u64>().ok()?; | ||
| let min = iter.next()?.parse::<u64>().ok()?; | ||
| let pat = iter.next()?.parse::<u64>().ok()?; | ||
| Some((maj, min, pat)) | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
| use pretty_assertions::assert_eq; | ||
|
|
||
| #[test] | ||
| fn extracts_version_from_latest_tag() { | ||
| assert_eq!( | ||
| extract_version_from_latest_tag("rust-v1.5.0").expect("failed to parse version"), | ||
| "1.5.0" | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn latest_tag_without_prefix_is_invalid() { | ||
| assert!(extract_version_from_latest_tag("v1.5.0").is_err()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn prerelease_version_is_not_considered_newer() { | ||
| assert_eq!(is_newer("0.11.0-beta.1", "0.11.0"), None); | ||
| assert_eq!(is_newer("1.0.0-rc.1", "1.0.0"), None); | ||
| } | ||
|
|
||
| #[test] | ||
| fn plain_semver_comparisons_work() { | ||
| assert_eq!(is_newer("0.11.1", "0.11.0"), Some(true)); | ||
| assert_eq!(is_newer("0.11.0", "0.11.1"), Some(false)); | ||
| assert_eq!(is_newer("1.0.0", "0.9.9"), Some(true)); | ||
| assert_eq!(is_newer("0.9.9", "1.0.0"), Some(false)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn source_build_version_is_not_checked() { | ||
| assert!(is_source_build_version("0.0.0")); | ||
| assert!(!is_source_build_version("0.1.0")); | ||
| } | ||
|
|
||
| #[test] | ||
| fn whitespace_is_ignored() { | ||
| assert_eq!(parse_version(" 1.2.3 \n"), Some((1, 2, 3))); | ||
| assert_eq!(is_newer(" 1.2.3 ", "1.2.2"), Some(true)); | ||
| } | ||
| } |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So hardcoding the platform is a bit ugly but we can make sure that all tarballs are covered. Alternatively we can use a sort given that we didn't really care about existence prviously.