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

Remove static linking for macos #6110

Closed
wants to merge 12 commits into from
Closed

Conversation

j8r
Copy link
Contributor

@j8r j8r commented May 20, 2018

@@ -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"
Copy link
Contributor

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.

@j8r j8r force-pushed the macos-static branch 2 times, most recently from 46e77fe to a56b574 Compare May 20, 2018 16:23
@straight-shoota
Copy link
Member

Maybe this should also include a reference to get more information on this limitation?

@@ -411,7 +411,11 @@ class Crystal::Command
compiler.verbose = true
end
opts.on("--static", "Link statically") do
compiler.static = true
{% if flag?(:darwin) %}
Copy link
Contributor

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 %}

Copy link
Member

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?

# 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"
Copy link
Member

Choose a reason for hiding this comment

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

information

Copy link
Contributor

@faustinoaq faustinoaq left a comment

Choose a reason for hiding this comment

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

👍

@wooster0
Copy link
Contributor

wooster0 commented May 28, 2018

Actually the error would look like this right now:

macOS doesn't support static linking.For more information: https://developer.apple.com/library/content/qa/qa1118/_index.html (Exception)
  from ???
  from /usr/share/crystal/src/crystal/system/unix/getrandom.cr:35:7 in '__crystal_main'
  from /usr/share/crystal/src/crystal/main.cr:0:3 in 'main'
  from __libc_start_main
  from _start
  from ???

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 %}

compiler.static = true
# https://developer.apple.com/library/content/qa/qa1118/_index.html
{% if flag?(:darwin) %}
raise "macOS doesn't support static linking.\
Copy link
Member

Choose a reason for hiding this comment

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

abort "..."

Copy link
Contributor

@RX14 RX14 left a 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.

@faustinoaq
Copy link
Contributor

faustinoaq commented May 29, 2018

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?

@j8r
Copy link
Contributor Author

j8r commented May 29, 2018

We can put it in a rescue block that prints this info when the linking fails on macOS?

compiler.static = true
# https://developer.apple.com/library/content/qa/qa1118/_index.html
{% if flag?(:darwin) %}
abort <<-E
Copy link
Contributor

@wooster0 wooster0 Jun 2, 2018

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.

@bew
Copy link
Contributor

bew commented Jun 4, 2018

Note: failed spec is unrelated (cannot allocate memory)

{% if flag?(:darwin) %}
STDERR.puts <<-INFO
macOS doesn't support static linking.
For more information: https://developer.apple.com/library/content/qa/qa1118/_ind
Copy link
Member

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

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.

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.

@j8r
Copy link
Contributor Author

j8r commented Jun 23, 2018

I think we can return a custom exception when the linking fails like Crystal::LinkingError, and thus put this macro on the appropriate new rescue block instead of Crystal::Exception.
I have found this line - not really sure how to do this properly. Put the linking errors in a variable instead of STDERR, and raise an exception if the variable isn't empty at the end?
The bad point is I haven't a mac to test it.

@j8r
Copy link
Contributor Author

j8r commented Jun 23, 2018

nvm let's just print an information when using --static on macOS, whether the linking fails or not after.

@Sija
Copy link
Contributor

Sija commented Jun 24, 2018

@j8r it's still inside the rescue block

macOS doesn't officially support static linking.
For more information: https://developer.apple.com/library/content/qa/qa1118/_index.html
INFO
{% end %
Copy link
Contributor

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.

@@ -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
Copy link
Contributor

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.

@@ -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
Copy link
Contributor

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?

@straight-shoota
Copy link
Member

A welcome alternative to compiler notifications would be expanding the pretty much meaningless wiki page on Static Linking.

@asterite
Copy link
Member

Can't we remove static linking altogether? Is anyone statically linking everything, like libpq, gc, pcre, openssl, etc., and using that single executable anywhere?

@straight-shoota
Copy link
Member

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 don't know much about actual uses, but the compiler itself is statically linked on linux x64 (allthough it's more complicated than --static).

@foi
Copy link

foi commented Jul 13, 2018

I prefer static because deployment

@asterite
Copy link
Member

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"

@straight-shoota
Copy link
Member

@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.

@RX14
Copy link
Contributor

RX14 commented Jul 13, 2018

@asterite I added the --static flag explicitly because I needed it to statically link the crystal compiler that we currently distribute in all the linux packages...

It's only useful on alpine though (not true, but thats what we need to convince users). We just need to document that.

@asterite
Copy link
Member

Maybe change it to --static-that-only-works-on-apline-not-true-but-thats-what-we-need-to-convince-you?

@RX14
Copy link
Contributor

RX14 commented Jul 13, 2018

or more realistically, -Dstatic.

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 --help and just fix the docs to not mention it.

@straight-shoota
Copy link
Member

@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.

@RX14
Copy link
Contributor

RX14 commented Jul 16, 2018

@j8r
Copy link
Contributor Author

j8r commented Jul 22, 2018

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 --static.

@RX14
Copy link
Contributor

RX14 commented Jul 23, 2018

We should close this now the doc fix has been merged. If we see more issues, we can reopen.

@j8r j8r closed this Jul 23, 2018
@j8r j8r deleted the macos-static branch October 16, 2018 08:43
@NightMachinery
Copy link

Was this added to the crystal binary? It's not enough for it to be in the docs, the binary should print an error itself. The whole point is to avoid a useless websearch.

@straight-shoota
Copy link
Member

But there is no error. You can use --static to link static libraries on macOS. There are no static versions for some system libraries like libc available. But that doesn't stop you from linking other libraries statically. You just can't get a fully statically linked binary that way.

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.

Static compilation of empty project fails on macos (OSX)