fix Issue 13290 - rdmd --eval ignores flags and program args

[ghost]

https://issues.dlang.org/show_bug.cgi?id=13290
https://issues.dlang.org/show_bug.cgi?id=13344 (but there is #143 for just this issue)
https://issues.dlang.org/show_bug.cgi?id=13345
https://issues.dlang.org/show_bug.cgi?id=13346

Depends on dlang/phobos#2429

[ghost]

This adds yet more arg handling logic to rdmd. Instead, it may be better to add some little features to std.getopt:

• stop processing on a predicate (stop on source and executable file names)
• distinguishing unrecognized options from non-options (dmd args vs program args)

[CyberShadow]

Do we really need --eval to be able to handle args?

[ghost]

Instead, it may be better to add some little features to std.getopt

Made a PR for that: dlang/phobos#2429
This PR now depends on that one.

Also, this now enables rdmd --eval 'writeln("Hello World!");'. Before, the code would have been mistaken for the program file.

[ghost]

Do we really need --eval to be able to handle args?

I think it would be an arbitrary restriction if --eval couldn't handle args. I don't see a reason against supporting it.

[quickfur]

I'm not sure, is there a point to writing things like:

rdmd --eval 'writeln(args[1]);' -- "Hello world"

as opposed to just:

rdmd --eval 'writeln("Hello world");'

?

[ghost]

On Friday 15 August 2014 11:05:02 H. S. Teoh wrote:

I'm not sure, is there a point to writing things like:

rdmd --eval 'writeln(args[1]);' -- "Hello world"

as opposed to just:

rdmd --eval 'writeln("Hello world");'

?

There is a point when you're learning D, and want to figure out how that args
thing works.

There may be a point when piecing together args is easier than piecing
together D code:

rdmd --eval 'writeln(args[1 .. $])' -- `ls`

I don't see a point in not supporting it. What's the downside?

[quickfur]

I dunno, if code gets to the point where you start needing to parse args, I'd argue that you should start using a source file instead of parsing a long list of options to rdmd. :) But, this is just my opinion. I'll leave it to the other reviewers to comment further.

[CyberShadow]

I don't see a point in not supporting it. What's the downside?

Cost of maintenance. If someone will want to add something else to the argument parsing logic, the code's complexity will continue increasing. And if someone starts using the functionality and discovers a bug in it, there will need to be someone to understand the code and fix it.

I'm not convinced about the std.getopt patch for mostly the same reasons, it seems to be a rather narrow use-case.

[quickfur]

I think that at the very least, it should be made possible to achieve what Nils wants to achieve in std.getopt, even if direct support isn't actually built in. I do find D's std.getopt rather crippled compared to, say, Perl's, or for that matter, Posix C's getopt, when it comes to argument processing. While I agree that we should minimize complexity in std.getopt, we should also at least make it configurable enough that it will be able to hand off complex needs to user code, instead of being so inflexible that anyone who wants to extend it ends up having to reinvent their own flavor of getopt.

[CyberShadow]

Fair enough, I can get behind the idea of keeping the user's options open, as long as the design is sound.

[ghost]

On Friday 15 August 2014 13:18:03 Vladimir Panteleev wrote:

Cost of maintenance. If someone will want to add something else to the
argument parsing logic, the code's complexity will continue increasing. And
if someone starts using the functionality and discovers a bug in it, there
will need to be someone to understand the code and fix it.

This PR decreases the complexity of rdmd's arg parsing, by letting getopt
handle things.

I'm not convinced about the std.getopt patch for mostly the same reasons, it
seems to be a rather narrow use-case.

They are very minor additions. But they do make getopt slightly more complex,
of course.

