Resolve string vs list crashing rebar3 #1807
Conversation
src/rebar_erlc_compiler.erl
Outdated
| @@ -796,3 +797,10 @@ dir_recursive(Opts, Dir, CompileOpts) when is_list(CompileOpts) -> | |||
| undefined -> rebar_dir:recursive(Opts, Dir); | |||
| Recursive -> Recursive | |||
| end. | |||
|
|
|||
| valid_erl_first_conf(FileList) when not is_list(hd(FileList)) -> | |||
| throw("An invalid file list was provided as part of your erl_files_first directive"); | |||
There was a problem hiding this comment.
check out the ?ABORT("...", [...]) macro for such usages
|
@ferd I wonder if there are more cases where this applies? If there are, then perhaps this can be made a little more generic name wise... it might also make better since to validate in rebar_opts if that is the case. |
|
There are possibly more cases. I guess the most generic we could make it for now is a call like |
starbelly
force-pushed
the
1645-erl_files_first_string_crashes_rebar3
branch
from
d1ff177 to
1b0d28a
Compare
src/rebar_erlc_compiler.erl
Outdated
|
|
||
| valid_erl_first_conf(FileList) -> | ||
| case FileList of | ||
| [] -> ?WARN("An empty file list (~p) was provided as part of your erl_files_first directive", [FileList]); |
There was a problem hiding this comment.
I don't know that an empty file is warning-worthy since it should be equivalent to not submitting anything. Does it break stuff?
There was a problem hiding this comment.
No this does not break stuff, will remove that. I locked on to the problem of trying to handle "" :)
starbelly
force-pushed
the
1645-erl_files_first_string_crashes_rebar3
branch
2 times, most recently
from
6a7fdff to
fa71427
Compare
starbelly
changed the title
|
@ferd This should be good to go. |
starbelly
force-pushed
the
1645-erl_files_first_string_crashes_rebar3
branch
from
fa71427 to
d1fc937
Compare
|
@RumataEstor Never mind I reproduced. |
|
If supplying atoms in this list is valid, I have a quick fix, but I will wait to hear from @ferd or @tsloughter |
|
@starbelly I'm not sure if it's supposed to work, just noticed in case the breakage wasn't intentional. I'm fine already ;-) |
|
I don't think it should be valid, as even rebar 2.x appears to have no code in use to support it. However, most of the file handling functions seem to accept atoms, and so it worked kind of accidentally since we never did much to validate these entries. We can possibly support atoms but with a warning. Something like "erl_first_files only expects lists of filenames as strings. The module name |
|
Noting for history that a check for the existence of atoms in erl_first_files was added. We filter out atoms from the list and perform string validation on only strings, but we do give a warning about the use of module names (atoms) in this config directive. See #1828 for details. |
This PR resolves #1645.
To be clear we can not abort if
""is given, because it may be[]which is valid. Neither result in a fatal though, so it's a non-issue.TODO:
"abc"