Modernize childprocess for the current Ruby ecosystem - #200
Open
dior001 wants to merge 1 commit into
Open
Conversation
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
NecroRuby has revived
childprocessNecroRuby is a bot that brings quality open-source Ruby libraries
up-to-date with the modern Ruby ecosystem — upgrading dependencies,
restoring test coverage, tightening security, and improving documentation
for gems whose last release is over a year old.
NecroRuby is a fully autonomous process and is capable of mistakes. If you
disagree with any of these changes, just say so on this PR (or close it) and
NecroRuby will move on. If you have questions, ask here — NecroRuby monitors
this PR and will respond.
Modernized and tested on Ruby 4.0.6, the latest Ruby release.
ChildProcess Modernization Report
This branch modernizes
childprocessfor Ruby 4.0.6 (and current maintainedRubies generally). The gem's core design was already in good shape -- v5.0
(2024) had already replaced the old
ffi/posix_spawnbackends with asingle
Process.spawn-based implementation, so there was no C-extension orplatform-fork logic to port. The work here is concentrated on dependency
health, test rigor, lint/documentation hygiene, and CI.
TL;DR
bundle installwas broken on a fresh checkout before this branch(the
coverallsdev dependency doesn't resolve). Fixed by replacing itwith
simplecov.and fixed along the way (see below).
required_ruby_versionraised from>= 2.4.0to>= 3.2;.ruby-versionpinned to
4.0.6; CI matrix now covers Ruby 3.2, 3.3, 3.4, 4.0,head,JRuby and TruffleRuby.
rubocop-performance,rubocop-rspec) added, whole codebasepasses with zero offenses.
bundler-auditadded as a CI job; zero vulnerabilities found in thecurrent dependency set.
passed cleanly (with
-w, no deprecation warnings) as soon as thedependency chain was fixed.
Dependency changes
coveralls< 1.0(dev)bundle installfailed outright on a clean checkout.simplecov~> 0.22(dev, new)rspec~> 3.0(dev)~> 3.13(dev)yard~> 0.0(dev)~> 0.9(dev)~> 0.0constraint was effectively unconstrained/stale; pinned to the current stable line.rakeGemfile~> 13.0(dev, gemspec)gem 'rake'in the Gemfile, sogem build/CI get a consistent, pinned version.rubocop~> 1.88(dev, new)rubocop-performance~> 1.26(dev, new)rubocop-rspec~> 3.10(dev, new)bundler-audit~> 0.9(dev, new)logger~> 1.5(runtime)~> 1.5(runtime, unchanged)The
Gemfile's Ruby-2.4-onlyterm-ansicolorpin (a transitive workaroundfor old Coveralls versions) was removed along with
coverallsitself --it's dead weight now that the floor is Ruby 3.2.
No runtime dependency other than
loggerexists; the gem's onlynon-stdlib runtime footprint remains minimal by design.
Security
bundle exec bundler-audit check --updatereports no vulnerabilitiesagainst the current
ruby-advisory-dbfor the resolved dependency set.Added as a dedicated
auditjob in CI so this is checked on every push.coverallsdependency chain itself (a supply-chain health issue more than a CVE):
a dev dependency that can no longer be installed is a liability for
contributors and CI alike. Resolved by removing it (see Dependency
changes above).
Security/Eval(RuboCop) flags severaleval(...)calls inspec/childprocess_spec.rb. These deserializeHash#inspect/Array#inspectoutput that a child process -- spawned by the very samespec, from a tempfile the spec itself wrote -- reports its
ENV/ARGVas. It's not attacker-controlled input, so the cop is excluded for that
one file with a comment explaining why, rather than papering over it or
rewriting a working test technique for cosmetic reasons.
Compatibility fixes for Ruby 4.0.6
Short version: none were needed in the library code. The full suite
ran clean on 4.0.6, including under
-w(no deprecation warnings), assoon as
bundle installcould complete (i.e. oncecoverallswasreplaced). This is because:
ffi-based Windows backend and theposix_spawn-via-C-extension path in v5.0, in favor of a pureProcess.spawn/Process.waitpid2/Process.killimplementation. Therewas no native extension or removed-stdlib surface to port.
ChildProcess.posix_spawn=/.posix_spawn?remain as inert, backward-compatible no-op API surface (some consumers, e.g. via the README's
JRuby caveat, still call
ChildProcess.posix_spawn = true).loggerwas already added as an explicit runtime dependency in aprior PR for the Ruby 3.4 "moved to a default gem" change, and
ostructwas already removed. Both remain correct for 4.0.6.Declared explicitly for 4.0.6:
.ruby-versionadded, pinned to4.0.6.childprocess.gemspec:required_ruby_versionraised from>= 2.4.0to
>= 3.2-- a floor the gem's dev tooling (simplecov1.x,rubocop-rspec3.x) already assumes, and consistent with actuallytesting/supporting only non-EOL-or-recent Rubies going forward.
.github/workflows/ci.yml): dropped Ruby 2.4-3.1 (now belowthe floor / EOL), added
4.0, kept3.2/3.3/3.4/head/jruby/truffleruby. Also bumpedactions/checkouttov4(v2 is EOL) andadded two new jobs,
lint(RuboCop) andaudit(bundler-audit).Test coverage: 86.9% → 100%
The very first successful coverage run (once
simplecovreplaced thebroken
coveralls) measured 86.92% line coverage (226/260). Gettingto 100% (280/280 -- the line count grew as documentation and small
refactors were added) involved:
spec/abstract_process_spec.rb,spec/process_spawn_process_spec.rb, and a substantial expansion ofspec/windows_spec.rb(previously entirely skipped outside of WindowsCI runners -- it's now exercised on every platform, since
Windows::Process/Windows::IOare plain Ruby with::Process.spawnstubbed out, not OS-specific code).
spec/childprocess_spec.rbto cover.platform,.platform_name,.linux?,.jruby?,.unix?/.windows?, theposix_spawn*family,.close_on_exec's error path,.new/.build'sper-OS dispatch and its unsupported-platform error, and every branch of
.os's host-OS detection (macOS, Windows, Cygwin, Solaris, AIX, and the"unknown OS" error).
ChildProcess.warn_once(private,never called anywhere in the codebase) rather than writing a test to
cover unreachable code.
if ChildProcess.windows? / elserequiredispatch in
lib/childprocess.rb-- is marked with SimpleCov's# :nocov:. Exactly one branch of that dispatch can ever run in a givenOS/process, symmetrically on every CI platform (on Windows CI, the
unixbranch would be the "uncovered" one instead), so no single testrun can ever exercise both sides of it. This is the one deliberate,
documented coverage exclusion in the suite.
A real bug caught along the way
spec/platform_detection_spec.rb'sexpected_arch_for_host_cpusharedexample had a pre-existing bug: it stubbed
RbConfig::CONFIG['host_cpu']to return
expected_archinstead of the actualhost_cpuparameter. Forthe macOS/i686-64-bit-workaround test in particular, this meant the stub
fed back
"x86_64"as the host_cpu value, which matches a differentcase/whenbranch than the one the test was supposedly exercising -- sothe test passed, but never actually ran the code path
(
workaround_older_macosx_misreported_cpu?) it claimed to. Fixed bystubbing the correct value; this is what let
lib/childprocess.rb's64-bit-Darwin-misreported-as-i686 workaround get real coverage instead of
a false-positive pass.
A self-caught regression
While bringing the codebase up to RuboCop's default style (
rubocop -A),one autocorrect changed
pid = nil if pid == 0topid = nil if pid.zero?inProcessSpawnProcess#exited?.Process.waitpid2(..., Process::WNOHANG)returns barenil(not[nil, nil]) while the childis still running, which is the common case for
#exited?/#alive?on alive process -- so
pid.zero?crashed withNoMethodErroron every callmade while a process was still alive. The full test suite (116 examples)
still passed after that autocorrect, only because the run confirming
100% coverage predated it. Re-running the suite after the RuboCop pass
caught this immediately. Fixed with
pid&.zero?, which is bothcrash-safe and satisfies the same style cop.
This wasn't shipped -- it's called out here as a concrete illustration of
why "lint autocorrect + rerun the full suite before considering it done"
matters, not as an outstanding issue.
Lint (RuboCop)
.rubocop.ymladded:rubocop-performance+rubocop-rspecplugins,NewCops: enable,TargetRubyVersion: 3.2.offenses. Most of the ~580 initial offenses were mechanical (string
quoting,
frozen_string_literal, hash syntax, redundantself,guard clauses, etc.) and safely autocorrected (after the regression
above was caught and fixed by hand).
lib/childprocess/process_spawn_process.rb#launch_processwasgenuinely too complex (flagged by four separate RuboCop metrics --
ABC size, cyclomatic complexity, perceived complexity, method length).
Refactored into
#launch_process(orchestration),#base_spawn_options,#sanitized_environment, and#spawn_args,each with a single, documented responsibility. Behavior is unchanged
(verified by the full suite before/after).
inline rationale comment in
.rubocop.ymlor the source: devdependencies staying in the gemspec rather than moving to the Gemfile;
has_fileno?/has_to_io?/set_exit_codekeeping their existing,clearer names over the cops' preferred alternatives;
before(:all)/RSpec/InstanceVariablefor the one spec that deliberately saves/restores global
RbConfigstate once per describe block; thealready-discussed
Security/Evalexclusion; and the spec file namingconvention (
childprocess_spec.rb, matching the gem/module name)over the cop's ActiveSupport-style
child_process_spec.rbinflection.Documentation
bundle exec yard stats): all 3modules, 12 classes, 2 constants, 12 attributes, and 31 methods across
the library now have doc comments, including
@param/@return/@raisetags where relevant and a top-level usage example on theChildProcessmodule itself.ChildProcess.posix_spawn=had a doc comment ("Set this to true to enable experimental use of
posix_spawn.") that, after an earlier RuboCop autocorrect merged it
into an
attr_writerdeclaration, ended up floating above theunrelated
#osmethod instead. Re-attached to the right place.README
actual CI matrix.
it anymore) and added a short note on how coverage is measured now.
(
rake spec,rubocop,yard doc,bundler-audit).to a
devbranch and Travis CI links that no longer exist for thisrepo (it's
master-only with GitHub Actions); updated to match reality.Everything else
.gitignore:.ruby-versionwas previously ignored, which would havesilently dropped the new pin from any commit; removed that line. Added
.yardoc/doc(generated YARD artifacts).gem build childprocess.gemspecsucceeds andgemspec.validateemitsno warnings (covered by the existing "validates cleanly" spec, now
using
Gem::Specification.loadinstead of a hand-rolledevalof thegemspec file).
Not done / out of scope
Nothing was blocking. The one thing intentionally left as-is: the
Code Climatebadge in the README wasn't touched, since it's unrelatedto any dependency/tooling change made here and its status is unknown/
unverifiable from this environment (no credentials to check it).
🤖 Opened automatically by NecroRuby, an UpWoof.ai service.