fix: fork-open exec emulation and pipe open %ENV passing#383
Merged
Conversation
The fork-open emulation (open FH, "-|") was not correctly handling
the indirect object syntax exec { $cmd[0] } @cmd used by Module::Build.
The completeForkOpen() method was passing raw flattenedArgs to ProcessBuilder,
but with indirect object syntax, flattenedArgs[0] is the program and
flattenedArgs[1] is the duplicate program name (argv[0]).
This fix mirrors the logic from the regular exec() path that correctly
extracts the program and skips the argv[0] argument.
Fixes Module::Build _backticks() which uses this pattern.
Generated with [Devin](https://cli.devin.ai/docs)
Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
When `open STDOUT, "> file"` redirects STDOUT to a file, the RuntimeIO.selectedHandle was not updated. This caused `print` without an explicit filehandle to still write to the original stdout instead of the file. The fix updates RuntimeGlob.setIO() to check if the old IO was the currently selected handle, and if so, updates selectedHandle to point to the new IO. This enables proper STDOUT/STDERR redirect patterns used by Module::Build and other modules. Generated with [Devin](https://cli.devin.ai/docs) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
When `open $fh, ">&BAREWORD"` is used to duplicate a filehandle by
name, look up the bareword in the caller's package first before
falling back to main::. This fixes the "Unsupported filehandle
duplication: SAVEOUT" error when using patterns like:
package MBTest;
local *SAVEOUT;
open SAVEOUT, ">&" . fileno(STDOUT);
# ... redirect ...
open STDOUT, ">&SAVEOUT"; # This now works
Uses RuntimeCode.getCurrentPackage() which leverages caller() to
get the package context at runtime, working for both JVM-compiled
and interpreter code.
Generated with [Devin](https://cli.devin.ai/docs)
Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
The jar:PERL5LIB marker is an internal PerlOnJava convention for accessing modules bundled in the JAR file. When Test::Harness spawns child processes, it passes @inc entries as -I switches, but jar: paths do not exist as filesystem directories. Filter out jar:* entries from @inc in both _filtered_inc() and _default_inc() to prevent passing invalid -I switches to child test processes. Generated with [Devin](https://cli.devin.ai/docs) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
PipeInputChannel and PipeOutputChannel were not copying Perl's %ENV hash to the ProcessBuilder environment, causing environment variables (including PERL5LIB) to be invisible to child processes spawned via pipe open (e.g., open($fh, "$command|")). This fix adds copyPerlEnvToProcessBuilder() to both classes, matching the behavior already present in SystemOperator.java for system() and exec(). Also fixes Test/Harness.pm to convert relative paths to absolute paths so they work correctly in child processes that may run from a different directory. Generated with [Devin](https://cli.devin.ai/docs) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Since PerlOnJava can't use fork() to share address space, child processes don't inherit @inc modifications. This caused tests using -Mblib to fail when they spawn child processes (e.g., Module::Build compatibility tests). The fix makes blib.pm set PERL5LIB in addition to modifying @inc, so child processes can find modules in blib/lib and blib/arch. Generated with [Devin](https://cli.devin.ai/docs) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
The Java implementation of canonpath() was missing the logic to strip
leading "./" from paths, unlike the pure-Perl File::Spec::Unix version.
This caused issues in Module::Build where files stored with
"lib/Simple.pm" didn't match canonicalized "./lib/Simple.pm" paths,
leading to files being incorrectly deleted during clean().
Now canonpath("./lib/Simple.pm") returns "lib/Simple.pm" as expected,
while preserving "./" when it's the entire path (e.g., canonpath("./")
returns "./").
Generated with [Devin](https://cli.devin.ai/docs)
Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Previously, when a regex with optional capture groups like m{^(.*/)?(.*)}s
matched against a string without a slash (e.g., "MANIFEST"), the optional
group that did not participate in the match was skipped in the return list.
Perl returns undef for groups that do not participate, so:
my ($a, $b) = ("MANIFEST" =~ m{^(.*/)?(.*)}s);
Should yield: $a = undef, $b = "MANIFEST"
But PerlOnJava was yielding: $a = "MANIFEST", $b = undef
This broke File::Basename::dirname() which uses this pattern, causing
dirname("MANIFEST") to return "MANIFEST" instead of "." as expected.
The fix includes undef values in the return list for unmatched groups,
matching Perl behavior.
Generated with [Devin](https://cli.devin.ai/docs)
Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
The previous fix to include undef for unmatched optional groups (commit 4c3a0b4) caused a regression in branch reset patterns (?|...). These patterns were returning undef values for groups in non-matching alternatives. The issue is that Java regex engine creates separate capture groups for each alternative in a branch reset pattern, while Perl reuses the same group numbers. When alternative 3 matches, Java returns: - Groups 1-4: null (from non-matching alternatives 1-2) - Groups 5-6: the matched values (from alternative 3) With the undef fix, all 6 groups were being returned, breaking assignment to fewer variables. The fix tracks whether a pattern uses branch reset syntax and: - For branch reset patterns: skip null groups (original behavior) - For non-branch-reset: include undef for unmatched optional groups This restores the re/pat.t test count to 1065/1298 while preserving the File::Basename::dirname fix for patterns like m{^(.*/)?(.*)}s. Generated with [Devin](https://cli.devin.ai/docs) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Added documentation about JPERL_UNIMPLEMENTED, JPERL_OPTS, and PERL_SKIP_BIG_MEM_TESTS environment variables that perl_test_runner.pl sets automatically but need to be set manually when running tests directly with ./jperl. Generated with [Devin](https://cli.devin.ai/docs) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
4ed242d to
02f44e6
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
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
This PR contains multiple fixes for fork/pipe/exec emulation issues and related bugs discovered while testing Module::Build.
Fix 1: Handle indirect object syntax in fork-open exec emulation
Problem: Module::Build's
_backticks()method uses this pattern:The fork-open emulation was returning empty output because
completeForkOpen()was not handling the indirect object syntax correctly.Fix: Mirror the logic from regular
exec()to extract the correct arguments.Fix 2: Pipe open not passing %ENV to child processes
Problem: Pipe open (
open($fh, "$command|")) was NOT passing the Perl%ENVhash to child processes. This caused:$ENV{PERL5LIB}) to be invisible to childrenFix: Added
copyPerlEnvToProcessBuilder()method to bothPipeInputChannel.javaandPipeOutputChannel.java.Fix 3: Test/Harness.pm relative path handling
Fixes Test/Harness.pm to convert relative paths to absolute paths so they work correctly in child processes that may run from a different directory.
Fix 4: blib.pm not setting PERL5LIB
Problem: The
-Mblibswitch only modified@INCin the current process but did not setPERL5LIB. Since PerlOnJava cannot usefork()to share address space, child processes did not inherit the blib paths.Fix: Modified
blib.pmto also setPERL5LIBso child processes can find modules inblib/libandblib/arch.Fix 5: File::Spec->canonpath not removing leading ./
Problem: The Java implementation of
File::Spec->canonpath()did not strip leading./prefixes from paths, unlike the Perl version.Example:
canonpath("./lib/Simple.pm")returned"./lib/Simple.pm"instead of"lib/Simple.pm"Impact: Caused Module::Build's
DistGen->clean()to fail matching files, leading to incorrect file deletion.Fix: Updated
FileSpec.javato remove leading./while preserving./when it is the entire path.Fix 6: Regex capture groups for optional groups
Problem: When a regex with optional capture groups (like
m{^(.*/)?(.*)}s) matched, groups that did not participate were skipped in the return list instead of returningundef.Example:
my ($a, $b) = ("MANIFEST" =~ m{^(.*/)?(.*)}s)was yielding($a = "MANIFEST", $b = undef)instead of($a = undef, $b = "MANIFEST")Impact: Broke
File::Basename::dirname()which relies on this pattern, causingdirname("MANIFEST")to return"MANIFEST"instead of".". This in turn caused Module::Build to incorrectly delete all files during clean().Fix: Updated
RuntimeRegex.javato include undef values for unmatched groups.Fix 7: Regex capture groups for branch reset patterns
Problem: Fix 6 caused a regression (-8 tests) in re/pat.t for branch reset patterns (
(?|...)). These patterns were returning undef values for groups in non-matching alternatives.Root cause: Java's regex engine creates separate capture groups for each alternative in a branch reset pattern, while Perl reuses the same group numbers. When alternative 3 of
(?|(p)(q)|(x)(y)|(a)(b))matches "ab":With Fix 6, all 6 groups were returned as
(undef, undef, undef, undef, "a", "b")instead of("a", "b").Fix: Track whether a pattern uses branch reset syntax and:
This restores re/pat.t to 1065/1298 while preserving the File::Basename fix.
Fix 8: Document test environment variables
Added documentation to AGENTS.md about environment variables needed when running tests directly (
JPERL_UNIMPLEMENTED,JPERL_OPTS,PERL_SKIP_BIG_MEM_TESTS).Test Plan
make)exec { $cmd[0] } @cmdwith fork-open now returns correct output-Mblibnow properly propagates blib paths to child processesFile::Spec->canonpath("./foo")correctly returns"foo"File::Basename::dirname("MANIFEST")correctly returns".""ab" =~ /(?|(p)(q)|(a)(b))/returns("a", "b")correctlyGenerated with Devin