Skip to content

Conversation

@fanquake
Copy link
Member

We currently do this sporadically. Not only amongst packages, but across OS's, i.e sometimes it's done for BSDs/Android, and sometimes not.

Configure with --with-pic globally instead. I think this generally makes more sense, and should not have any downsides.

See related discussion in #28846 (comment).

@fanquake fanquake requested a review from theuni February 27, 2024 15:17
@DrahtBot
Copy link
Contributor

DrahtBot commented Feb 27, 2024

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK hebasto
Concept ACK theuni

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #29708 (depends: build libnatpmp with CMake by fanquake)
  • #29707 (depends: build miniupnpc with CMake by fanquake)
  • #29706 (depends: set two CMake options globally by fanquake)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Copy link
Member

@theuni theuni left a comment

Choose a reason for hiding this comment

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

Concept ACK.

I believe we historically used --with-pic per-platform because some platforms (win32 and/or darwin) spewed warnings when turning it on. Is that no longer true? IIRC for darwin, pic is the default, so the "no need to specify pic" warnings were doubly annoying.

Even if that's still the case, it makes more sense to use --without-pic for those platforms to override this more sane default.

@fanquake
Copy link
Member Author

win32 and/or darwin) spewed warnings when turning it on. Is that no longer true?

I haven't seen any warnings / issues across any platforms.

@DrahtBot
Copy link
Contributor

Guix builds (on x86_64)

File commit ba907f9
(master)
commit cebea5eefe81e51bfcf96dc335709d8cf90eb8c4
(master and this pull)
SHA256SUMS.part b5eba8a283436c3b... 754a4c8f11da35d7...
*-aarch64-linux-gnu-debug.tar.gz 829c712d2b08d013... 7529ebc97c119d03...
*-aarch64-linux-gnu.tar.gz 419ac661f51e5b5a... b99c5e6ad7e73155...
*-arm-linux-gnueabihf-debug.tar.gz 5663fee152088d6f... d912f46f2873d15d...
*-arm-linux-gnueabihf.tar.gz 8d4e834d33f08496... 0b5042b863f0265d...
*-arm64-apple-darwin-unsigned.tar.gz 2b74ac81b62642f4... 0101b28252174327...
*-arm64-apple-darwin-unsigned.zip 3c4e78caeed83315... 8113c1196beef64d...
*-arm64-apple-darwin.tar.gz 876a77dd6de31080... e9b1336611c5c4ce...
*-powerpc64-linux-gnu-debug.tar.gz 46ea102b47db3905... 5f46a3adcf74d416...
*-powerpc64-linux-gnu.tar.gz 2b3b9a798d909d80... 15b07d4d146d9d68...
*-powerpc64le-linux-gnu-debug.tar.gz b0c8e866e7f17f23... 0333e6e7a1829a14...
*-powerpc64le-linux-gnu.tar.gz 9fa3773087c6550e... e02d21aaea403233...
*-riscv64-linux-gnu-debug.tar.gz 344064b382c4c6cc... 975c5dfada7d2a08...
*-riscv64-linux-gnu.tar.gz 870c4ffefb4eeb78... f17f70118885c38c...
*-x86_64-apple-darwin-unsigned.tar.gz 94d1d92ffd669c37... 4b1f0b979b904e54...
*-x86_64-apple-darwin-unsigned.zip 735ada59d43cbc8d... 32378d710a7eaa48...
*-x86_64-apple-darwin.tar.gz dc837a3576093024... 1f0c66a0edc82ce5...
*-x86_64-linux-gnu-debug.tar.gz f6cc9e3e9a625706... 8068815d3fb0815e...
*-x86_64-linux-gnu.tar.gz 365e71ff601e95fa... 276a7474c9dbe0d4...
*.tar.gz 43ffebf97a4c5fc5... 23982d4d0eb818cc...
guix_build.log b98357662408e8ef... 4325978f1cff1cd5...
guix_build.log.diff 175e98b4006f89f1...

We currently do this sporadically. Not only amongst packages, but across
OS's, i.e sometimes it's done for BSDs/Android, and sometimes not.

Configure with `--with-pic` globally instead. I think this generally
makes more sense, and should not have any downsides.

See related discussion in
bitcoin#28846 (comment).
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

Concept ACK.

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK e037c4f.

My Guix build:

x86_64
3e0ae9b99f1d11d7e8d474ed1be002b165cc81dada4f187d86a4cc1011261d98  guix-build-e037c4fe0914/output/aarch64-linux-gnu/SHA256SUMS.part
40edb8b425282776f54192eedbdb2c6a48ae5474075aa267a1d7af12a6a5facf  guix-build-e037c4fe0914/output/aarch64-linux-gnu/bitcoin-e037c4fe0914-aarch64-linux-gnu-debug.tar.gz
2df23792f1dba91ca0c81fb2f1c31ee0f0cc13780d9b99762bd6ccbbc7bdafc5  guix-build-e037c4fe0914/output/aarch64-linux-gnu/bitcoin-e037c4fe0914-aarch64-linux-gnu.tar.gz
4dd2d1eee3cbe6a6e3319fbfd526f7737b421f2c1edc0d6032db2855bf0c4806  guix-build-e037c4fe0914/output/arm-linux-gnueabihf/SHA256SUMS.part
fa0565599c3f58707253f36f79cb49921e1e7c77d6c830dfea1bef66f0af79bb  guix-build-e037c4fe0914/output/arm-linux-gnueabihf/bitcoin-e037c4fe0914-arm-linux-gnueabihf-debug.tar.gz
a56ca1384ea4b0c4ad61ca24a975ad1142902c17b08800ba147cbfba62026a33  guix-build-e037c4fe0914/output/arm-linux-gnueabihf/bitcoin-e037c4fe0914-arm-linux-gnueabihf.tar.gz
0c4429b4af5fb11efa8614f8fba6e5f56397f411a87d981ce5d6b10217dcd361  guix-build-e037c4fe0914/output/arm64-apple-darwin/SHA256SUMS.part
d2267cda3b8c48cf5b02d99144657c9448afa89b85519566e89e6cc32c2ad947  guix-build-e037c4fe0914/output/arm64-apple-darwin/bitcoin-e037c4fe0914-arm64-apple-darwin-unsigned.tar.gz
3cdc7a717e9d81c611ce321962f56e67fdfd6d241eb77c21a8ae32f45c347a4a  guix-build-e037c4fe0914/output/arm64-apple-darwin/bitcoin-e037c4fe0914-arm64-apple-darwin-unsigned.zip
ba6b062dd506045cd63eedaa75e6bcfe7b50214c73b5546feb3048aa0569047f  guix-build-e037c4fe0914/output/arm64-apple-darwin/bitcoin-e037c4fe0914-arm64-apple-darwin.tar.gz
cf99ecd355a4683ff4508d278d0ca68687869ff42a4a2625daff4946d80bdcf1  guix-build-e037c4fe0914/output/dist-archive/bitcoin-e037c4fe0914.tar.gz
9be46a24ab5e386ad7e03c9dd6a1ee8fbdc636bb3971a8c2af6147ad883cd83d  guix-build-e037c4fe0914/output/powerpc64-linux-gnu/SHA256SUMS.part
d35c1d16465b5bdf2485c9ebf67de0aa871c5e8fb14a51ac3f27184671275246  guix-build-e037c4fe0914/output/powerpc64-linux-gnu/bitcoin-e037c4fe0914-powerpc64-linux-gnu-debug.tar.gz
fa4658014c7a2e4b65161a0a9f0bc4ad88000f719e72ac77c89c61c970eed6f1  guix-build-e037c4fe0914/output/powerpc64-linux-gnu/bitcoin-e037c4fe0914-powerpc64-linux-gnu.tar.gz
b29b6265fc7194bf01407daf6b3fd26af26c00c36287a48c624fc92becea80f4  guix-build-e037c4fe0914/output/riscv64-linux-gnu/SHA256SUMS.part
f3b2818f56c03938d08b1916bcd9599bad398169d4bb06a2db7ea846db9bcb80  guix-build-e037c4fe0914/output/riscv64-linux-gnu/bitcoin-e037c4fe0914-riscv64-linux-gnu-debug.tar.gz
479966f0c8bb21b28cc0c772e46621ae4fe434d47b1d04f50f88d290a12921fa  guix-build-e037c4fe0914/output/riscv64-linux-gnu/bitcoin-e037c4fe0914-riscv64-linux-gnu.tar.gz
c5e2180c4464755137b9c567d93c093f28022973bf582414a10445f9264ca13b  guix-build-e037c4fe0914/output/x86_64-apple-darwin/SHA256SUMS.part
620c6563f7639325d9e5dd96f459d1af4f667d8604d9f8f88c37ba9417eb10ca  guix-build-e037c4fe0914/output/x86_64-apple-darwin/bitcoin-e037c4fe0914-x86_64-apple-darwin-unsigned.tar.gz
9ff0d89415fd05a296265b29829e059c72befed45149b2aabac68c30b2726ff5  guix-build-e037c4fe0914/output/x86_64-apple-darwin/bitcoin-e037c4fe0914-x86_64-apple-darwin-unsigned.zip
dbfe5885052afab52f0cf2dda2148cb0079385dc386d817c98679a50032a7252  guix-build-e037c4fe0914/output/x86_64-apple-darwin/bitcoin-e037c4fe0914-x86_64-apple-darwin.tar.gz
5e7cfa136d8c8923124f9eb1f5a84007215f4f4c44b9571d710cb64f177751a4  guix-build-e037c4fe0914/output/x86_64-linux-gnu/SHA256SUMS.part
f6930d10a849eb313a140fd7ae9fb14c856cd9925d55e45de18fbfd14d43fe51  guix-build-e037c4fe0914/output/x86_64-linux-gnu/bitcoin-e037c4fe0914-x86_64-linux-gnu-debug.tar.gz
d5e171d84595b80bff224ccb6d184d8433895b87098f1ada01c5d08435ca18a6  guix-build-e037c4fe0914/output/x86_64-linux-gnu/bitcoin-e037c4fe0914-x86_64-linux-gnu.tar.gz
adc3f93db33ab431576c4a01cca8b749f05a623ff82be29c51257075b3d86d72  guix-build-e037c4fe0914/output/x86_64-w64-mingw32/SHA256SUMS.part
c680b565970dba64b38517e00a535b5d710e5a9f2a31493dc0f2ab6285f5ad9a  guix-build-e037c4fe0914/output/x86_64-w64-mingw32/bitcoin-e037c4fe0914-win64-debug.zip
e43572eebd4b240b9caf1a9ab2abefc4ac2770c326934a57a55a642a5bdc70a6  guix-build-e037c4fe0914/output/x86_64-w64-mingw32/bitcoin-e037c4fe0914-win64-setup-unsigned.exe
5182a532a782a209e32f08967a36adf60ae87bd4b6b08da0480d9d2e37bb1157  guix-build-e037c4fe0914/output/x86_64-w64-mingw32/bitcoin-e037c4fe0914-win64-unsigned.tar.gz
79a502f6d2637bea6471e9f9fc27c26443bbc512b7cee0e37a46180bb2ca3f4a  guix-build-e037c4fe0914/output/x86_64-w64-mingw32/bitcoin-e037c4fe0914-win64.zip

@DrahtBot DrahtBot requested a review from theuni March 24, 2024 14:46
@fanquake fanquake merged commit 5560741 into bitcoin:master Mar 25, 2024
fanquake added a commit to fanquake/bitcoin that referenced this pull request Mar 25, 2024
Rather than potentially having to set this per-package, set it globally,
as this should always be what we want. Without doing this, changes in
later commits will have to add this per-package.

Similar to bitcoin#29488, which is the
Autotools equivalent.
@fanquake fanquake deleted the depends_with_pic_cleanup branch March 25, 2024 10:52
@sedited
Copy link
Contributor

sedited commented Mar 25, 2024

Post-merge ACK e037c4f

fanquake added a commit that referenced this pull request Mar 25, 2024
76045bb depends: always set CMAKE_POSITION_INDEPENDENT_CODE=ON (fanquake)
d046236 depends: always set CMAKE_INSTALL_LIBDIR=lib/ (fanquake)

Pull request description:

  Set `CMAKE_INSTALL_LIBDIR=lib/` and `CMAKE_POSITION_INDEPENDENT_CODE=ON` globally in depends, rather than per-package. `CMAKE_INSTALL_LIBDIR=lib/` is needed to override the annoying [`GNUInstallDirs`](https://cmake.org/cmake/help/latest/module/GNUInstallDirs.html) `lib` vs `lib64` behaviour, and we always want PIC code. The PIC commit is the counterpart to the same Autotools change in #29488. I'm PRing these commits as I have a CMake branch building on top, and want to avoid adding the same workarounds to every package we are going to touch, but these can go in separately as the build should be tested for existing packages (i.e multiprocess).

ACKs for top commit:
  hebasto:
    re-ACK 76045bb.
  theuni:
    utACK 76045bb. Both changes make sense to me, and both can be overridden if needed, though I can't imagine we'd need to.

Tree-SHA512: 655a0b6b7ee5a5820f52e8e919ef03fc216d29f13f3904f72b64ce57436510e073c903039488d5740535c56e1f6221267229238c5231de5f8467d238fd562578
janus pushed a commit to BitgesellOfficial/bitgesell that referenced this pull request Apr 6, 2024
Rather than potentially having to set this per-package, set it globally,
as this should always be what we want. Without doing this, changes in
later commits will have to add this per-package.

Similar to bitcoin/bitcoin#29488, which is the
Autotools equivalent.
Fabcien added a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Aug 1, 2024
Summary:
```
We currently do this sporadically. Not only amongst packages, but across OS's, i.e sometimes it's done for BSDs/Android, and sometimes not.

Configure with `--with-pic` globally instead. I think this generally
makes more sense, and should not have any downsides.
```

Backport of [[bitcoin/bitcoin#29488 | core#29488]].

Test Plan: Run the GUIX builds.

Reviewers: #bitcoin_abc, PiRK

Reviewed By: #bitcoin_abc, PiRK

Differential Revision: https://reviews.bitcoinabc.org/D16566
roqqit pushed a commit to doged-io/doged that referenced this pull request Aug 1, 2024
Summary:
```
We currently do this sporadically. Not only amongst packages, but across OS's, i.e sometimes it's done for BSDs/Android, and sometimes not.

Configure with `--with-pic` globally instead. I think this generally
makes more sense, and should not have any downsides.
```

Backport of [[bitcoin/bitcoin#29488 | core#29488]].

Test Plan: Run the GUIX builds.

Reviewers: #bitcoin_abc, PiRK

Reviewed By: #bitcoin_abc, PiRK

Differential Revision: https://reviews.bitcoinabc.org/D16566
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Sep 27, 2024
e037c4f depends: always configure with --with-pic (fanquake)

Pull request description:

  We currently do this sporadically. Not only amongst packages, but across OS's, i.e sometimes it's done for BSDs/Android, and sometimes not.

  Configure with `--with-pic` globally instead. I think this generally makes more sense, and should not have any downsides.

  See related discussion in bitcoin#28846 (comment).

ACKs for top commit:
  hebasto:
    ACK e037c4f.

Tree-SHA512: efc743ff92f9f99f3ac16514e98363ad395c6f956cd4be7e785b5c573685baf7fcd68c51d6a705ee8761fc676eb045b7e61676595be0eb0f70f34e99174cddc0
PastaPastaPasta added a commit to dashpay/dash that referenced this pull request Sep 27, 2024
801c4fc build: followup to 29488 applied to gmp (pasta)
4b704a6 Merge bitcoin#28627: depends: zeromq 4.3.5 (fanquake)
0e6cb98 Merge bitcoin#26421: build: copy config.{guess,sub} post autogen in zmq package (fanquake)
cd33b69 Merge bitcoin#29488: depends: always configure with `--with-pic` (fanquake)
1b88674 Merge bitcoin#29287: depends: Do not override `CFLAGS` when building SQLite with `DEBUG=1` (fanquake)
f50fb6e Merge bitcoin#26998: depends: ensure we are appending to sqlite cflags (Andrew Chow)
25a594f Merge bitcoin#25987: build: compile depends sqlite with more recommended options (Andrew Chow)
d725c58 Merge bitcoin#27312: depends: qrencode 4.1.1 (fanquake)
482e5bb Merge bitcoin#27462: depends: fix compiling bdb with clang-16 on aarch64 (fanquake)
f0a53c9 Merge bitcoin#26994: depends: define `__BSD_VISIBLE` for FreeBSD bdb build (merge-script)
b1ac992 Merge bitcoin#26073: build: fix depends bdb compilation for BSDs (fanquake)
45e0f6e Merge bitcoin#25763: bdb: disable Werror for format-security (fanquake)

Pull request description:

  ## Issue being fixed or feature implemented
  First batch of PRs which lead towards cmake, focusing on having nothing or very little done out of order / with significant conflicts

  ## What was done?
  batch of 11 backports which lead towards cmake

  ## How Has This Been Tested?
  Built locally, seems to work, let's see CI

  ## Breaking Changes

  ## Checklist:
    _Go over all the following points, and put an `x` in all the boxes that apply._
  - [ ] I have performed a self-review of my own code
  - [ ] I have commented my code, particularly in hard-to-understand areas
  - [ ] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  UdjinM6:
    utACK 801c4fc
  knst:
    utACK  801c4fc

Tree-SHA512: b8b5299da9d82ab485ba5141ea12ba5c606f1a783b34c957d61e0a68d45865754fbc8bcbb0c5eabe3d410ff6262ce26789cf4a3af696905f7b7908e523c97816
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Oct 4, 2024
76045bb depends: always set CMAKE_POSITION_INDEPENDENT_CODE=ON (fanquake)
d046236 depends: always set CMAKE_INSTALL_LIBDIR=lib/ (fanquake)

Pull request description:

  Set `CMAKE_INSTALL_LIBDIR=lib/` and `CMAKE_POSITION_INDEPENDENT_CODE=ON` globally in depends, rather than per-package. `CMAKE_INSTALL_LIBDIR=lib/` is needed to override the annoying [`GNUInstallDirs`](https://cmake.org/cmake/help/latest/module/GNUInstallDirs.html) `lib` vs `lib64` behaviour, and we always want PIC code. The PIC commit is the counterpart to the same Autotools change in bitcoin#29488. I'm PRing these commits as I have a CMake branch building on top, and want to avoid adding the same workarounds to every package we are going to touch, but these can go in separately as the build should be tested for existing packages (i.e multiprocess).

ACKs for top commit:
  hebasto:
    re-ACK 76045bb.
  theuni:
    utACK 76045bb. Both changes make sense to me, and both can be overridden if needed, though I can't imagine we'd need to.

Tree-SHA512: 655a0b6b7ee5a5820f52e8e919ef03fc216d29f13f3904f72b64ce57436510e073c903039488d5740535c56e1f6221267229238c5231de5f8467d238fd562578
@bitcoin bitcoin locked and limited conversation to collaborators Mar 25, 2025
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.

5 participants