-
Notifications
You must be signed in to change notification settings - Fork 67
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
cperl and XSConfig support #63
base: master
Are you sure you want to change the base?
Conversation
t/source_handler.t
Outdated
@@ -355,9 +355,10 @@ sub test_handler { | |||
|
|||
SKIP: | |||
{ | |||
my $planned = 1; | |||
%int::; # init the coretype if missing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the comment
@@ -4,7 +4,21 @@ BEGIN { | |||
*CORE::GLOBAL::fork = sub { die "you should not fork" }; | |||
} | |||
use Config; | |||
tied(%Config)->{d_fork} = 0; # blatant lie | |||
if ($Config::Config{d_fork}) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments would be helpful here, to explain what is going on. What is the fix, and why is this code needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
%Config is readonly in theory but in apparently one of my predecessors found an ugly way around that. XSConfig is more strictly readonly.
A far cleaner solution IMO to this would be to make a writable copy of %Config
, modify the d_fork
entry and replace the hash member in *Config::Config with this new hash (or untie and assign). This would need to be done before anything else imports Config.
perl -E 'require Config; my %Config = %Config::Config; $Config{cc} = "something"; *Config::Config = \%Config; say $Config::Config{cc}'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cleaner solution would be to use Mock::Config of course. But this is a core module, so I went with the least effort.
With XSConfig with p5p perl, or cperl, Test::Harness fails in t/nofork-mux.t because of reliance on internal implementation of Config.pm. Without using any external libraries or modules, fake not having fork by putting a shim between the XS FETCH sub/method and the Config class that traps and changes 'd_fork' key. Sample of error below that is fixed with this patch. C:\cpan\testharness>prove -v t/nofork-mux.t t/nofork-mux.t .. Not a HASH reference at t/lib/NoFork.pm line 7. Compilation failed in require at t/nofork-mux.t line 10. BEGIN failed--compilation aborted at t/nofork-mux.t line 10. Dubious, test returned 255 (wstat 65280, 0xff00) No subtests run Test Summary Report ------------------- t/nofork-mux.t (Wstat: 65280 Tests: 0 Failed: 0) Non-zero exit status: 255 Parse errors: No plan found in TAP output Files=1, Tests=0, 0 wallclock secs ( 0.11 usr + 0.00 sys = 0.11 CPU) Result: FAIL Original author of this code is Reini Urban, in cperl commit 42cac300080de7d3a9df8eed040750ec9583de20 "Test-Harness: fix 2 tests" on "Date: 9/28/2015 10:59:36 AM". The code was submitted upstream in Apr 2017 in Perl-Toolchain-Gang#63 but that PR stalled. I've extracted the minimum code needed to get T::H to pass with XSConfig with p5p perl and placed it in this patch so that the code can be published sooner.
just TODO the failing tests
14f8f52
to
bbdd07f
Compare
Config maybe readonly.
cperl uses stricter types for the typical tests, to catch errors at compile-time already.