Skip to content

Add github action rules for static binaries generation (Linux, Macos) #391

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

Merged
merged 16 commits into from
Jul 23, 2021

Conversation

AltGr
Copy link
Collaborator

@AltGr AltGr commented May 6, 2021

Note: this PR supersedes #388.

@AltGr
Copy link
Collaborator Author

AltGr commented May 6, 2021

Aforementioned binaries can be downloaded, in the examples of this run, here: https://github.com/ocaml-sf/learn-ocaml/actions/runs/817393844

Testers welcome!

(note that this might be turned on only for master and tags, but I wanted to demonstrate the PR first :) )

@erikmd
Copy link
Member

erikmd commented May 13, 2021

Hi @AltGr, thanks a lot for these two PRs 👍

I'll test the generated binaries ASAP − at least the Linux ones − and let you know!

@erikmd erikmd added the kind: feature New user-facing feature. label May 14, 2021
@AltGr AltGr mentioned this pull request Jun 1, 2021
AltGr added 5 commits July 16, 2021 15:05
To enable: `ln -s static-linking.sexp src/main/linking.sexp`

This still links dynamically to glibc and pthreads, but should already
considerably improve portability. Next to do is use musl instead of glibc.

Before:
```
% ldd _build/default/src/main/learnocaml_server_main.exe
        linux-vdso.so.1 (0x00007fff743ee000)
        libssl.so.1.1 => /usr/lib/x86_64-linux-gnu/libssl.so.1.1 (0x00007f29aeb43000)
        libcrypto.so.1.1 => /usr/lib/x86_64-linux-gnu/libcrypto.so.1.1 (0x00007f29ae84f000)
        libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00007f29ae82d000)
        libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007f29ae6e9000)
        libdl.so.2 => /lib/x86_64-linux-gnu/libdl.so.2 (0x00007f29ae6e3000)
        libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007f29ae51e000)
        /lib64/ld-linux-x86-64.so.2 (0x00007f29af80e000)
```
After:
```
% ldd _build/default/src/main/learnocaml_server_main.exe
        linux-vdso.so.1 (0x00007ffcdddd5000)
        libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00007f90e8f0e000)
        libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007f90e8dca000)
        libdl.so.2 => /lib/x86_64-linux-gnu/libdl.so.2 (0x00007f90e8dc4000)
        libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007f90e8bff000)
        /lib64/ld-linux-x86-64.so.2 (0x00007f90e9e50000)
```
Dune 2 is not compatible with OCaml 4.05 which we depend on at this stage; there
is the hack to still have it with compiling a secondary, more recent OCaml on
the switch but I really like *not* to do that. And dune 1 seems to work fine.
`make static-binaries` will generate `learnocaml`, `learnocaml-client` and
`learnocaml-server` in at the project root
Linking in the client docker builds seem to fail with the most recent dune
version...
@AltGr AltGr force-pushed the gh-act-static-bin branch from 9a93dab to 0222a4a Compare July 16, 2021 13:28
@AltGr
Copy link
Collaborator Author

AltGr commented Jul 16, 2021

Updated to use the env, thanks @gasche for the suggestion

…round

Thanks @gasche for the hint about this hidden dune feature :)
@AltGr AltGr force-pushed the gh-act-static-bin branch from 0222a4a to 1150bfa Compare July 16, 2021 14:59
Copy link
Member

@erikmd erikmd left a comment

Choose a reason for hiding this comment

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

Hi @AltGr, thanks a lot for this work! (and sorry for the delay… I was quite swamped with teaching this term :-/)

FYI, I've tested successfully the last version of the artifact with:

  • GNU/Linux Debian 10 amd64, and
  • macOS 11.1, thanks to the help of a colleague of mine.

Apart from minor comments I've left in the review (regarding documentation and bash details), I've identified three points that could be improved, but not necessarily within this PR:

  1. The GHA artifacts don't seem to be public (I believe I am able to download them only because I am collaborator of the repo) and according to the docs, they are removed after 90 days, so the way to go would be to rely on the GHA https://github.com/softprops/action-gh-release (mentioned by the legacy GHA https://github.com/actions/upload-release-asset)

  2. For the sake of CI, to detect regressions (namely, if a dependency is missing, and this missing dependency couldn't not be detected without running the binary after compilation), I believe it would be nice to add one job that fetches the artifact using actions/download-artifact@v2 and runs, say, learn-ocaml-client --version; WDYT? and do you believe running the --version command would suffice to detect missing deps?

  3. For MacOS, my colleague got some warning Impossible d’ouvrir « … » car le développeur ne peut pas être vérifié., as the binary is not signed :-|
    however it was fine after running xattr -d com.apple.quarantine learn-ocaml-client as suggested here, for example. As neither @yurug nor me have a macOS, I guess it'd be difficult to sign the binaries for the time being, so I'd propose that we document that the macOS binaries are not signed but can be enabled this way with xattr

@AltGr, as the points 1. and 2. above are just related to GHA and can be addressed in another PR (and the point 3. has a workaround), I can propose to look after 1. and 2. myself in another PR if you want; in order to merge this one as soon as you have taken a look at my other few line-comments.

Comment on lines +39 to +48
# Need unreleased 2.1.0~rc
# - name: Retrieve opam
# run: |
# mkdir "$HOME/bin"
# wget https://github.com/ocaml/opam/releases/download/2.1.0-beta2/opam-2.1.0-beta2-x86_64-macos -O $HOME/bin/opam
# chmod a+x $HOME/bin/opam
# echo "$HOME/bin" >> $GITHUB_PATH
- name: Install latest opam
run: |
brew install opam --HEAD
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a comment somewhere recalling the feature (or PR) that is needed here, and not available in opam 2.0.x?

'tar x >&2 &&
sudo apk add openssl-libs-static >&2 &&
opam switch create . ocaml-system "dune<2" --deps-only >&2 &&
opam exec make LINKING_MODE=static >&2 &&
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick (if you don't disagree):

Suggested change
opam exec make LINKING_MODE=static >&2 &&
opam exec -- make LINKING_MODE=static >&2 &&

@erikmd
Copy link
Member

erikmd commented Jul 19, 2021

The GHA artifacts don't seem to be public … and according to the docs, they are removed after 90 days.

Actually this is customizable, but necessarily <= 90 days:
2021-07-19_21-03-09_Screenshot_Artifacts_Logs

Co-authored-by: Erik Martin-Dorel <[email protected]>
@AltGr
Copy link
Collaborator Author

AltGr commented Jul 22, 2021

Feel free to finalise or clarify some comments if you think it's needed before merging; for me it's good to go.

erikmd added 3 commits July 22, 2021 18:53
* For each OS, run "file", "ldd" or so and "_ --version"

* No need for "actions/download-artifact@v2"
@erikmd
Copy link
Member

erikmd commented Jul 22, 2021

@AltGr (Cc @gasche) just FTR, here is a recap of this PR status and of the few commits I've just pushed, following this Thursday's video meeting with Louis.

First, to follow-up my earlier comment #391 (review) :

  1. The GHA artifacts don't seem to be public (I believe I am able to download them only because I am collaborator of the repo) and according to the docs, they are removed after 90 days

→ indeed, we could/should rely on https://github.com/softprops/action-gh-release
I'll take care of this before the upcoming release.

  1. For the sake of CI, to detect regressions (namely, if a dependency is missing, and this missing dependency couldn't not be detected without running the binary after compilation), I believe it would be nice to add one job that fetches the artifact using actions/download-artifact@v2 and runs, say, learn-ocaml-client --version

Actually it seems unneeded to use actions/download-artifact@v2 because running directly learn-ocaml-client --version in the toplevel context within the GHA runner ensures we check the "portability" of the static binary: compiled in alpine (musl-based), then run in Ubuntu Linux.

Anyway I've added a few unit tests, running not only ldd learn-ocaml-client … but also file learn-ocaml-client … and learn-ocaml-client --version (likewise for the two other binaries).

Then, I took the opportunity of this PR to automate what Louis had documented in src/main/linking_flags.sh: there is a new Makefile target that prints this 🙂

$ make detect-libs  # run on Debian GNU/Linux 10 amd64
...
learnocaml_client: -lcamlstr -lcstruct_stubs -lbase_stubs -lssl_threads_stubs -lssl -lcrypto -llwt_unix_stubs -lpthread -lthreads -lunix -lpthread -lbigarray -lunix -lcamlrun -lm -ldl -lpthread
learnocaml_main: -llaolao_stubs -lbase_stubs -lssl_threads_stubs -lssl -lcrypto -llwt_unix_stubs -lpthread -lthreads -lunix -lpthread -lcamlstr -lcstruct_stubs -lbigarray -lunix -lcamlrun -lm -ldl -lpthread
learnocaml_server_main: -lcamlstr -llaolao_stubs -lbase_stubs -lssl_threads_stubs -lssl -lcrypto -llwt_unix_stubs -lpthread -lthreadsnat -lpthread -lcstruct_stubs -lbigarray -lunix -lm -ldl

I have only two remaining questions for @AltGr:

  1. among the libs mentioned above, this one is not mentioned in src/main/linking_flags.sh nor in ./dune: -ldl. Is this OK anyway? or we should add it in COMMON_LIBS.

  2. on macOS, the otool -L command displays (link to the logs):

_build/install/default/bin/learn-ocaml:
	/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1281.100.1)
	/usr/lib/libncurses.5.4.dylib (compatibility version 5.4.0, current version 5.4.0)
_build/install/default/bin/learn-ocaml-client:
	/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1281.100.1)
	/usr/lib/libncurses.5.4.dylib (compatibility version 5.4.0, current version 5.4.0)
_build/install/default/bin/learn-ocaml-server:
	/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1281.100.1)

