Conversation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR switches Rust extension installation (Cargo metadata parsing) from YAML-based parsing to JSON parsing, aligning with the effort to reduce reliance on Psych/YAML internals within RubyGems.
Changes:
- Replace
Gem::SafeYAML.safe_loadwithJSON.parseforcargo metadataoutput. - Add a local
require "json"to support JSON parsing in the cargo builder.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| # cargo metadata output is specified as json | ||
| require "json" | ||
| metadata = JSON.parse(output) | ||
| package = metadata["packages"].find {|pkg| normalize_path(pkg["manifest_path"]) == manifest_path } |
There was a problem hiding this comment.
Open3.capture2e merges stdout+stderr, but cargo metadata writes JSON to stdout and can emit warnings/diagnostics to stderr even on success. With the new JSON.parse(output), any stderr content will make the string invalid JSON and break installs. Capture stdout separately (e.g., Open3.capture3 or capture2 with stderr captured independently) and parse only stdout; keep stderr for really_verbose/results output.
| # cargo metadata output is specified as json | ||
| require "json" | ||
| metadata = JSON.parse(output) |
There was a problem hiding this comment.
Now that cargo metadata is parsed as JSON, the earlier Gem.load_yaml in this method is unused and forces the YAML engine to be loaded during Rust extension installs. Please remove that call to avoid an unnecessary dependency/load cost (and to align with the goal of moving away from Psych).
| metadata = Gem::SafeYAML.safe_load(output) | ||
| # cargo metadata output is specified as json | ||
| require "json" | ||
| metadata = JSON.parse(output) |
There was a problem hiding this comment.
If JSON.parse fails (e.g., unexpected cargo output), it will currently raise JSON::ParserError and may surface as a noisy backtrace during gem install. Consider rescuing parse errors here and re-raising Gem::InstallError with a concise message (and include the raw output only when really_verbose or via results).
| metadata = JSON.parse(output) | |
| metadata = | |
| begin | |
| JSON.parse(output) | |
| rescue JSON::ParserError => error | |
| if Gem.configuration.really_verbose | |
| puts output | |
| else | |
| results << output | |
| end | |
| raise Gem::InstallError, "cargo metadata produced invalid JSON: #{error.message}" | |
| end |
Use JSON for cargo metadata parsing (cherry picked from commit c8694f0)
from #9352
I'm working to replace Psych to pure Ruby implementation now. It's hard to support JSON at
YAMLSerializer.It's no problem to load
jsonat RubyGems becauselib/rubygems/ext/cargo_builder.rbis only called atgem i.