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: Fix zeromq package when cross-compiling #24134

Merged
merged 3 commits into from
Jan 27, 2022

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Jan 23, 2022

Since v4.3.3 (zeromq/libzmq@068385c) libzmq uses libbsd by default.

This PR disables libbsd explicitly, as it's not a part of our depends. Zeromq will fallback to its internal strlcpy implementation.

Otherwise, on systems with installed libbsd-dev package the zeromq package build system erroneously detects libbsd package from the host system:

--- a/libzmq.pc
+++ b/libzmq.pc
@@ -8,5 +8,5 @@
 Version: 4.3.4
 Libs: -L${libdir} -lzmq
 Libs.private:  -lpthread
-Requires.private: 
+Requires.private:  libbsd
 Cflags: -I${includedir} 

This causes the configure fails to detect the zeromq package:

configure: WARNING: libzmq version 4.x or greater not found, disabling

Other minor improvements:

  • fixed netbsd_kevent_void.patch offset
  • disabled valgrind as it's used in unit tests which we do not run:
--- a/zmq-configure-output
+++ b/zmq-configure-output
@@ -119,11 +119,6 @@
 checking whether the g++ -m64 linker (/usr/bin/ld -m elf_x86_64) supports shared libraries... yes
 checking dynamic linker characteristics... (cached) GNU/Linux ld.so
 checking how to hardcode library paths into programs... immediate
-checking for valgrind... valgrind
-checking for Valgrind tool memcheck... memcheck
-checking for Valgrind tool helgrind... helgrind
-checking for Valgrind tool drd... drd
-checking for Valgrind tool exp-sgcheck... exp-sgcheck
 checking linker version script flag... --version-script
 checking if version scripts can use complex wildcards... yes
 checking for working posix_memalign... yes

@hebasto
Copy link
Member Author

hebasto commented Jan 24, 2022

Assign "23.0" milestone?

@fanquake
Copy link
Member

Approach NACK. Why not just pass --disable-libbsd? Would be good to update the PR description to actually explain what this is used for, and why it's ok to avoid.

on some systems (impish, jammy)

Isn't it just any system with libbsd available?

@hebasto
Copy link
Member Author

hebasto commented Jan 25, 2022

on some systems (impish, jammy)

Isn't it just any system with libbsd available?

Correct. The PR description updated.

@hebasto
Copy link
Member Author

hebasto commented Jan 25, 2022

Approach NACK. Why not just pass --disable-libbsd? Would be good to update the PR description to actually explain what this is used for, and why it's ok to avoid.

  1. It is wrong when a package build system searches for its dependencies among host's packages instead of strictly relying on $(package)_dependencies.
  2. This PR is not about "build with libbsd or avoid it". It restores behavior that was before build: use zeromq 4.3.4 in depends & fix NetBSD 10 build #23956. Including libbsd in our depends build system as a zeromq package dependency is out of this PR scope.

@fanquake
Copy link
Member

It is wrong when a package build system searches for its dependencies among host's packages instead of strictly relying on $(package)_dependencies.

I agree, but that's also something that should be solved at a higher level in the build system, rather than ad-hoc on a per-package basis. Otherwise we should just go ahead and add the same change to all other packages that use pkg-config.

This PR is not about "build with libbsd or avoid it". It restores behavior that was before

If this PR makes a change that will ensure zeromqs build system doesn't find libbsd, how is it not about avoiding it? I'm not sure what you mean by restoring the previous behaviour (other than make things work), but I don't necessarily think that is the right goal. We're building a newer version of the package, that has a different build system and new (optional) dependencies.

Including libbsd in our depends build system as a zeromq package dependency

I'm not suggesting we do that, especially if we end up passing --disable-libbsd. If we pass that flag to zeromqs config opts, it will just use it's internal strlcpy implementation, and libbsd will no-longer a dependency.

This change fixes "Hunk #1 succeeded at 307 (offset -1 lines)."
Since v4.3.3 (068385c951c0608edec6264d55ba9c4c923acccc) libbsd is used
by default. As we have no libbsd package in our depends, disable it
explicitly. Zeromq will fallback to its internal strlcpy implementation.
We are not running unit tests, therefore it is not required.
@hebasto
Copy link
Member Author

hebasto commented Jan 25, 2022

Updated c0f664c -> f13e642 (pr24134.01 -> pr24134.02):

  • addressed @fanquake's comments
  • additionally disabled valgrind which is used in unit tests

PR description has been updated.

Copy link
Member

@fanquake fanquake left a comment

Choose a reason for hiding this comment

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

