Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for rebar3 shell in Erlang 26 when ShellArgs==undefined #2819

Merged
merged 2 commits into from
Aug 13, 2023

Conversation

choptastic
Copy link
Contributor

Heh folks!

It seems that #2818 introduced a bug when there are no shell args defined (with Erlang 26), since the default value of ShellArgs in rebar_prv_shell is undefined.

This small fix just handles that situation, since undefined is not a valid value for the 3rd argument of erlang:apply/3.

Thanks!

@ferd
Copy link
Collaborator

ferd commented Aug 13, 2023

Hm, I think the better fix would be on line 136 by replacing undefined by []:

ShellArgs = debug_get_value(shell_args, rebar_state:get(State, shell, []), undefined,

I missed it on review because I saw the shell default as [] and forgot there was an extra check in there.

@choptastic
Copy link
Contributor Author

Thanks, I thought about that as an option as well, but https://github.com/erlang/rebar3/blob/main/apps/rebar/src/rebar_prv_shell.erl#L229 also has as separate check for undefined and I wasn't sure if there would be further cascading regressions for whichever versions of Erlang, so I went with the easy route.

The solution there would be to change that line as well to matching against [] as well, eh?

@ferd
Copy link
Collaborator

ferd commented Aug 13, 2023

Yeah I think so, it would make the whole thing more aligned in terms of types.

@choptastic
Copy link
Contributor Author

Thanks, I've updated the PR. Hopefully it's all good.

@ferd
Copy link
Collaborator

ferd commented Aug 13, 2023

Looks good, thanks! will merge as soon as CI passes, which should be unrelated to this since we don't have tests for the shell.

@ferd ferd merged commit 6d026e4 into erlang:main Aug 13, 2023
6 checks passed
@choptastic
Copy link
Contributor Author

Thanks @ferd!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants