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: build --no-codegen output file name error #14239

Conversation

apainintheneck
Copy link
Contributor

This avoids errors based on output file name when the --no-codegen command is passed to crystal build since it shouldn't be necessary to specify an output file if no file will be created anyway.

$ crystal build --no-codegen  example/example.cr
Error: can't use `example` as output filename because it's a directory

This change means that the following lines of code get skipped when using --no-codegen.

# Check if we'll overwrite the main source file
if !no_codegen && !run && first_filename == File.expand_path(output_filename)
error "compilation will overwrite source file '#{Crystal.relative_filename(first_filename)}', either change its extension to '.cr' or specify an output file with '-o'"
end

if !no_codegen && !run && Dir.exists?(output_filename)
error "can't use `#{output_filename}` as output filename because it's a directory"
end

It will also means that we skip the combine rpaths step.

combine_rpath = run && !no_codegen

This avoids errors based on output file name when the `--no-codegen`
command is passed to `crystal build` since it shouldn't be necessary
to specify an output file if no file will be created anyway.

Example:

$ crystal build --no-codegen  example/example.cr
Error: can't use `example` as output filename because it's a directory
@apainintheneck apainintheneck marked this pull request as ready for review January 15, 2024 02:28
@apainintheneck apainintheneck changed the title fix: build --no-codegen output file name warning fix: build --no-codegen output file name error Jan 15, 2024
@straight-shoota
Copy link
Member

straight-shoota commented Jan 15, 2024

Hey @apainintheneck, thanks for picking this up! 👋

This PR looks all right, as in it should probably work. But I'm a bit concerned about the semantics and wondering if there couldn't be a better solution.
The reason is that no_codegen is an input parameter and it feels a bit sketchy to change its value somewhere down the line. It will probably work fine because all the relevant code that uses it as the original input parameter is in the initial setup that and that has already executed at this point. So it's not used for that anymore.

Yet, wie have some duplication with compiler.no_codegen which contains the configuration value. So I'm thinking maybe instead we could replace all references to no_codegen by compiler.no_codegen in the subsequent code after the option parser setup.
And set compiler.no_codegen = no_codegen at the top of the method.
Then there would be a clear separation: no_codegen is only responsible for the CLI configuration. compiler.no_codegen determines if codegen is actually enabled in the compiler.

Does this make sense to you?

@apainintheneck
Copy link
Contributor Author

Yet, wie have some duplication with compiler.no_codegen which contains the configuration value. So I'm thinking maybe instead we could replace all references to no_codegen by compiler.no_codegen in the subsequent code after the option parser setup. And set compiler.no_codegen = no_codegen at the top of the method. Then there would be a clear separation: no_codegen is only responsible for the CLI configuration. compiler.no_codegen determines if codegen is actually enabled in the compiler.

Does this make sense to you?

This makes sense to me. It shouldn't change semantics like you said but it will make the code easier to reason about.

The idea here is to make the `#create_compiler` method easier to reason
about by splitting up the responsibilities of the `no_codegen` input
parameter which is used when building the options parser and the
`compiler.no_codegen?` boolean value which is used when deciding how to
compile the program.
@straight-shoota straight-shoota added this to the 1.12.0 milestone Feb 1, 2024
@straight-shoota straight-shoota merged commit 3def4de into crystal-lang:master Feb 2, 2024
57 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants