Skip to content

fix: build --no-codegen output file name error#14239

Merged
straight-shoota merged 2 commits intocrystal-lang:masterfrom
apainintheneck:fix-no-codegen-build-warning
Feb 2, 2024
Merged

fix: build --no-codegen output file name error#14239
straight-shoota merged 2 commits intocrystal-lang:masterfrom
apainintheneck:fix-no-codegen-build-warning

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
@Blacksmoke16 Blacksmoke16 added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:compiler labels 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:compiler:cli

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants