Ticket #1194 (closed bug: fixed)

Opened 5 years ago

Last modified 5 years ago

7 config steps improperly rely on Perl 5 %Config 'OSNAME' element

Reported by: jkeenan Owned by: jkeenan
Priority: normal Milestone:
Component: configure Version: 1.7.0
Severity: medium Keywords:
Cc: mikehh Language:
Patch status: Platform:

Description

In the course of working on TT #1189, I have stumbled upon certain problems with the Parrot configuration system which go beyond the question of the dependence of the t/steps/*.t tests on init::defaults.

In this ticket I describe one such problem: Several configuration steps depend on the value for OSNAME found in the Perl 5 %Config even after Parrot's Configure.pl has made a later (and, presumably, better) determination of what osname should be.

The Parrot configuration value for osname is determined in auto::arch. One would therefore expect that when subsequent configuration steps needed to know the name of the operating system for some purpose, it would use osname. The following configuration steps, however, continue to rely on the value found in the Perl 5 %Config:

config/auto/gettext.pm
config/auto/opengl.pm
config/auto/gmp.pm
config/auto/gdbm.pm
config/auto/crypto.pm
config/auto/readline.pm
config/auto/pcre.pm

The above files have code like this:

    my $osname = $conf->data->get_p5('OSNAME');

... instead of code like this:

    my $osname  = $conf->data->get('osname');

Unless someone beats me to it, I plan to start a branch to convert the seven files above to using Parrot's osname.

(Note: There are other issues here which I'll defer to future tickets: Is auto::arch's method for determining osname correct? Where should auto::arch fall in the sequence of configuration steps? What do we do about configuration steps (e.g., auto::headers) which request the Perl 5 OSNAME but which come before auto::arch in the configuration step sequence?)

Thank you very much.

kid51

Change History

  Changed 5 years ago by jkeenan

  • status changed from new to assigned

  Changed 5 years ago by jkeenan

Started convert_OSNAME branch to work on this ticket.

  Changed 5 years ago by jkeenan

  • cc mikehh added

Work in branch complete. Passes make fulltest on Linux/i386. Testing it now on Darwin/PPC; expect it to pass make test. It would be good to get a report on a 64-bit platform. But, in any event, I will probably merge this into trunk between now and Saturday Nov 07.

Thank you very much.

kid51

follow-up: ↓ 5   Changed 5 years ago by mikehh

I get the same results for tests as on trunk.

run with:

perl Configure.pl --optimize --test --cc=g++ --cxx=g++ --link=g++ --ld=g++ --maintainer --configure_trace

at r42280 in convert_OSNAME branch  http://smolder.plusthree.com/app/public_projects/report_details/29679

t/src/warnings.t fails test 2 - --optimize problems - TT #1187

pre/post-config tests PASS

in fulltest I get a failure with testr but also get this in trunk

Test Summary Report
-------------------
t/pmc/eval.t                       (Wstat: 0 Tests: 17 Failed: 0)
  TODO passed:   12
t/pmc/threads.t                    (Wstat: 256 Tests: 14 Failed: 1)
  Failed test:  9
  Non-zero exit status: 1
Files=221, Tests=6718, 61 wallclock secs ( 3.07 usr  0.51 sys + 55.79 cusr 33.96 csys = 93.33 CPU)
Result: FAIL
make[1]: *** [testr] Error 1

the rest of fulltest passes (except for the t/src/warnings.t test 2)

[in trunk if I build with gcc t/pmc/threads.t passes testr but the TODO test fails]

in reply to: ↑ 4 ; follow-up: ↓ 6   Changed 5 years ago by jkeenan

Replying to mikehh:

I get the same results for tests as on trunk.

Mike:

I have to apologize for an error which led you to waste your time. Checking my commit logs, I see that I had not actually done 'svn ci' on the 7 files which needed to change. So of course you got the same results as in trunk. It was trunk (for all practical purposes).

In r42295 I finally did the commits. Would you be able to re-test?

Thank you very much.

kid51

in reply to: ↑ 5   Changed 5 years ago by jkeenan

Replying to jkeenan:

I've identified all the configuration steps which explicitly rely on Perl 5 %Config data by calling $conf->data->get_p5('something'). Three of those steps call get_p5('OSNAME') and are run after auto::arch.

config/auto/thread.pm
config/gen/opengl.pm
config/gen/platform.pm

So, in principle, we should be able to replace OSNAME with osname in those steps. That worked for auto::thread and gen::opengl (see r42300 and  Smolder 29686), but I got test failures for gen::platform.

Continuing to investigate.

  Changed 5 years ago by jkeenan

In r42431, I converted these configuration steps to using $conf->data->get('osname');

U    config/auto/gettext.pm
U    config/auto/opengl.pm
U    config/auto/thread.pm
U    config/auto/gmp.pm
U    config/auto/gdbm.pm
U    config/auto/crypto.pm
U    config/auto/readline.pm
U    config/auto/pcre.pm
U    config/gen/opengl.pm

config/gen/platform.pm had testing problems and is not yet converted.

Please post any test failures resulting from this change. Thank you very much.

kid51

follow-up: ↓ 9   Changed 5 years ago by jkeenan

Of the configuration steps which follow auto::arch, only gen::platform still calls $conf->data->get_p5('OSNAME'). Eliminating this last call is, however, a bit tricky, as it is bound up with that step's internal method _get_platform(), which is used to set a value for a $platform used in path names and header file content. _get_platform() also depends on the value of Parrot::Configure attribute archname, which, in most cases, is ultimately dependent on what is read in the Perl 5 %Config.

IMO, it would be better if a value for this $platform were determined much earlier in the configuration process -- most likely in config/auto/arch.pm, at the same time that the values for cpuarch and osname are finalized. I believe that the gen configuration step classes should not be making determinations about important configuration data such as the platform descriptor. Rather, they should be doing only what their name implies, i.e., generating files from data accumulated earlier in the Parrot::Configure object.

That, however, is a bit too much scope creep for one ticket. I will keep this ticket open for a few more days to allow for any complaints or test failures. Then, after I've applied a patch dealing with a similar issue in config/auto/format.pm (see TT #1236), I will open a new TT which moves determination of the platform descriptor out of gen::platform and into (in all likelihood) auto::arch.

Thank you very much.

kid51

in reply to: ↑ 8   Changed 5 years ago by jkeenan

  • status changed from assigned to closed
  • resolution set to fixed

Replying to jkeenan:

I will keep this ticket open for a few more days to allow for any complaints or test failures. Then, after I've applied a patch dealing with a similar issue in config/auto/format.pm (see TT #1236), I will open a new TT which moves determination of the platform descriptor out of gen::platform and into (in all likelihood) auto::arch.

The new ticket is TT #1263. Closing this ticket.

Thank you very much.

kid51

Note: See TracTickets for help on using tickets.