Skip to content

fix: bump openjd-rs crate deps to 0.3.0/0.5.0/0.5.0 - #329

Merged
seant-aws merged 1 commit into
OpenJobDescription:mainlinefrom
seant-aws:bump-openjd-rs-crates
Aug 14, 2026
Merged

fix: bump openjd-rs crate deps to 0.3.0/0.5.0/0.5.0#329
seant-aws merged 1 commit into
OpenJobDescription:mainlinefrom
seant-aws:bump-openjd-rs-crates

Conversation

@seant-aws

Copy link
Copy Markdown
Contributor

Bump Rust crate dependencies to pick up wrap-action bug fixes:

  • openjd-expr 0.2.1 -> 0.3.0
  • openjd-model 0.4.0 -> 0.5.0 (resolves 0.5.1 via semver)
  • openjd-sessions 0.4.0 -> 0.5.0 (resolves 0.5.1 via semver)

Adapts range_expr.rs for expr 0.3.0 breaking changes:

  • IntRange fields are now private (use accessor methods)
  • RangeExpr::from_values returns Result

Updates 3 string-operation-counting test expectations to match expr 0.3.0's more granular operation budgeting.

Fixes:

What was the problem/requirement? (What/Why)

What was the solution? (How)

What is the impact of this change?

How was this change tested?

See DEVELOPMENT.md for information on running tests.

  • Have you run the unit tests?

Was this change documented?

  • Are relevant docstrings in the code base updated?

Is this a breaking change?

A breaking change is one that modifies a public contract in a way that is not backwards compatible. See the
Public Interfaces section
of the DEVELOPMENT.md for more information on the public contracts.

If so, then please describe the changes that users of this package must make to update their scripts, or Python applications.

Does this change impact security?

  • Does the change need to be threat modeled? For example, does it create or modify files/directories that must only be readable by the process owner?
    • If so, then please label this pull request with the "security" label. We'll work with you to analyze the threats.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Comment thread rust-bindings/Cargo.toml
openjd-expr = "0.2.1"
openjd-model = "0.4.0"
openjd-sessions = "0.4.0"
openjd-expr = "0.3.0"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

THIRD-PARTY-LICENSES.txt was not regenerated alongside this dependency bump, so the committed file is now stale relative to Cargo.lock.

The THIRD-PARTY-LICENSES check job in .github/workflows/rust_quality.yml:183 regenerates the file from Cargo.lock via cargo about and diffs it against the committed copy, so this will likely fail CI. The committed file still records the pre-bump versions:

2073:** ahash; version 0.8.12          (ahash was removed from Cargo.lock in this PR)
2525:** granit-parser; version 0.0.7   (now 1.0.1)
2529:** openjd-expr; version 0.2.1     (now 0.3.0)
2545:** serde-saphyr; version 0.0.29   (now 1.0.1)

Plus the many other version deltas in the Cargo.lock diff, and the crates newly pulled in / dropped (getrandom, r-efi, wasip2, which, version_check). Running scripts/check_third_party_licenses.sh --update should bring it back in sync.

