[Ruby 3.0 support] Add pager support into Launcher

[Strech]

⚠️ This is not a code to review, but rather PoC and discussion ⚠️

After digging into the MRI implementation and Graal Launcher I have a set of questions and problems to ask.

MRI-features question

The MRI pager support comes with variates of sub-features. And it would be cool to understand which one I should port:

1. COLUMNS environment variable and word wrapping
2. Section highlighting (via escape codes)
3. HAVE_WORKING_FORK-thing which I'm not sure how is used 🤔

Implementation question

In Graal SDK for Launcher, all the tiny methods to display help combined inside the runLauncherAction method which also should return true to terminate the build part. And this method seems the best place to inject new output. The problem I face is that I can't determine does the --help is actually called or it's a --version.

Unfortunately in the Graal launcher help variable is declared as private and I can't use it. Is there a way to understand it or I'm doing it in the wrong place?

And the main issue I see is that logic is spread across many methods and some of them are responsible for termination decisions 🤔

P.S Does the autoFlash a concern or it's totally fine for such small output as help?

[Strech]

@eregon May I ask you for some help here?

[eregon]

I don't think we should touch runLauncherAction().
I think we just want special behavior for --help as handled in RubyLauncher.

We can know if --help was used by setting a flag in CommandLineParser.

But I think we don't even need to know that, RubyLauncher's protected void printHelp(OptionCategory maxCategory) seems only called for --help.

So I think just adding the logic there to set the output and use a pager seems the best.

I think highlighting is not so important, and it would likely require more effort than what it's worth for the help output by the launcher (Runtime options: and below).

[Strech]

But I think we don't even need to know that, RubyLauncher's protected void printHelp(OptionCategory maxCategory) seems only called for --help.

Yes, that was my very first attempt, but let me show the issue

This is an original runLauncherAction which triggers different print statements, and I've checked them

    protected boolean runLauncherAction() {
        boolean printDefaultHelp = help || ((helpExpert || helpInternal) && kindAndCategory.isEmpty() && !helpVM);
        OptionCategory hc = getHelpCategory();
        if (printDefaultHelp) {
            printDefaultHelp(hc); <------ this one will print main Ruby help (and later printHelp call)
        }
        maybePrintAdditionalHelp(hc);

        if (helpVM) {
            if (nativeAccess == null) {
                printJvmHelp();
            } else {
                nativeAccess.printNativeHelp();
            }
        }

        return printAllOtherHelpCategories(printDefaultHelp); <------ this one will print see more in graalvm ...
    }

so if I modify just RubyLauncher's printHelp I will get part of the output in the terminal, part in the pager. (printDefaultHelp is printing some and printAllOtherHelpCategories is printing some)

I think we can achieve the same result via protected void printDefaultHelp(OptionCategory printCategory) override because it's the first method called, it's 100% help call and it will allow me to forward an entire output in the pager.

[eregon]

I see, overriding printDefaultHelp() seems a reasonable compromise.

If we want more control, we could always handle --help ourselves (much like we handle -h, and so "eat" the --help and not keep it in unrecognizedArgs), do setOutput() and call into runLauncherAction() ourselves to generate the --help output, and do any cleanup needed after it.
There is ShowHelp.LONG but that's currently unused, that could be a flag to remember we saw --help.

[eregon]

Given we probably need to wait for the pager to be quit and so need some cleanup after the output has been written, detecting and handling --help ourselves might be the better approach. Also it makes it clearer how --help is handled which is not a bad thing (it took me some time to realize how it's done currently).

BTW, we can modify the launcher superclasses if needed, but if we can avoid it it's simpler/faster.

[Strech]

Given we probably need to wait for the pager to be quit and so need some cleanup after the output has been written

Yes, the wait for the process to quit is a problem with any print method override. The current structure doesn't allow us to spawn a process and wait for it till the last print statement and that was the reason I modified runLauncherAction because there I get the full scope of printing calls and can wait after the last one.

If we want more control, we could always handle --help ourselves (much like we handle -h, and so "eat" the --help and not keep it in unrecognizedArgs)

---

detecting and handling --help ourselves might be the better approach. Also it makes it clearer how --help is handled which is not a bad thing (it took me some time to realize how it's done currently).

I will try to draft PoC with handling --help ourselves then 👍🏼
Thanks for your suggestion!

[Strech]

@eregon I've decided to go the baby steps and the first one is to get on the same page regarding that point

If we want more control, we could always handle --help ourselves (much like we handle -h, and so "eat" the --help and not keep it in unrecognizedArgs)

The problem here is that runLauncherAction requires --help to be present in unrecognizedArgs otherwise it will print nothing.

protected boolean runLauncherAction() {
  boolean printDefaultHelp = help || ((helpExpert || helpInternal) && kindAndCategory.isEmpty() && !helpVM);
  // ...
}

runLauncherAction (Launcher.java) and help is defined in parseCommonOption (Launcher.java)

I think we can take "some" control in our hands and set config.ShowHelp to LONG in CommandLineParser. I've changed it, updated PR, and now it's printing help as before.

If you would say – let's keep it, I will proceed with the next step – a new print method which will keep the logic around output, TTY check, process cleanup, etc, so WDYT?

[eregon]

Should explicitly do this here:

                    // add --help in unknown arguments so that runLauncherAction() actually prints the help
                    config.getUnknownArguments().add(argument);
                    break FOR;

because the fallthrough is quite subtle and far.

[Strech]

I've done it that way and changed, will return back good point 👍🏼

[eregon]

Yes, the current diff looks good. You're right, we cannot "eat" the --help we need it in unknown arguments as you said.

[Strech]

@eregon I'm very sorry, but I've made a mistake, the code I wrote never handles the LONG because it never reached it. The problem is the flow from the AbstractLanguageLauncher.java

In short it looks like that

    final void launch(List<String> args, Map<String, String> defaultOptions, boolean doNativeSetup) {
        // ... skipped ...

        parseUnrecognizedOptions(getLanguageId(), polyglotOptions, unrecognizedArgs); // <-- Here we parse options

        if (runLauncherAction()) { // <-- Here we will exit if we place `--help` in unrecognized args
            return;
        }

        // ... skipped ...

        launch(builder); // <-- here our RubyLauncher.launch is called
    }

As the result, I can either completely skip unrecognized options by simply not adding --help, but I lose runLauncherAction functionality, or I can't wrap around runLauncherAction and redirect output without actually overriding it, which we try to avoid.

I see 3 ideas about what we can do about it:

1. LanguageLauncherBase.java-way
   1. Override parseCommonOption to handle --help and set an internal flag that we need to show help
   2. Override runLauncherAction to handle pager
2. Simplified runLauncherAction-way
   1. Since we ate --help and know that it's a showHelp.LONG we call only methods necessary for the help output (we know that it's not internal, expert or VM help).
   2. New method will be something like this

      protected void printHelp() {
        OptionCategory hc = getHelpCategory();
      
        printDefaultHelp(hc);
        maybePrintAdditionalHelp(hc);
        printAllOtherHelpCategories(true);
      }

      Downsides will be to become out of sync with runLauncherAction content
3. Dirty-way
   1. Override 2 help methods printDefaultHelp and printAllOtherHelpCategories since they are the first and the last inside the runLauncherAction and handle the logic of pager + cleaning ... but it's ruining the whole idea of having better control over help printing.

[Strech]

@eregon Happy new Year 🎄

After a short break, I revisit this topic and I think I would like to go with option 1. The reason for that is a misleading name of runLauncherAction which is in fact printing version or help and not running any "action" other than that.

So I think overriding is the way to go.

[eregon]

I think overriding runLauncherAction() is OK for this.
I think we don't need to override parseCommonOption(), we can just remember we saw --help in org.truffleruby.launcher.CommandLineParser#processArgument which is called before.

So we'd leave --help in, but override runLauncherAction() to wrap it with the pager logic if --help was seen, and otherwise just call super.

[Strech]

I'm sorry for holding this task for a while in a draft state, I have some IRL events which took all my evening, but I hope it will be resolved soon and I can continue. If you feel blocked by this – just tell me 💛

[eregon]

@Strech It's fine, this is currently not blocking anything.
Of course it'd be great to wrap this PR up once you have time.
We could also have a call if needed, just ping me on Slack in that case.

[graalvmbot]

Hello Sergey Fedorov, thanks for contributing a PR to our project!

We use the Oracle Contributor Agreement to make the copyright of contributions clear. We don't have a record of you having signed this yet, based on your email address oni -(dot)- strech -(at)- gmail -(dot)- com. You can sign it at that link.

If you think you've already signed it, please comment below and we'll check.

[Strech]

@eregon Hola 👋🏼

After our discussion, I've removed obsolete code and added logic similar to MRI

When the environment variable RUBY_PAGER or PAGER is present and has
non-empty value, and the standard input and output are tty, --help
option shows the help message via the pager designated by the value.

I'm not sure about the quality of the Java code, so I would appreciate some feedback.

P.S Not sure why, but CRuby 2.6 tests are failing and I'm not sure how to test my changes 🤔

[eregon]

That looks good, but on jt clean cexts && jt build (locally, with $PAGER set) it opens a pager, that's definitely wrong.
Same on jt ruby -e0.
So I think we need to detect/remember if --help was passed, and only use the pager behavior in that case.

[Strech]

Oh, I've noticed that, but though that it was due to unfinished code. I'm going to add the check back and incorporate it into the run launcher action.

[eregon]

Thank you, all is good now, I pushed a few extra changes/cleanups.