The point is, without them, if rdmd was to have proper arg parsing (rdmd -- option value doesn't work right now), it would have to re-implement getopt.

And once rdmd has proper arg parsing, supporting --eval + args is trivial.

I see three ways to go:

1. rdmd does use getopt, but parsing is weird, because of homemade cruft on
   top of it. That's what we have right now.
2. rdmd doesn't use getopt, does all arg parsing by itself. getopt is right
   there, would be a shame not to use it.
3. getopt gets the proposed features. rdmd makes use of them, gets rid of
   homemade cruft. I'm pushing for this one.

[ghost]

I split up https://issues.dlang.org/show_bug.cgi?id=13290 into three separate issues:
https://issues.dlang.org/show_bug.cgi?id=13344
https://issues.dlang.org/show_bug.cgi?id=13345
https://issues.dlang.org/show_bug.cgi?id=13346

The fix for 13344 doesn't require touching getopt. I made PR #143 for that.

[ghost]

Here's another idea for making getopt flexible enough for rdmd:

Have a function std.getopt.getOneOpt (or popopt or whatever) that handles and removes one option from the front of args. If args[0] is no option, it does nothing.

Usage in rdmd would look something like this:

string[] compilerArgs;
string[] programFileAndArgs;
args.popFront(); // rdmd itself
while(!args.empty)
{
    auto r = getOneOpt(args, /* etc */);
    switch(r)
    {
        case GetOneOptResult.handledOption: /* ok, go on */ break;

        case GetOneOptResult.notRecognized:
        if(isProgram(args[0])) // everything from here is program + args
        {
            programFileAndArgs = args;
            args = [];
        }
        else // flag or file for the compiler
        {
            compilerArgs ~= args[0];
            args.popFront();
        }
        break;

        case GetOneOptResult.endOfOptions: // everything from here is program + args
        programFileAndArgs = args;
        args = [];
        break;
    }
}

Essentially, this gives the user a way to intervene on each argument. Allowing them to choose per argument to handle it themselves, or stop processing, or just let getOneOpt do its thing, or call another version of getOneOpt or getopt, etc.

getopt itself would be implemented in terms of getOneOpt, too. That would probably need a major refactoring, because getopt currently does foreach(options) {foreach(args) {}}, and it would have to be the other way around.

Would this fly?

[quickfur]

I agree that the current implementation of getopt could use some improvement -- currently, it scans over the entire argument list per available option, which seems horribly inefficient for me. Of course, most of the time this doesn't matter since we're only dealing with a small number of arguments and a relatively small number of possible options. But in non-trivial CLI applications this cost could become prohibitive.

I'm also not particularly pleased with the way --help is implemented. It looks more like a hack than a properly designed implementation.

I'm not so sure getOneOpt is a good idea, though. The whole point of using getopt is to be able to specify all available options in a single place, and needing to call getOneOpt multiple times feels like the wrong approach. I think we should take a step back at look at a possible redesign of getopt, perhaps via discussion on the forum, and do a proper, clean, reimplementation of it, taking into account what others have done before (I especially like Perl's implementation of it, even though I don't recommend copying the Perl approach 100%, I do think its flexibility is something we should try to emulate). In particular, I'd like to have a sane way of supporting a completely-automated --help instead of the current helpNeeded hack. This is not possible within the current design.

[ghost]

On Thursday 21 August 2014 07:37:18 H. S. Teoh wrote:

I agree that the current implementation of getopt could use some improvement
-- currently, it scans over the entire argument list per available option,
which seems horribly inefficient for me.

foreach(options) foreach(args) isn't fundamentally worse than foreach(args) foreach(options), though, or am I missing something?

[...]

I'm not so sure getOneOpt is a good idea, though. The whole point of using
getopt is to be able to specify all available options in a single place,

There may be a misunderstanding here. You'd still give getOneOpt all option
definitions, but it wouldn't process all args in one go. Also, getopt
wouldn't go anywhere.

and needing to call getOneOpt multiple times feels like the wrong approach.
I think we should take a step back at look at a possible redesign of
getopt, perhaps via discussion on the forum, and do a proper, clean,
reimplementation of it,

I'm hesitating to start from scratch, breaking everything.

taking into account what others have done before (I
especially like Perl's implementation of it, even though I don't recommend
copying the Perl approach 100%, I do think its flexibility is something we
should try to emulate).

I'm looking at http://perldoc.perl.org/Getopt/Long.html#Configuring-Getopt::Long, and as far as I can see, our getopt already tries to mimic that
rather closely. Interface-wise that is.

Also, I can't figure out how to implement rdmd's arg style with perl's getopt.
It allows to set a callback for non-option args, but how to stop processing at
an arbitrary arg and get everything behind that point?

In particular, I'd like to have a sane way of
supporting a completely-automated --help instead of the current
helpNeeded hack. This is not possible within the current design.

What's missing? And how is the current design in its way?

Option descriptions can be passed along with the names. And
defaultGetoptPrinter prints all options and their descriptions in a nice
table.

getopt could do that itself and exit the program, but I guess it was a
conscious decision not to do so. We could of course add a config flag for that.

[quickfur]

It's true that foreach(args) foreach(opts) isn't fundamentally different from foreach(opts) foreach(args), but the former can be improved by using a faster lookup scheme for opts than linear search.

Sorry, gotta run, will type the rest of my reply later.

[ghost]

Ping?

[quickfur]

Sorry, got busy with other stuff and forgot about this one.

What I meant about getOneOpt is that if you need to call it in multiple places, you'd have to re-specify all of the arguments again. So pretty much the only practical way to use it is to call it in a loop, in which case we might as well go back to a single-call getopt.

Wrt the automated help, I was thinking more in terms of a more declarative approach, where user code would specify each option, its receiving variable, plus associated help text, and getopt would automatically construct the help text from that. Currently, you have to manually test for helpNeeded and manually keep the help text up-to-date, which is hard because the text is separate from the option specs. Ideally, we'd want to keep the two together.

One approach, though, that doesn't require ground-up rewrite, is to define a special HelpText struct that contains the help text, and which getopt recognizes and treats as part of the option spec. Then you'd be able to write something like:

int n;
float x;
getopt(
    "n", &n, HelpText("The number of iterations to run"),
    "x", &x, HelpText("The initial seed value for the algorithm"),
    ....
);

Then getopt would be able to automatically construct help text when the user asks for it.

But of course, this is an enhancement request beyond the scope of this PR.

[ghost]

What I meant about getOneOpt is that if you need to call it in multiple places, you'd have to re-specify all of the arguments again.

Ok, got it. I agree that passing the option spec over and over again is not ideal.

The first thing I had thought of was to add a callback for non-options. I had dismissed that for some reason, but I can't see why now. It would signal if processing is to stop, and if the arg is to be removed from the array.

It would be possible to expand on the idea of getOneOpt, separating the option specificiation from the popping, maybe turn it into a range. But it would still be out of line with the rest of getopt which works with callbacks.

So, I'd go with the callback. And I think I'm running out of fuel if that's not good either.

---

Wrt the automated help, I was thinking more in terms of a more declarative approach, where user code would specify each option, its receiving variable, plus associated help text, and getopt would automatically construct the help text from that.

You can already pass help text for options as part of the option spec:

import std.getopt;
void main(string[] args)
{
    int n;
    float x;
    auto r = getopt(args,
        "n", "The number of iterations to run", &n,
        "x", "The initial seed value for the algorithm", &x,
        // ....
    );

    if(r.helpWanted)
        defaultGetoptPrinter("Some information about the program.", r.options);
}

Called with --help this prints:

Some information about the program.
      --n The number of iterations to run
      --x The initial seed value for the algorithm
-h --help This help information.

---

By the way, could I get someone to review PR #143? It fixes the one issue I had originally hit. It doesn't require any changes to getopt, doesn't touch arg parsing at all.

[andralex]

#143 is in, what's the status of this?

[ghost]

#143 is in, what's the status of this?

The current code here is obsolete.

I'd like to get a green light, or rather a second opinion, on the proposed addition to getopt before I get to work again. That is, how about adding a callback for non-options that

• handles the argument, of course;
• can signal if the argument is to be removed from the array; and
• can stop argument processing right there.

Option callbacks could get those abilities, too, of course.

rdmd would then use that to stop on the first non-option argument, leaving it in the array.

[MartinNowak]

Any work on the underpowered getopt would be appreciated, but I agree, that passing arguments to eval is too much.
You might want to try http://drepl.dawg.eu or http://dpaste.dzfl.pl.

[ghost]

Any work on the underpowered getopt would be appreciated, but I agree, that passing arguments to eval is too much.

Point is, when getopt gets that little extra power, --eval+args doesn't need any extra work. Then arguments for the program are simply what's behind getopt's stop position.