Comment thread rust-bindings/src/expr/range_expr.rs Outdated
format!(
"IntRange(start={}, end={}, step={})",
self.inner.start, self.inner.end, self.inner.step
self.inner.start(), self.inner.end(), self.inner.step()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This line is likely not rustfmt-clean. self.inner.start(), self.inner.end(), self.inner.step() is 60 chars of args on one line inside a format! whose first arg already spans a line — with the field-access form it fit, but the added () calls push it past the width where rustfmt breaks each argument onto its own line:

        format!(
            "IntRange(start={}, end={}, step={})",
            self.inner.start(),
            self.inner.end(),
            self.inner.step()
        )

.github/workflows/rust_quality.yml:97 runs cargo fmt --check, so this would fail the quality gate. Worth running cargo fmt over the file. (Line 94, the __reduce__ tuple, may be affected too.)

# total = 6
assert result.operation_count == 6
# total = 16
assert result.operation_count == 16

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The 6 -> 16 jump here looks anomalous and worth verifying against the upstream change rather than just accepting the new number.

The other two updated expectations move by exactly +1 (join 4→5, zfill 6→7), consistent with "more granular budgeting" adding one op somewhere. But repr_sh on a 500-char string jumps by +10, while:

  • repr_sh(hello) (line 256) is unchanged at 2, so the base cost of repr_sh did not change.
  • re_search(a * 500, ...) (line 105) is unchanged at 6, so __mul__ on a 500-char string still costs 3 and a peer string function on the same 500-char input still costs 3.

So under the documented 1 + ceil(len/256) model this should still be 6, and nothing else in the file corroborates a 10x-ish per-chunk cost that only affects repr_sh at length 500 but not at length 5. That pattern is more consistent with an upstream shell-quoting path being charged per-character (or per some smaller chunk) than with an intentional budgeting change — which would matter for users, since repr_sh on large strings would now burn the operation budget ~5x faster than re_escape/zfill on the same input.

Also, the inline comments above all three changed asserts still show the old arithmetic and now contradict the asserted values:

  • line 110-111 computes 3 + 3 but asserts 16
  • line 276 says 1 call + 3 list iterations = 4 but asserts 5
  • line 283-284 computes 3 + 3 but asserts 7

Please update the comments to explain the new arithmetic (they are the only documentation of the counting model in this file), and confirm the 16 is intended upstream behavior.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

hold on. updating the comment, but it seems like upstream openjdrs changed the way this is counted.

Comment thread rust-bindings/Cargo.toml
@@ -12,9 +12,9 @@ name = "_openjd_rs"
crate-type = ["cdylib", "rlib"]

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The stated goal of this PR may not actually be achieved by the committed lockfile.

The commit message says:

openjd-model 0.4.0 -> 0.5.0 (resolves 0.5.1 via semver)
openjd-sessions 0.4.0 -> 0.5.0 (resolves 0.5.1 via semver)

But Cargo.lock pins 0.5.0 for both, not 0.5.1:

Cargo.lock:698  name = "openjd-model"     version = "0.5.0"
Cargo.lock:730  name = "openjd-sessions"  version = "0.5.0"

Since Cargo.lock is committed and CI builds use it, the "wrap-action bug fixes" this PR is meant to pick up will not be in the built artifact if they landed in 0.5.1. A cargo update -p openjd-model -p openjd-sessions (or bumping the manifest requirement to "0.5.1" to make the floor explicit) would be needed.

Worth double-checking which patch version actually contains the wrap-action fix, and whether the lockfile reflects it.

@seant-aws
seant-aws force-pushed the bump-openjd-rs-crates branch from 52e02b6 to c12c05a Compare August 14, 2026 21:33
Comment thread TEST_PLAN_BUMP.md Outdated
@seant-aws
seant-aws force-pushed the bump-openjd-rs-crates branch 4 times, most recently from 3338eb0 to 99aab32 Compare August 14, 2026 21:46
Bump Rust crate dependencies to pick up wrap-action bug fixes:
- openjd-expr 0.2.1 -> 0.3.0
- openjd-model 0.4.0 -> 0.5.0 (resolves 0.5.1 via semver)
- openjd-sessions 0.4.0 -> 0.5.0 (resolves 0.5.1 via semver)

Adapts range_expr.rs for expr 0.3.0 breaking changes:
- IntRange fields are now private (use accessor methods)
- RangeExpr::from_values returns Result

Updates 3 string-operation-counting test expectations to match
expr 0.3.0's more granular operation budgeting.

Signed-off-by: Sean Tang <171081544+seant-aws@users.noreply.github.com>
@seant-aws
seant-aws force-pushed the bump-openjd-rs-crates branch from 99aab32 to 4d620c7 Compare August 14, 2026 22:08
@seant-aws
seant-aws marked this pull request as ready for review August 14, 2026 22:11
@seant-aws
seant-aws requested a review from a team as a code owner August 14, 2026 22:11
@seant-aws
seant-aws enabled auto-merge (squash) August 14, 2026 22:22
@seant-aws
seant-aws merged commit 60dc6a6 into OpenJobDescription:mainline Aug 14, 2026
31 checks passed
# total = 6
assert result.operation_count == 6
# zfill: charges on the output length (500)
# 1 + ceil(500/256)=2 + 1 dispatch = 4

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The updated comments document a counting model that the rest of this file contradicts, and the module docstring was not updated alongside them.

I checked the repr_sh derivation and it is self-consistent — escaped_bound(5) = 5*6+2 = 32, 1 + ceil(32/256) = 2, matching the unchanged repr_sh('hello') == 2 at line 258. So the 16 looks right. The issues are the surrounding documentation and the other two expectations.

1. The module docstring (lines 3-8) still states the old universal rule:

When a function processes a string or path value, ceil(len/256) is added
to the operation count.

That is now false for repr_sh (charges ceil(escaped_bound(len)/256), ~6x the input length) and for zfill (charges on the output length plus an extra term). Since this docstring is the only statement of the counting contract in the file, it should describe the actual rules — or at least note that some functions charge a worst-case-output bound rather than input length.

2. zfill's + 1 dispatch term (line 286) is inconsistent with peer functions.

No other string method here is charged a dispatch op: ('abc' * 100).replace('a','x') is 3 + 3 = 6 (line 64), ('a' * 1000).upper() is 5 + 5 = 10 (line 56), and the parametrized cases at lines 236-266 are all 1 + ceil(len/256) with no extra term. If zfill genuinely costs one more op than replace for comparable work, that is either an upstream inconsistency worth reporting to openjd-rs, or the + 1 accounts for something else (e.g. the padding write charged separately) and the comment mislabels it. As written it reads as a number fitted to the observation rather than a derivation.

3. join's expectation is now purely observational (line 277: Observed total from openjd-rs 0.3.0 = 5). That makes the assertion a change-detector: it will flag future upstream drift but gives a maintainer no basis to judge whether a new number is a bug or intended. Deriving the 5 explicitly (1 call + 3 list iterations + 1 for the 5-char result?) would keep it a conformance check.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants