Custom rebar3 shells cannot pass args when using Erlang 26

[oubiwann]

Pre-Check

@ferd and I have been discussing this for a few days in a Slack thread:

• https://erlanger.slack.com/archives/C0SEMDD36/p1691071778823049

Environment

• Erlang: 26.0.2
• LFE: 2.1.2
• rebar3: 3.22.1
• rebar3_lfe: 0.4.6+

Current behaviour

With Erlang 26, rebar3 uses erlang:function_exported(shell, start_interactive, 0) to determine whether to execute the old work-around for the user_drv startup issues (ownership, etc.). The Erlang 26 code path skips the user_drv setup and instead calls the new shell:start_interactive function. However, rebar3_prv_shell only uses the arity-0 form of this function and thus any args for custom shells are do not get passed. As such, the LFE rebar3 plugin cannot run the LFE REPL as a custom shell in Erlang 26. Instead, they get the default with no args: the Erlang shell.

For Erlang 25 and earlier, example custom shell plugin support is utilised with something the following:

            ShellArgs = [{shell_args, ['tty_sl -c -e',{lfe_shell,start,[AdditionalArgs]}]}],
            State1 = rebar_state:set(State, shell, ShellArgs),
            rebar_prv_shell:do(State1),

Expected behaviour

Users of the rebar3_lfe plugin should be able to execute rebar3 lfe repl and have the LFE REPL start up. As a plugin developer, I should be able write a plugin that lets users start a custom shell with something like the following:

            ShellArgs = [{shell_args, [{lfe_shell,start,[AdditionalArgs]}]}],
            State1 = rebar_state:set(State, shell, ShellArgs),
            rebar_prv_shell:do(State1),

When rebar_prv_shell:setup_shell detects the interactive shell for Erlang 26, it should call rebar_prv_shell:start_interactive but it should also pass ShellArgs if those have been defined.

Contributing

I am more than happy to contribute a small patch to fix this issue. I've hacked a couple different workarounds in rebar3_lfe, with the most recent being the cleanest (thanks entirely to pointers from @rvirding).

I do have a stylistic preference question for you, though:

• the fix for this can be implemented in an almost pass-through fashion with very little extra code by calling apply(shell, start_interactive, ShellArgs) (thus taking advantage of the multiple arities defined for that Erlang 26 function). Is this okay?
• or, would you prefer explicit calls to the different arities of shell:start_interactive?

In lieu of guidance on team preference, I can start with the former, since that's the least code; if there are objections, I can easily switch to the latter.

Lastly, I'd like to update the docs to make things a little more clear for plugin developers writing custom shells, explaining the allowed/expected args, and why. Not sure where to look for that, yet -- any time-saving pointers are deeply appreciated!

Tasks:

• Patch rebar_prv_shell
• Discuss approach with team
• Update docs

[oubiwann]

As for docs, it looks like I would need to update these:

• The user_drv_args entry in rebar_prv_shell's opts
• https://github.com/tsloughter/rebar3.org/blob/master/content/en/docs/commands.md

[ferd]

Yeah looking at it, and since the default shell_args argument is [], the apply/3 form might work fine. The doc does need updating, and if you're submitting a PR, I'll merge it ASAP since it seems the args had been changed a while ago.

The only risk with the apply/3 form is ending up with an undef error, which would mean the list of arguments given was invalid. The other slight risk there is that it's possible at some point the two formats for ShellArgs could start changing between versions, and keeping compatibility would be a challenge, all pushed onto plugin writers.

If the latter risk seems acceptable to you (the target audience for this option set) then I think we're good with the proposed implementation.

[oubiwann]

Nice -- Thanks!

I'll update the docs and take the PR out of draft.

[oubiwann]

Btw, the GH Actions CI/CD that need to run for this PR, only the most recent should be run -- that's the one that has the update for the dialyzer ignore (arity change).

[ferd]

Yep, sorry. I've just manually allowed the run, always manual mode until a contrib is merged apparently.

[ferd]

I merged it, but I'm just noticing now that I'm looking at the doc PR that we fetch the config from shell_args but allow the config value to be user_drv_args How are we actually wiring these two arguments together? How has this ever worked?

Oh I guess one is the CLI option and one is the state option?

[oubiwann]

Oh I guess one is the CLI option and one is the state option?

That is my understanding, yes. (this trips me up every couple of years when I dive into this part of the code ...)