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

Make compiler aware of output extension when building programs #13370

Conversation

HertzDevil
Copy link
Contributor

@HertzDevil HertzDevil commented Apr 21, 2023

The compiler is currently unaware of the use of .exe when building a program on Windows, and incorrectly reports about existing files:

>echo 1 > foo
>mkdir bar
>crystal build foo
Error: compilation will overwrite source file 'foo', either change its extension to '.cr' or specify an output file with '-o'
>crystal build -obar foo 
Error: can't use `bar` as output filename because it's a directory

In the above example, it is always foo.exe or bar.exe that is being built, so the existence of a file called foo or bar should not generate an error.

This PR adds .exe right after the output filename is collected from the command-line arguments. The above builds will succeed, whereas files called foo.exe or bar.exe will now block the builds:

>echo 1 > foo.exe
>mkdir bar.exe
>crystal build foo.exe
Error: compilation will overwrite source file 'foo.exe', either change its extension to '.cr' or specify an output file with '-o'
>crystal build -obar foo.exe
Error: can't use `bar.exe` as output filename because it's a directory
>crystal build -obar.exe foo.exe
Error: can't use `bar.exe` as output filename because it's a directory

30f656c extends this to object files on all platforms.

Copy link
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO the code for determining the output filename gets quite confusing. Maybe it could use some refactoring?
For example, the calculation of original_output_filename is completely unnecessary if an explicit output file is given.

src/compiler/crystal/command.cr Outdated Show resolved Hide resolved
src/compiler/crystal/command.cr Outdated Show resolved Hide resolved
src/compiler/crystal/command.cr Outdated Show resolved Hide resolved
@HertzDevil HertzDevil marked this pull request as draft April 24, 2023 10:23
@HertzDevil
Copy link
Contributor Author

HertzDevil commented Apr 24, 2023

This PR now adds .exe to all the object files even when cross-compiling. And it reveals that cross-compilation has the same issue on other platforms, because they too automatically add an extension:

# on x86_64-linux-gnu
$ echo 1 > foo
$ mkdir bar
$ crystal build --cross-compile --target=x86_64-linux-gnu foo
Error: compilation will overwrite source file 'foo', either change its extension to '.cr' or specify an output file with '-o'
$ crystal build --cross-compile --target=x86_64-linux-gnu -obar foo
Error: can't use `bar` as output filename because it's a directory

Neither build should produce an error because foo.o or bar.o does not exist. (On Windows or WebAssembly this would be .obj or .wasm instead.)

@HertzDevil HertzDevil marked this pull request as ready for review May 3, 2023 14:12
@HertzDevil HertzDevil changed the title Make compiler aware of .exe extension when building programs Make compiler aware of output extension when building programs May 3, 2023
@straight-shoota straight-shoota added this to the 1.9.0 milestone May 23, 2023
@straight-shoota straight-shoota merged commit 0627b47 into crystal-lang:master May 25, 2023
46 checks passed
@HertzDevil HertzDevil deleted the bug/compiler-output-filename-exe branch May 25, 2023 13:31
AbrilRBS pushed a commit to compiler-explorer/compiler-explorer that referenced this pull request Oct 17, 2023
Crystal 1.9+ has made the `.s` and `.ll` files follow the base name of
the output rather than the input (I think this is due to
crystal-lang/crystal#13370), this PR matches the Crystal compiler's
behavior
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants