-
-
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
Remove static linking for macos #6110
Conversation
src/compiler/crystal/command.cr
Outdated
@@ -411,7 +411,11 @@ class Crystal::Command | |||
compiler.verbose = true | |||
end | |||
opts.on("--static", "Link statically") do | |||
{% if flag?(:darwin) %} | |||
raise "Macos doesn't support static linking" |
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.
Please, adjust indentation (ditto below) + use canonical form macOS
.
46e77fe
to
a56b574
Compare
Maybe this should also include a reference to get more information on this limitation? |
src/compiler/crystal/command.cr
Outdated
@@ -411,7 +411,11 @@ class Crystal::Command | |||
compiler.verbose = true | |||
end | |||
opts.on("--static", "Link statically") do | |||
compiler.static = true | |||
{% if flag?(:darwin) %} |
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 think we can add source link here:
# https://developer.apple.com/library/content/qa/qa1118/_index.html
{% if flag?(:darwin) %}
raise "macOS doesn't support static linking"
{% else %}
compiler.static = true
{% end %}
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.
Perhaps even include the link in the error output?
src/compiler/crystal/command.cr
Outdated
# https://developer.apple.com/library/content/qa/qa1118/_index.html | ||
{% if flag?(:darwin) %} | ||
raise "macOS doesn't support static linking.\ | ||
For more informations: https://developer.apple.com/library/content/qa/qa1118/_index.html" |
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.
information
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.
👍
Actually the error would look like this right now:
I think it should be this: {% if flag?(:darwin) %}
abort "macOS doesn't support static linking. For more information see: https://developer.apple.com/library/content/qa/qa1118/_index.html"
{% else %}
compiler.static = true
{% end %} |
src/compiler/crystal/command.cr
Outdated
compiler.static = true | ||
# https://developer.apple.com/library/content/qa/qa1118/_index.html | ||
{% if flag?(:darwin) %} | ||
raise "macOS doesn't support static linking.\ |
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.
abort "..."
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.
We should check the actual architecture the compiler is targeting instead of use macro flags (i.e. the build architecture of the compiler).
And I don't like the idea of flat-out forbidding people to static link on macos. It seems possible to me to create a toolchain on macos which is capable of static linking.
@RX14 What about a warning? |
We can put it in a |
src/compiler/crystal/command.cr
Outdated
compiler.static = true | ||
# https://developer.apple.com/library/content/qa/qa1118/_index.html | ||
{% if flag?(:darwin) %} | ||
abort <<-E |
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.
Why naming it "E"? "INFO" or "ERROR" would be better I think.
Note: failed spec is unrelated (cannot allocate memory) |
src/compiler/crystal/command.cr
Outdated
{% if flag?(:darwin) %} | ||
STDERR.puts <<-INFO | ||
macOS doesn't support static linking. | ||
For more information: https://developer.apple.com/library/content/qa/qa1118/_ind |
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 be https://developer.apple.com/library/archive/qa/qa1118/_index.html
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 condition and placement is completely off. Currently, the error message would be printed when any Crystal::Exception
is raised when running on darwin.
The message should only be printed when targetting darwin and the --static
command line flag is present.
I think we can return a custom exception when the linking fails like |
nvm let's just print an information when using |
@j8r it's still inside the |
src/compiler/crystal/command.cr
Outdated
macOS doesn't officially support static linking. | ||
For more information: https://developer.apple.com/library/content/qa/qa1118/_index.html | ||
INFO | ||
{% end % |
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.
Theres a bracket missing here.
src/compiler/crystal/command.cr
Outdated
@@ -412,6 +412,13 @@ class Crystal::Command | |||
end | |||
opts.on("--static", "Link statically") do | |||
compiler.static = true | |||
# https://developer.apple.com/library/content/qa/qa1118/_index.html | |||
{% if flag?(:darwin) %} | |||
STDERR.puts <<-INFO |
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.
This should be STDOUT
since it's not an error.
src/compiler/crystal/command.cr
Outdated
@@ -408,6 +408,13 @@ class Crystal::Command | |||
end | |||
opts.on("--static", "Link statically") do | |||
compiler.static = true | |||
# https://developer.apple.com/library/content/qa/qa1118/_index.html |
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.
Since the message to the terminal contains the link to Apple's website do you need this comment?
A welcome alternative to compiler notifications would be expanding the pretty much meaningless wiki page on Static Linking. |
Can't we remove static linking altogether? Is anyone statically linking everything, like libpq, gc, pcre, openssl, etc., and using that single executable anywhere? |
Some people prefer statically linked binaries. It can have benefits over dynamic libraries. And it's not even always about linking everything statically, maybe just some dependencies that are tricky to get the right version, or at all on the target platform etc. Obviously, it has it's pros and cons, but there are strong use cases. The only trouble is, that it doesn't really work on most platforms... But it does work on Alpine Linux. Why remove that? I don' think there is any real benefit from that. I really think that compiler feature should stay. People are often asking about it, so there is really some interest. |
I prefer static because deployment |
Can we output a similar comment to the one said by @straight-shoota when compiling statically, independently of the platform? I feel like everyone is having trouble with static compilation at least once or twice a month. That way it's like "look, unless you are in this one OS, it's probably not going to work" |
@asterite Yes, but I'm even more convinced this should really be documented first (see #6110 (comment)). Then we can discuss if there should also be a compiler message. That's the order of priorities because a) documentation change propagates instantly, b) maybe proper documentation is all people are missing and c) a compiler message should point to the docs for more information. |
@asterite I added the It's only useful on alpine though (not true, but thats what we need to convince users). We just need to document that. |
Maybe change it to |
or more realistically, Or just stop documenting it at all in the docs. I don't know why we did because so few people should be using it. There's a reason that it was Go that popularized static linking, and no other language before it. And the reason is that only Go makes static linking sane to do, because they used all their googlebucks to completely ignore the last~30 years of unix history and rewrite from scratch (actually plan⑨ toolchain but nobody knows what that is). Lets bury this option in |
@RX14 I don't think removing documentation is a wise choice. People will find it anyway on other websites etc. So it better should be properly explained what the limitations are and how it can be used. |
Printing more information to the STDOUT doens't hurt, but do we print a link to the Crystal's wiki, Crystal's docs or the official Apple docs? Another option is to print a link to https://github.com/crystal-lang/crystal/wiki/Static-Linking or https://crystal-lang.org/docs/using_the_compiler/#creating-a-statically-linked-executable whenever we use |
We should close this now the doc fix has been merged. If we see more issues, we can reopen. |
Was this added to the |
But there is no error. You can use |
Static linking isn't supported on macos.
Fix #5991