-
Notifications
You must be signed in to change notification settings - Fork 510
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
rebar3 release doesn't parse --relnames arguments correctly when specifying a profile #2811
Comments
Sorry for the barebones report. I can add a couple of examples next week, but I think it's pretty easy to reproduce. |
does it work with |
It doesn't make any difference :( How big of a change that would be? Just curious. Maybe for next big release, at this point I'm not even concerned by the issue. |
I'm not 100% sure but my understanding is that this is due to how we implemented two providers: Chances are we could better cover some cases, but maybe not all of them, without breaking other functionality. As it turns out, the comment in there specifically warned of this issue: rebar3/apps/rebar/src/rebar_utils.erl Lines 253 to 255 in c46bec6
rebar3/apps/rebar/src/rebar_utils.erl Lines 871 to 883 in c46bec6
I'm guessing we could actually do some lookahead or lookbehind parsing without using regexes and deal with quoting better without breaking compatibility, but I'm not 100% sure. |
So I've been adding tests, and it appears that the parsing at least currently works fine with long form arguments, which cancel out the ambiguity -- calling with |
Yeah, I've just tried with long option and it works as long as using The only missing thing would be maybe fixing
|
The current mechanism had a few oddities, as reported in erlang#2811: - `rebar3 do a,b` runs `a` then `b` - `rebar3 as profile task --a=b,c` runs the task with the flag `a` set to `b,c` - `rebar3 as profile task -a b,c` runs the task with the flag `a` set to `b`, and then tries to run `c` as a task - `rebar3 as profile task -a b, c` runs the task with the flag `a` set to `b`, and then tries to run `c` as a task So the issue is that to pass a comma to an argument, we can only do so when it exists as a long form argument (`--flag`) and that it does not contain spaces. This patch attempts to correct things by making the white-space following the comma significant. If it's missing, it gets grouped as a single entity. If it's there, the task is considered to be split. This becomes significant for commands that internally parse the `,` to allow list of args (such as `rebar3 release --relnames one,two`) which were only invokable as `relnames=one,two`, and not the spaced version nor the short version (`m one,two`), which should now be supported. This should *not* break any backwards compatibility, but there is the possibility that users who did invoke many commands with both arguments and sequences without spaces (`rebar3 do release --relname a,tar`) now get broken commands. I don't quite know if we'd be okay taking that risk, so this is up for comments I guess.
See #2813 for a potential fix. Turns out we had a bunch of tests and I think it can work, but there are some backwards compat risks there that makes it possible we wouldn't include it even if it works there. We'll need to think about it. |
The current mechanism had a few oddities, as reported in erlang#2811: - `rebar3 do a,b` runs `a` then `b` - `rebar3 as profile task --a=b,c` runs the task with the flag `a` set to `b,c` - `rebar3 as profile task -a b,c` runs the task with the flag `a` set to `b`, and then tries to run `c` as a task - `rebar3 as profile task -a b, c` runs the task with the flag `a` set to `b`, and then tries to run `c` as a task So the issue is that to pass a comma to an argument, we can only do so when it exists as a long form argument (`--flag`) and that it does not contain spaces. This patch attempts to correct things by making the white-space following the comma significant. If it's missing, it gets grouped as a single entity. If it's there, the task is considered to be split. This becomes significant for commands that internally parse the `,` to allow list of args (such as `rebar3 release --relnames one,two`) which were only invokable as `relnames=one,two`, and not the spaced version nor the short version (`m one,two`), which should now be supported. This should *not* break any backwards compatibility, but there is the possibility that users who did invoke many commands with both arguments and sequences without spaces (`rebar3 do release --relname a,tar`) now get broken commands. I don't quite know if we'd be okay taking that risk, so this is up for comments I guess.
Environment
rebar3 3.22.0
Erlang/OTP 24
macOS Ventura
(but probably every other platform is affected)Current behaviour
Release building with
-m|--relnames
option doesn't work when a profile is explicitly set (see example above). Name after first,
is interpreted as a command. This behaviour is not observed when running on default profile.Expected behaviour
Comma separated list of releases should be parsed correctly.
The text was updated successfully, but these errors were encountered: