Git CPAN module support: fix local-tie + System::Command IPC::Open3 fallback#538
Merged
Git CPAN module support: fix local-tie + System::Command IPC::Open3 fallback#538
Conversation
`local $tied_scalar = value` previously replaced the global slot with a fresh untied GlobalRuntimeScalar, silently dropping the tie. This broke File::chdir (whose tied $CWD calls chdir in STORE) and therefore Git::Wrapper, which scopes every subcommand with `local $CWD = $self->dir`. Now, when `GlobalRuntimeScalar.dynamicSaveState()` sees a TIED_SCALAR in the slot, it keeps the tied scalar in place, dispatches STORE(undef) on entry (matching real Perl), and saves the current FETCH value. On scope exit, `dynamicRestoreState()` dispatches STORE with the saved value instead of swapping slots. Non-tied path is unchanged. - Extended src/test/resources/unit/tie_scalar.t with explicit assertions that STORE fires on localized entry, on assignments inside the scope, and on scope exit. - `./jcpan -t Git::Wrapper` now passes 75/75 (was 52/57). - `make` stays green. Plan: dev/modules/git_modules_support.md Generated with [Devin](https://cli.devin.ai/docs) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
System::Command is the subprocess-spawning core that Git::Repository
and Git::Wrapper's CPAN ecosystem leans on. Its `_spawn` closure uses a
hand-rolled pipe+fork+exec that silently dies on PerlOnJava (no fork on
the JVM), so every Git::Repository test skipped with
`fork() not supported on this platform (Java/JVM)`.
This commit bundles a patched System::Command (and the unchanged
Reaper) under src/main/perl/lib/System/ that detects PerlOnJava via
$Config{perlonjava} and spawns the child through IPC::Open3 (already
implemented on top of java.lang.ProcessBuilder). The existing cwd/env
handling in System::Command::new() is unchanged — open3 inherits the
parent's cwd and %ENV naturally.
Results:
- ./jcpan -t System::Command: 132/140 (94%). Remaining 8 are an
unrelated $ENV{SHLVL} mismatch (open3 wraps some argv shapes in a
shell).
- ./jcpan -t Git::Repository: 304/328 (93%). Previously almost fully
skipped.
Caveat: ~/.perlonjava/lib has precedence over the JAR-bundled lib, so
users who already installed System::Command from CPAN need to
`rm ~/.perlonjava/lib/System/Command.pm` once to pick up the bundled
patch. New installs Just Work. See dev/modules/git_modules_support.md
for follow-up on install-time patching.
Plan: dev/modules/git_modules_support.md
Generated with [Devin](https://cli.devin.ai/docs)
Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Add the cleanly-passing subset of System::Command's t/ suite to the `testModule` harness so that the bundled patch in `src/main/perl/lib/System/Command.pm` is regression-covered by `make test-bundled-modules`: - t/00-compile.t (requires $^X) - t/01-load.t (load both .pm files) - t/21-loop_on.t (loop_on with stdout/stderr callbacks; uses t/lines.pl) - t/25-refopts.t (ref-based exit/signal/core out params) - t/90-output.t (STDOUT/STDERR merging via options) Tests that need deeper fork semantics (10-command SHLVL mismatch, 15-scope zombie-reaping timing, 30-exit status quirks) are left out for now — they're tracked in dev/modules/git_modules_support.md. The testModule gradle task now sets PERLONJAVA_EXECUTABLE to the project's `jperl` launcher so tests that spawn a child perl via $^X (most of System::Command's suite) can locate a working interpreter. Without this, $^X defaults to the bare string "jperl" which isn't on $PATH under Gradle's test harness. Generated with [Devin](https://cli.devin.ai/docs) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
`GlobalDestruction.runGlobalDestruction()` iterated directly over
`GlobalVariable.{globalVariables,globalArrays,globalHashes}` while
calling `DESTROY` on every tracked blessed reference. A DESTROY
callback that touches any global (opens a handle, creates a tied
variable, registers END-like cleanup, etc.) would mutate the backing
HashMap mid-iteration and raise ConcurrentModificationException —
aborting the program with exit 255 instead of the intended exit code.
Real-world trigger, discovered while scoping System::Command 100%:
jperl -MSystem::Command -e 'my $x = System::Command->new(...);
exit 3'
On exit(3), Reaper's DESTROY ran on the still-live command, touching
globals → CME → process exited 255 instead of 3 (t/30-exit.t).
Fix: snapshot each collection (and each array's/hash's contents) with
`new ArrayList<>(...)` before iterating, so DESTROY callbacks are free
to mutate the originals.
Generated with [Devin](https://cli.devin.ai/docs)
Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
IPC::Open3 always wrapped single-element @cmd in `/bin/sh -c`, which swallowed exec failures (sh returns 127, but the caller never learns the exec never happened). Real Perl's IPC::Open3 only invokes the shell when the command contains shell metacharacters — otherwise it `exec`'s directly so the parent can observe exec-failure errors via its stat-pipe. Match that behaviour: - Add a shell-metacharacter sniff in IPCOpen3.java. Bare executable names now go direct via ProcessBuilder. - On IOException from ProcessBuilder.start(), translate the JDK's "Cannot run program X (in directory Y): error=2, No such file or directory" into Perl-shaped "open3: exec of X failed: No such file or directory" and set $!. - Don't double-wrap: the outer catch was prepending a second "open3: " prefix. In the bundled System/Command.pm, translate that "open3: exec of X failed: Y" into System::Command's own fork-path error phrasing "Can't exec( @cmd ): Y", so the eval-and-croak pattern its tests expect continues to work. System::Command t/11-spawn-fail.t now passes 2/2. Generated with [Devin](https://cli.devin.ai/docs) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
POSIX specifies that a `SIG_IGN` disposition for SIGCHLD makes the
kernel auto-reap exited children, so subsequent waitpid() returns -1
with errno=ECHILD (perlipc documents the same for Perl). PerlOnJava
doesn't model signal dispositions, so waitpid() always returned the
real exit status and System::Command's `_reap` never took its
"zombie ate the status" branch.
Add a lightweight check: if `%SIG{CHLD}` is set to the string
'IGNORE' at waitpid() time, and we're looking at a Java-registered
child process, remove the process from our table, leave `$?` alone,
and return -1. That's enough for Reaper to observe $zed=false and
publish (-1, -1, -1) as the triple exit/signal/core.
System::Command t/20-zombie.t now passes 31/32 (was 27/30); the
remaining failure is the "process should be dead" scope-DESTROY
assertion tracked separately.
Generated with [Devin](https://cli.devin.ai/docs)
Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Generated with [Devin](https://cli.devin.ai/docs) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
fglock
force-pushed
the
feature/git-modules-support
branch
from
02071b3 to
9df6489
Compare
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
1 participant
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.
Summary
Make Git-related CPAN modules work on PerlOnJava. Plan & investigation in
dev/modules/git_modules_support.md.Results
Git::Rawis XS-only libgit2 and remains unsupported — a JGit-backedport would be a separate, sizable effort comparable to the recent
Crypt::OpenSSL Bouncy Castle work.
Changes
1.
fix(tie): dispatch STORE/FETCH through tie magic underlocal``GlobalRuntimeScalar.dynamicSaveState/dynamicRestoreStatepreviouslyswapped the tied slot for a fresh untied
GlobalRuntimeScalarwhenentering a
local $tied = valuescope, silently dropping the tie.File::chdir's tied$CWDnever calledchdirunderlocal, whichmeant
Git::Wrapper'slocal $CWD = $self->dirwas a no-op — everyfailure in its suite traced back to this one bug.
Now, when the slot holds a
TIED_SCALAR, the tie stays in place forthe whole localized scope. We dispatch
STORE(undef)on entry andSTORE(savedValue)on exit, matching real Perl's observable behaviour(verified against system
perl).New subtest in
src/test/resources/unit/tie_scalar.tpins this down.2.
feat(System::Command): IPC::Open3 fallback for PerlOnJava (no fork)System::Command::_spawnwas hand-rolledpipe + fork + exec. Sincethe JVM has no
fork, everyGit::Repositorytest harness skippedwith
1..0 # SKIP fork() not supported on this platform (Java/JVM).The bundled
src/main/perl/lib/System/Command.pmnow detectsPerlOnJava via
$Config{perlonjava}and routes the child throughIPC::Open3::open3(already implemented on top ofjava.lang.ProcessBuilder). The caller's existingcwd/envhandling (
chdir+local %ENV) around_spawnis untouched.3.
test(System::Command): bundle test suite under src/test/resources/moduleAdd the cleanly-passing subset of System::Command's test suite
(
00-compile.t,01-load.t,21-loop_on.t,25-refopts.t,90-output.t+t/lines.pl) undersrc/test/resources/module/System-Command/so the patch is regression-covered by
make test-bundled-modules.Tests that need deeper fork semantics are left out for now and
documented in the plan doc.
The
testModulegradle task now exportsPERLONJAVA_EXECUTABLEpointing at the project's
jperllauncher so$^Xresolves duringtests that spawn a child perl (most of System::Command's suite).
Test plan
make— full unit tests passmake test-bundled-modules— all 5 new System::Command tests pass./jcpan -t Git::Wrapper— 75/75./jcpan -t System::Command— 132/140 (remaining 8 are SHLVL diffs,unrelated to fork)
./jcpan -t Git::Repository— 304/328 (remaining failures areminor edge cases)
tie_scalar.t: local on tied scalar dispatches STORE/FETCHpasses under both system
perlandjperlCaveat
~/.perlonjava/libtakes precedence over the JAR-bundled lib in@INC.Users who already installed System::Command from CPAN need to
rm ~/.perlonjava/lib/System/Command.pmonce to pick up the bundledpatch. New installs Just Work. Plan doc discusses an install-time
patching hook as follow-up.
Generated with Devin