While /usr/lib/libSystem.B.dylib corresponds to the macOS standard library that looks mandatory to keep as is, do you think that /usr/lib/libncurses.5.4.dylib ought to be "bundled", somehow?

Anyway, as said earlier, we had tested these executables on macOS 11.1 and they were running fine… so maybe a good motto is "don't fix what is not broken…" :)

To sum up (and sorry for the long post):

I think the PR is ready, so if @AltGr confirms there is nothing further to do for the two points above, I could merge it on, say, this Friday.

@erikmd erikmd force-pushed the gh-act-static-bin branch from 781edca to e41d8a1 Compare July 22, 2021 23:23
@erikmd erikmd self-assigned this Jul 23, 2021
@erikmd
Copy link
Member

erikmd commented Jul 23, 2021

following-up my question

on macOS

about this code

COMMON_LIBS="camlstr base_stubs ssl_threads_stubs /usr/local/opt/openssl/lib/libssl.a /usr/local/opt/openssl/lib/libcrypto.a cstruct_stubs lwt_unix_stubs bigarray unix pthread"

To sum up, thanks to these added lines:

# See src/main/linking_flags.sh
make detect-libs

we get on macOS (link to the logs):

learnocaml_client: -lcamlstr -lcstruct_stubs -lbase_stubs -lssl_threads_stubs -lssl -lcrypto -llwt_unix_stubs -lpthread -lthreads -lunix -lpthread -lbigarray -lunix -lcamlrun -lcurses -lpthread
learnocaml_main: -llaolao_stubs -lbase_stubs -lssl_threads_stubs -lssl -lcrypto -llwt_unix_stubs -lpthread -lthreads -lunix -lpthread -lcamlstr -lcstruct_stubs -lbigarray -lunix -lcamlrun -lcurses -lpthread
learnocaml_server_main: -lcamlstr -llaolao_stubs -lbase_stubs -lssl_threads_stubs -lssl -lcrypto -llwt_unix_stubs -lpthread -lthreadsnat -lpthread -lcstruct_stubs -lbigarray -lunix

so I guess that we could add curses on the two lines calling ./linking_flags.sh?

(rule
(targets linking_main.sexp)
(action (with-stdout-to %{targets}
(run ./linking_flags.sh %{env:LINKING_MODE=dynamic} laolao_stubs threads camlrun))))
(rule
(targets linking_client.sexp)
(action (with-stdout-to %{targets}
(run ./linking_flags.sh %{env:LINKING_MODE=dynamic} threads camlrun))))

(conditionaly if (= %{ocaml-config:system} macosx), maybe)

@AltGr
Copy link
Collaborator Author

AltGr commented Jul 23, 2021

among the libs mentioned above, this one is not mentioned in src/main/linking_flags.sh nor in ./dune: -ldl. Is this OK anyway? or we should add it in COMMON_LIBS.

Well dl stands for dynamic loading, so... we shouldn't need it :)

Anyway, as said earlier, we had tested these executables on macOS 11.1 and they were running fine… so maybe a good motto is "don't fix what is not broken…" :)

Yes, apparently you are not really supposed to do purely static executables on macOS, and the system is much, much more homogeneous so we shouldn't need to bother either, our execs should run; the point of statically linking the libs is to avoid runtime dependencies upon optional libs/packages.

so I guess that we could add curses on the two lines calling ./linking_flags.sh?

IIRC curses is part of the base macOS install so it shouldn't be required, but we could add it (maybe there are older versions still around on some systems ??).
That leaves open the question of why the heck learn-ocaml would require ncurses but well...

@erikmd
Copy link
Member

erikmd commented Jul 23, 2021

Hi @AltGr, thanks for your feedback.

That leaves open the question of why the heck learn-ocaml would require ncurses but well...

Maybe because of the cmdliner opam dependency?

Also just FYI, I've found this reference saying:

-lm or -lpthread do no harm, but are unnecessary. The -lm option links to the math library, and -lpthread links to the POSIX threads library. Since libSystem provides these functions, you don't need to use these options.

In Mac OS X 10.1 and earlier versions, the curses screen library (a set of functions for controlling a terminal display) was part of libSystem.dylib. In Mac OS X 10.2 (Jaguar), the ncurses library (/usr/lib/libncurses.5.dylib) took the place of curses. You may still encounter source code releases that look for curses in libSystem.dylib, which will result in linking errors. You can work around this problem by adding -lcurses to the linker arguments. This is portable to earlier versions of Mac OS X as well,

So as a last tweak, I've removed pthread when %{ocaml-config:system} = macosx, and I've added curses as had suggested the output of make detect-libs… If the CI passes anew 😅, I'll merge the current version.

@erikmd erikmd merged commit ff9f091 into ocaml-sf:master Jul 23, 2021
@erikmd erikmd added this to the learn-ocaml 0.13.0 milestone Sep 30, 2021
@erikmd erikmd mentioned this pull request Oct 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: feature New user-facing feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants