On 9/1/07, James E Keenan <[EMAIL PROTECTED]> wrote:
> (Gabor: You didn't cc: the list on this, so I'm replying directly to
> you. But I think the issues you are raising merit discussion on the
> list. So please feel free to copy what I write below to the list.)
Sure, I meant to send it to the list actually.
It seems the beer from YAPC::EU still have not fully settled down..
> On Sep 1, 2007, at 12:35 PM, Gabor Szabo wrote:
>
> > Thanks for the pointers, I was not sure where to add this.
> >
> > On 9/1/07, James E Keenan <[EMAIL PROTECTED]> wrote:
> >> 2. Modify config/auto/gc.pm to have configuration step auto::gc
> >> fail if
> >> any invalid option is provided. (I did a quick grep on config/
> >> and this
> >> is the only place I saw the 'gc' option handled -- but I might have
> >> gotten confused by the many instances of 'gcc'.) A statement
> >> along the
> >> lines of 'return unless $gc =~ /list|of|valid|options/;' around
> >> line 62
> >> of that package would probably be the way to go.
> >
> > The problem is that if I place a return; in the function you
> > recommended
> > the configuration process displays a small error message, then
> > a bigger unrelated one (at least unrelated from the point of view
> > of the
> > user running it) , creates the Makefile and exit()s with exit value 0
> > meaning success.
> > It also suggests to type in 'make' to build Parrot.
> >
> > Even if I croak() in that function, the same happens.
> >
>
> I understand your point -- but it's a much larger point than the
> permissible values for the --gc option. Read on.
>
> > IMHO if a user supplies an invalid option (to --gc or any other
> > parameter)
> > an appropriate error message should be printed, the configuration
> > process
> > should be halted and exit() should indicate failure.
>
> All of the (currently) 58 Parrot configuration steps are set up to
> return undef on failure. But Parrot::Configure::runsteps() is set up
> as a harness that reports failures in individual steps/tasks but
> nonetheless tries to keep going forward. It does nothing with the
> return value of a particular step.
>
> #####
> sub runsteps {
> my $conf = shift;
>
> my $n = 0; # step number
> my ( $verbose, $verbose_step, $ask ) =
> $conf->options->get( qw( verbose verbose-step ask ) );
>
> foreach my $task ( $conf->steps ) {
> $n++;
> $conf->_run_this_step( {
> task => $task,
> verbose => $verbose,
> verbose_step => $verbose_step,
> ask => $ask,
> n => $n,
> } );
> }
> return 1;
> }
> #####
>
> Whether that's what it ideally *should* be doing is a valid
> question. But in the refactoring/testing work I've been doing on it
> in recent months, I've viewed it as my mandate to preserve its
> current functioning unless there's a strong consensus that change is
> needed.
>
> Some might argue, "If you're foolish enough to continue on to 'make'
> when your configuration process is spewing error messages, you get
> what you deserve." That's in effect what happens now.
>
> Another alternative would be to note whether any of the configuration
> steps fails, then change the message which
> Parrot::Configure::Messages::print_conclusion() prints after all the
> configuration steps complete.
At this point I really don't want to get involved any deeper in the development
and I am sure you will clean this up later so I'll try to send a patch according
to what you wrote.
One thing though, I think it would be really good if at least the exit code of
the script would be something different from 0 when any of the steps failed.
That will be very important when we try to integrate with the PG Build Farm
or any other fully automated smoke test suite to make sure we won't go on
when the configuration step fails.
Gabor
