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

build: Link bench binary statically #1119

Closed
wants to merge 1 commit into from

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Jul 5, 2022

While testing #1022 I've noticed a linker error when cross compiling with --host=x86_64-w64-mingw32 in particular circumstances.

I cannot recall my building environment then, but currently, on Ubuntu 22.04 with gcc 11.2, this error raises unconditionally:

$ ./autogen.sh && ./configure --host=x86_64-w64-mingw32 && make clean
$ make
make  all-am
make[1]: Entering directory '/home/hebasto/git/secp256k1'
  CC       src/bench.o
  CC       src/libsecp256k1_la-secp256k1.lo
  CC       src/libsecp256k1_precomputed_la-precomputed_ecmult.lo
  CC       src/libsecp256k1_precomputed_la-precomputed_ecmult_gen.lo
  CCLD     libsecp256k1_precomputed.la
  CCLD     libsecp256k1.la
  CCLD     bench.exe
libtool:   error: Could not determine the host path corresponding to
libtool:   error:   '/home/hebasto/git/secp256k1/.libs'
libtool:   error: Continuing, but uninstalled executables may not work.
libtool:   error: Could not determine the host path corresponding to
libtool:   error:   '/home/hebasto/git/secp256k1/.libs:/usr/local/lib:/usr/local/bin'
libtool:   error: Continuing, but uninstalled executables may not work.
  CC       src/bench_internal-bench_internal.o
  CCLD     bench_internal.exe
  CC       src/bench_ecmult-bench_ecmult.o
  CCLD     bench_ecmult.exe
  CC       src/tests-tests.o
  CCLD     tests.exe
  CC       src/exhaustive_tests-tests_exhaustive.o
  CCLD     exhaustive_tests.exe
make[1]: Leaving directory '/home/hebasto/git/secp256k1'

This PR prevents linker errors when cross compiling with --host=x86_64-w64-mingw32.

Please note that the same linker flag is used for all of the tests and examples. Moreover, it was used in the initial Autotools commit:

bench_LDFLAGS = -static

This change prevents linker errors when cross compiling with
`--host=x86_64-w64-mingw32`.
@real-or-random
Copy link
Contributor

real-or-random commented Jul 5, 2022

For what it's worth, these messages are entirely harmless.

For shared libraries, libtool has some magic which creates wrapper executables (scripts or binaries) that let you run your programs without installing the shared library or fiddling around with environment settings such as LD_LIBRARY_PATH. In this case, this means libtool creates a ./bench.exe, which is no more than a wrapper binary that sets up an environment with the correct settings and then runs the proper ./libs/bench.exe, which in turn then knows where to find the shared library in ./libs/libsecp256k1-0.dll. So you could run ./bench.exe if you happen to have wine installed.

Now the crux is that even creating the wrappers needs a filename conversion between Unix and Windows, and this requires the winepath program from wine. Because you don't have wine installed, this conversion fails. But this is not a problem because you anyway couldn't run ./bench.exe because you don't have wine installed...

The problem with -static is that it avoids the entire process of creating wrapper executables. Yes, this makes the problem disappear. But if you install wine and run ./configure --host=x86_64-w64-mingw32 --disable-static to disable static libraries entirely. Then ./bench.exe won't work, it just can't find the dll. (You may wonder why the build worked at all... This is just libtool being too clever and stripping -static when you pass --disable-static...) Same happens with the examples.

So I believe we should do the opposite: Drop -static for all binaries that should be linked against the library (as a user would do). This naturally includes all the examples but also the normal benchmark (because the bench binary benchmarks the public API only), but not the tests. In other words, there was a reason why -static was missing for the benchmarks.

@elichai Is there a specific reason why you set -static for the examples? Or was this just "copy/paste and it worked"?

@hebasto
Copy link
Member Author

hebasto commented Jul 5, 2022

For what it's worth, these messages are entirely harmless.

Yes, they are. In terms of the resulted artifacts. From the user's point of view, the "error" word in the log means something wrong. It should be avoided or documented clearly.

So I believe we should do the opposite: Drop -static for all binaries that should be linked against the library (as a user would do). This naturally includes all the examples but also the normal benchmark (because the bench binary benchmarks the public API only), but not the tests. In other words, there was a reason why -static was missing for the benchmarks.

Maybe make behavior dependent on the fact whether Wine has been installed?

@real-or-random
Copy link
Contributor

Maybe make behavior dependent on the fact whether Wine has been installed?

Yeah, I'd be somewhat reluctant to maintain a workaround for this if it's not actually only a problem in the error message. I agree an "error" sounds scary but even the message then says "Continuing, but uninstalled executables may not work.". What do you think of reporting that to libtool and suggesting they change it to a warning? The manual even says it's a warning:

Failure to convert paths (see File Name Conversion Failure) will cause a warning to be issued,

libtool was dead for a long time but now they have a new maintainer and they recently tagged a release after 7 years.

@elichai
Copy link
Contributor

elichai commented Jul 6, 2022

@elichai Is there a specific reason why you set -static for the examples? Or was this just "copy/paste and it worked"?

From what I remember it was that I saw that everything was using -static so I also added it, I also seem to remember that removing it complicates running Valgrind a bit because it runs it on the wrapper script, but I'm not sure about that.

@hebasto
Copy link
Member Author

hebasto commented Jul 12, 2022

But if you install wine and run ./configure --host=x86_64-w64-mingw32 --disable-static to disable static libraries entirely. Then ./bench.exe won't work...

IIUC, we do not use such a configuration in CI. Why we should bother about it at all?

https://gnusha.org/secp256k1/2022-07-12.log

@hebasto hebasto closed this Jan 18, 2023
@hebasto hebasto deleted the 220705-bench branch January 19, 2023 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants