-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Add error handling to compiler when linker is unavailable #12899
Add error handling to compiler when linker is unavailable #12899
Conversation
src/compiler/crystal/compiler.cr
Outdated
Process.run(command, args, shell: true, | ||
input: Process::Redirect::Close, output: Process::Redirect::Inherit, error: Process::Redirect::Pipe) do |process| | ||
process.error.each_line(chomp: false) do |line| | ||
next if line.matches?(/^--: 1: .*: not found$/) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we avoid skipping lines if --verbose is used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. This is definitely one of the more fuzzy aspects. I suppose it should also only skip the first line, not if it appears later in the output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't follow why only the first line. I would avoid filtering entirely on --verbose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First line would be independent of verbose.
next if line.matches?(/^--: 1: .*: not found$/) | |
next if first_line && !verbose && line.matches?(/^--: 1: .*: not found$/) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The filter is supposed to remove the "command not found" string? The zsh: command not found: xxxx
? Or is it something else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's the idea. But I feel like maybe it's better to remove that. At least for now. It's a convenience feature, but not really necessary. And it's not super accurate in any case.
else | ||
link_flags = @link_flags || "" | ||
link_flags += " -rdynamic" | ||
|
||
{ %(#{CC} "${@}" -o #{Process.quote_posix(output_filename)} #{link_flags} #{program.lib_flags}), object_names } | ||
{CC, %(#{CC} "${@}" -o #{Process.quote_posix(output_filename)} #{link_flags} #{program.lib_flags}), object_names} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The actual executable could be different from CC
if the environment variable is something like FOO=BAR gcc
. But I don't have a good solution for that because parse_arguments
is not sufficient for arbitrary shell commands
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think that's good enough. And definitely an improvement over printing the full linker command line which can easily span multiple pages.
I suppose if we had a good solution for that, we also wouldn't need to run Process.run(shell: true)
through /bin/sh
🤷 🤣
0157e60
to
b4366be
Compare
Rebased on top of https://github.com/crystal-lang/crystal/tree/release/1.7 for merging into 1.7.2. That required a force push. |
Fixes #12839
The first two commits are simply refactoring to drop the
process_wrapper
helper method which is unnecessary and only complicates control flow.The third commit introduces a short name in
linker_command
which refers to the command itself without the additional autogenerated arguments (which can be quite a lot).Alternatively, we could also use
parse_arguments
to extract the name from the final command. But the original value is already available anyway, and it's easy to just forward it.The fourth commit then implements the actual error handling. Due to #12873 it covers errors indicated by exceptions as well as exit status.