Skip to content

Use JSON for cargo metadata parsing#9373

Merged
hsbt merged 1 commit intomasterfrom
use-json
Mar 9, 2026
Merged

Use JSON for cargo metadata parsing#9373
hsbt merged 1 commit intomasterfrom
use-json

Conversation

@hsbt
Copy link
Member

@hsbt hsbt commented Mar 9, 2026

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 json at RubyGems because lib/rubygems/ext/cargo_builder.rb is only called at gem i.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings March 9, 2026 05:45
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

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_load with JSON.parse for cargo metadata output.
  • 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.

Comment on lines +230 to 233
# 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 }
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
Comment on lines +230 to +232
# cargo metadata output is specified as json
require "json"
metadata = JSON.parse(output)
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

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

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).

Copilot uses AI. Check for mistakes.
metadata = Gem::SafeYAML.safe_load(output)
# cargo metadata output is specified as json
require "json"
metadata = JSON.parse(output)
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

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

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).

Suggested change
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

Copilot uses AI. Check for mistakes.
@hsbt hsbt merged commit c8694f0 into master Mar 9, 2026
95 checks passed
@hsbt hsbt deleted the use-json branch March 9, 2026 06:33
hsbt added a commit that referenced this pull request Mar 10, 2026
Use JSON for cargo metadata parsing

(cherry picked from commit c8694f0)
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.

2 participants