ACK f13e642

@fanquake fanquake merged commit 4241c19 into bitcoin:master Jan 27, 2022
@laanwj laanwj added this to the 23.0 milestone Jan 27, 2022
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jan 28, 2022
f13e642 build: Disable valgrind when building zeromq package in depends (Hennadii Stepanov)
b970f03 build: Disable libbsd when building zeromq package in depends (Hennadii Stepanov)
7789999 build: Update netbsd_kevent_void.patch (Hennadii Stepanov)

Pull request description:

  Since v4.3.3 (zeromq/libzmq@068385c) `libzmq` uses `libbsd` by default.

  This PR disables `libbsd` explicitly, as it's not a part of our depends. Zeromq will fallback to its internal `strlcpy` implementation.

  Otherwise, on systems with installed `libbsd-dev` package the `zeromq` package build system erroneously detects `libbsd` package from the host system:

  ```diff
  --- a/libzmq.pc
  +++ b/libzmq.pc
  @@ -8,5 +8,5 @@
   Version: 4.3.4
   Libs: -L${libdir} -lzmq
   Libs.private:  -lpthread
  -Requires.private:
  +Requires.private:  libbsd
   Cflags: -I${includedir}
  ```

  This causes the `configure` fails to detect the `zeromq` package:
  ```
  configure: WARNING: libzmq version 4.x or greater not found, disabling
  ```

  ---

  Other minor improvements:
  - fixed `netbsd_kevent_void.patch` offset
  - disabled valgrind as it's used in unit tests which we do not run:
  ```diff
  --- a/zmq-configure-output
  +++ b/zmq-configure-output
  @@ -119,11 +119,6 @@
   checking whether the g++ -m64 linker (/usr/bin/ld -m elf_x86_64) supports shared libraries... yes
   checking dynamic linker characteristics... (cached) GNU/Linux ld.so
   checking how to hardcode library paths into programs... immediate
  -checking for valgrind... valgrind
  -checking for Valgrind tool memcheck... memcheck
  -checking for Valgrind tool helgrind... helgrind
  -checking for Valgrind tool drd... drd
  -checking for Valgrind tool exp-sgcheck... exp-sgcheck
   checking linker version script flag... --version-script
   checking if version scripts can use complex wildcards... yes
   checking for working posix_memalign... yes
  ```

ACKs for top commit:
  fanquake:
    ACK f13e642

Tree-SHA512: d4c86d4a841eb6e7c32157e84972243072f905496c2a4c14ec6f6ab4216df6695cbf29baa2233ce27eaede35d1e250ad2f9975b16f570d01509f0c5da4597cad
@hebasto hebasto deleted the 220123-zmq branch February 2, 2022 09:28
hebasto added a commit to hebasto/bitcoin that referenced this pull request Feb 6, 2022
Since v4.3.3 (068385c951c0608edec6264d55ba9c4c923acccc) libbsd is used
by default. As we have no libbsd package in our depends, disable it
explicitly. Zeromq will fallback to its internal strlcpy implementation.

Github-Pull: bitcoin#24134
Rebased-From: b970f03
hebasto added a commit to hebasto/bitcoin that referenced this pull request Feb 6, 2022
Since v4.3.3 (068385c951c0608edec6264d55ba9c4c923acccc) libbsd is used
by default. As we have no libbsd package in our depends, disable it
explicitly. Zeromq will fallback to its internal strlcpy implementation.

Github-Pull: bitcoin#24134
Rebased-From: b970f03
fanquake added a commit to bitcoin-core/gui that referenced this pull request Dec 8, 2022
… to all `$(package)_*_cmds`

affbf58 build: Move environment variables into `$(package)_config_env` (Hennadii Stepanov)
d44fcd3 build: Make $(package)_*_env available to all $(package)_*_cmds (Hennadii Stepanov)

Pull request description:

  On master (1e7564e) the depends build system, which is based on pure GNU Make, works, but it lacks robustness, and in some corner cases it fails. For example, see bitcoin/bitcoin#22552.

  Another [bug](bitcoin/bitcoin#22719) in the depends build system has already become a problem at least two times in the past (bitcoin/bitcoin#16883 (comment) and bitcoin/bitcoin#24134). Each time the problem was solved with other means.

  The initial [solution](bitcoin/bitcoin#19882) had some discussion. Also it was discussed on the IRC meeting in #bitcoin-core-builds channel. This PR, actually, is a resurrection of it, as the bug silently struck pretty [recently](bitcoin/bitcoin#24134).

  The bug is well described in bitcoin/bitcoin#22719.

  Here is another, a bit simpler description, which requires only basic shell (bash, dash etc) experience.
  After creating targets by this code:https://github.com/bitcoin/bitcoin/blob/1e7564eca8a688f39c75540877ec3bdfdde766b1/depends/funcs.mk#L280 a "draft" line of recipe like `$($(1)_config_env) $(call $(1)_config_cmds, $(1))` becomes a shell command sequence `VAR1=foo VAR2=bar command1 && command2` which is supposed to be executed in a [new sub-shell](https://www.gnu.org/software/make/manual/html_node/Execution.html#Execution).

  Please note that `VAR1=foo VAR2=bar` part is visible for the first `command1` only (for details see shell docs). Example:
  ```sh
  $ VAR1="foo" VAR2="bar" echo "begin" && printenv VAR1 && printenv VAR2 && echo "end"
  begin
  $ echo $?
  1
  ```

  Using the `export` command is a trivial solution:
  ```sh
  $ export VAR1="foo" VAR2="bar"; echo "begin" && printenv VAR1 && printenv VAR2 && echo "end"
  begin
  foo
  bar
  end
  $ echo $?
  0
  ```

  As a [new sub-shell](https://www.gnu.org/software/make/manual/html_node/Execution.html#Execution) is invoked for each line of the recipe, there are no side effects of using `export`. It means this solution should not be considered invasive.

  Fixes bitcoin/bitcoin#22719.

  ---

  Also this PR removes no longer needed crutch from `qt.mk`.

ACKs for top commit:
  fanquake:
    ACK affbf58

Tree-SHA512: 0ce2cf82870a7774bdf1592fac50857126ae47da902e349f1092d50109223be9d6a8efd5e92ec08c2ca775b17516482aabaf232378950ade36484a883acc177b
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 8, 2022
…`$(package)_*_cmds`

affbf58 build: Move environment variables into `$(package)_config_env` (Hennadii Stepanov)
d44fcd3 build: Make $(package)_*_env available to all $(package)_*_cmds (Hennadii Stepanov)

Pull request description:

  On master (1e7564e) the depends build system, which is based on pure GNU Make, works, but it lacks robustness, and in some corner cases it fails. For example, see bitcoin#22552.

  Another [bug](bitcoin#22719) in the depends build system has already become a problem at least two times in the past (bitcoin#16883 (comment) and bitcoin#24134). Each time the problem was solved with other means.

  The initial [solution](bitcoin#19882) had some discussion. Also it was discussed on the IRC meeting in #bitcoin-core-builds channel. This PR, actually, is a resurrection of it, as the bug silently struck pretty [recently](bitcoin#24134).

  The bug is well described in bitcoin#22719.

  Here is another, a bit simpler description, which requires only basic shell (bash, dash etc) experience.
  After creating targets by this code:https://github.com/bitcoin/bitcoin/blob/1e7564eca8a688f39c75540877ec3bdfdde766b1/depends/funcs.mk#L280 a "draft" line of recipe like `$($(1)_config_env) $(call $(1)_config_cmds, $(1))` becomes a shell command sequence `VAR1=foo VAR2=bar command1 && command2` which is supposed to be executed in a [new sub-shell](https://www.gnu.org/software/make/manual/html_node/Execution.html#Execution).

  Please note that `VAR1=foo VAR2=bar` part is visible for the first `command1` only (for details see shell docs). Example:
  ```sh
  $ VAR1="foo" VAR2="bar" echo "begin" && printenv VAR1 && printenv VAR2 && echo "end"
  begin
  $ echo $?
  1
  ```

  Using the `export` command is a trivial solution:
  ```sh
  $ export VAR1="foo" VAR2="bar"; echo "begin" && printenv VAR1 && printenv VAR2 && echo "end"
  begin
  foo
  bar
  end
  $ echo $?
  0
  ```

  As a [new sub-shell](https://www.gnu.org/software/make/manual/html_node/Execution.html#Execution) is invoked for each line of the recipe, there are no side effects of using `export`. It means this solution should not be considered invasive.

  Fixes bitcoin#22719.

  ---

  Also this PR removes no longer needed crutch from `qt.mk`.

ACKs for top commit:
  fanquake:
    ACK affbf58

Tree-SHA512: 0ce2cf82870a7774bdf1592fac50857126ae47da902e349f1092d50109223be9d6a8efd5e92ec08c2ca775b17516482aabaf232378950ade36484a883acc177b
@bitcoin bitcoin locked and limited conversation to collaborators Feb 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants