depends: always configure with `--with-pic`

fanquake commented at 3:17 pm on February 27, 2024: 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 (review).

fanquake requested review from theuni on Feb 27, 2024

DrahtBot commented at 3:17 pm on February 27, 2024: contributor

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.

fanquake added the label DrahtBot Guix build requested on Feb 27, 2024

theuni commented at 7:08 pm on February 27, 2024: member

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 commented at 10:45 pm on February 27, 2024: member

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 commented at 3:23 pm on February 28, 2024: contributor

Guix builds (on x86_64)

File	commit ba907f96ad37c09c49c0e1532fad118fcb8dd4a8(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...`

DrahtBot removed the label DrahtBot Guix build requested on Feb 28, 2024

DrahtBot added the label Build system on Feb 28, 2024

depends: always configure with --with-pic

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
https://github.com/bitcoin/bitcoin/pull/28846#discussion_r1399123100.

e037c4fe09

fanquake force-pushed on Mar 14, 2024

fanquake referenced this in commit 6166bf45ca on Mar 22, 2024

fanquake referenced this in commit c9f1b78d91 on Mar 22, 2024

fanquake referenced this in commit 5b2ae227ad on Mar 22, 2024

in depends/packages.md:166 in e037c4fe09

161@@ -162,6 +162,9 @@ From the [Gentoo Wiki entry](https://wiki.gentoo.org/wiki/Project:Quality_Assura
162 >  creates. This leads to massive overlinking, which is toxic to the Gentoo
163 >  ecosystem, as it leads to a massive number of unnecessary rebuilds.
164 
165+Where possible, packages are built with Position Independant Code. Either using
166+the autotools `--with-pic` flag, or `DCMAKE_POSITION_INDEPENDENT_CODE` with CMake.

hebasto commented at 11:53 am on March 24, 2024:

I believe that -DCMAKE_POSITION_INDEPENDENT_CODE=ON should be added somewhere in funcs.mk:https://github.com/bitcoin/bitcoin/blob/e037c4fe0914d8fa9149ce7532c0d70f738e79e9/depends/funcs.mk#L173-L187

Otherwise, the PIC is not applied consistently across all static libraries in depends (capnp, ~multiprocess~ and their native counterparts for now).

UPD. Just pull 5b2ae227ad6175393aff4b1df3df588c56e4a303 from #29708?

nit: maybe

0the autotools `--with-pic` flag, or `-DCMAKE_POSITION_INDEPENDENT_CODE=ON` with CMake.

?

fanquake commented at 12:00 pm on March 24, 2024:

I believe that -DCMAKE_POSITION_INDEPENDENT_CODE=ON should be added somewhere in funcs.mk:

This PR is for Autotools, not CMake. The CMake PR is #29706.

fanquake commented at 12:02 pm on March 24, 2024:

As for the nit, I don’t think we need to show exactly how to use the option, naming it should be fine.

hebasto commented at 12:08 pm on March 24, 2024:

To just name an option, the D flag should be skipped. If “built with Position Independant Code” was mentioned as a goal, the CMake equivalent of Autotools’ --with-pic is exactly the suggested variant. The current variant does not follow either goal.

However, it’s just a minor nit, and I’m going to ACK this PR as it is after finishing my Guix build.

fanquake commented at 12:25 pm on March 24, 2024:

I can drop the D in whichever of the conflicting PRs is merged second.

fanquake commented at 10:52 am on March 25, 2024:

Addressed in #29706.

hebasto commented at 11:58 am on March 24, 2024: member

Concept ACK.

hebasto approved

hebasto commented at 2:46 pm on March 24, 2024: member

ACK e037c4fe0914d8fa9149ce7532c0d70f738e79e9.

My Guix build:

 0x86_64
 13e0ae9b99f1d11d7e8d474ed1be002b165cc81dada4f187d86a4cc1011261d98  guix-build-e037c4fe0914/output/aarch64-linux-gnu/SHA256SUMS.part
 240edb8b425282776f54192eedbdb2c6a48ae5474075aa267a1d7af12a6a5facf  guix-build-e037c4fe0914/output/aarch64-linux-gnu/bitcoin-e037c4fe0914-aarch64-linux-gnu-debug.tar.gz
 32df23792f1dba91ca0c81fb2f1c31ee0f0cc13780d9b99762bd6ccbbc7bdafc5  guix-build-e037c4fe0914/output/aarch64-linux-gnu/bitcoin-e037c4fe0914-aarch64-linux-gnu.tar.gz
 44dd2d1eee3cbe6a6e3319fbfd526f7737b421f2c1edc0d6032db2855bf0c4806  guix-build-e037c4fe0914/output/arm-linux-gnueabihf/SHA256SUMS.part
 5fa0565599c3f58707253f36f79cb49921e1e7c77d6c830dfea1bef66f0af79bb  guix-build-e037c4fe0914/output/arm-linux-gnueabihf/bitcoin-e037c4fe0914-arm-linux-gnueabihf-debug.tar.gz
 6a56ca1384ea4b0c4ad61ca24a975ad1142902c17b08800ba147cbfba62026a33  guix-build-e037c4fe0914/output/arm-linux-gnueabihf/bitcoin-e037c4fe0914-arm-linux-gnueabihf.tar.gz
 70c4429b4af5fb11efa8614f8fba6e5f56397f411a87d981ce5d6b10217dcd361  guix-build-e037c4fe0914/output/arm64-apple-darwin/SHA256SUMS.part
 8d2267cda3b8c48cf5b02d99144657c9448afa89b85519566e89e6cc32c2ad947  guix-build-e037c4fe0914/output/arm64-apple-darwin/bitcoin-e037c4fe0914-arm64-apple-darwin-unsigned.tar.gz
 93cdc7a717e9d81c611ce321962f56e67fdfd6d241eb77c21a8ae32f45c347a4a  guix-build-e037c4fe0914/output/arm64-apple-darwin/bitcoin-e037c4fe0914-arm64-apple-darwin-unsigned.zip
10ba6b062dd506045cd63eedaa75e6bcfe7b50214c73b5546feb3048aa0569047f  guix-build-e037c4fe0914/output/arm64-apple-darwin/bitcoin-e037c4fe0914-arm64-apple-darwin.tar.gz
11cf99ecd355a4683ff4508d278d0ca68687869ff42a4a2625daff4946d80bdcf1  guix-build-e037c4fe0914/output/dist-archive/bitcoin-e037c4fe0914.tar.gz
129be46a24ab5e386ad7e03c9dd6a1ee8fbdc636bb3971a8c2af6147ad883cd83d  guix-build-e037c4fe0914/output/powerpc64-linux-gnu/SHA256SUMS.part
13d35c1d16465b5bdf2485c9ebf67de0aa871c5e8fb14a51ac3f27184671275246  guix-build-e037c4fe0914/output/powerpc64-linux-gnu/bitcoin-e037c4fe0914-powerpc64-linux-gnu-debug.tar.gz
14fa4658014c7a2e4b65161a0a9f0bc4ad88000f719e72ac77c89c61c970eed6f1  guix-build-e037c4fe0914/output/powerpc64-linux-gnu/bitcoin-e037c4fe0914-powerpc64-linux-gnu.tar.gz
15b29b6265fc7194bf01407daf6b3fd26af26c00c36287a48c624fc92becea80f4  guix-build-e037c4fe0914/output/riscv64-linux-gnu/SHA256SUMS.part
16f3b2818f56c03938d08b1916bcd9599bad398169d4bb06a2db7ea846db9bcb80  guix-build-e037c4fe0914/output/riscv64-linux-gnu/bitcoin-e037c4fe0914-riscv64-linux-gnu-debug.tar.gz
17479966f0c8bb21b28cc0c772e46621ae4fe434d47b1d04f50f88d290a12921fa  guix-build-e037c4fe0914/output/riscv64-linux-gnu/bitcoin-e037c4fe0914-riscv64-linux-gnu.tar.gz
18c5e2180c4464755137b9c567d93c093f28022973bf582414a10445f9264ca13b  guix-build-e037c4fe0914/output/x86_64-apple-darwin/SHA256SUMS.part
19620c6563f7639325d9e5dd96f459d1af4f667d8604d9f8f88c37ba9417eb10ca  guix-build-e037c4fe0914/output/x86_64-apple-darwin/bitcoin-e037c4fe0914-x86_64-apple-darwin-unsigned.tar.gz
209ff0d89415fd05a296265b29829e059c72befed45149b2aabac68c30b2726ff5  guix-build-e037c4fe0914/output/x86_64-apple-darwin/bitcoin-e037c4fe0914-x86_64-apple-darwin-unsigned.zip
21dbfe5885052afab52f0cf2dda2148cb0079385dc386d817c98679a50032a7252  guix-build-e037c4fe0914/output/x86_64-apple-darwin/bitcoin-e037c4fe0914-x86_64-apple-darwin.tar.gz
225e7cfa136d8c8923124f9eb1f5a84007215f4f4c44b9571d710cb64f177751a4  guix-build-e037c4fe0914/output/x86_64-linux-gnu/SHA256SUMS.part
23f6930d10a849eb313a140fd7ae9fb14c856cd9925d55e45de18fbfd14d43fe51  guix-build-e037c4fe0914/output/x86_64-linux-gnu/bitcoin-e037c4fe0914-x86_64-linux-gnu-debug.tar.gz
24d5e171d84595b80bff224ccb6d184d8433895b87098f1ada01c5d08435ca18a6  guix-build-e037c4fe0914/output/x86_64-linux-gnu/bitcoin-e037c4fe0914-x86_64-linux-gnu.tar.gz
25adc3f93db33ab431576c4a01cca8b749f05a623ff82be29c51257075b3d86d72  guix-build-e037c4fe0914/output/x86_64-w64-mingw32/SHA256SUMS.part
26c680b565970dba64b38517e00a535b5d710e5a9f2a31493dc0f2ab6285f5ad9a  guix-build-e037c4fe0914/output/x86_64-w64-mingw32/bitcoin-e037c4fe0914-win64-debug.zip
27e43572eebd4b240b9caf1a9ab2abefc4ac2770c326934a57a55a642a5bdc70a6  guix-build-e037c4fe0914/output/x86_64-w64-mingw32/bitcoin-e037c4fe0914-win64-setup-unsigned.exe
285182a532a782a209e32f08967a36adf60ae87bd4b6b08da0480d9d2e37bb1157  guix-build-e037c4fe0914/output/x86_64-w64-mingw32/bitcoin-e037c4fe0914-win64-unsigned.tar.gz
2979a502f6d2637bea6471e9f9fc27c26443bbc512b7cee0e37a46180bb2ca3f4a  guix-build-e037c4fe0914/output/x86_64-w64-mingw32/bitcoin-e037c4fe0914-win64.zip

DrahtBot requested review from theuni on Mar 24, 2024

fanquake merged this on Mar 25, 2024

fanquake closed this on Mar 25, 2024

fanquake referenced this in commit 76045bb9d6 on Mar 25, 2024

fanquake deleted the branch on Mar 25, 2024

TheCharlatan commented at 12:04 pm on March 25, 2024: contributor

Post-merge ACK e037c4fe0914d8fa9149ce7532c0d70f738e79e9

depends: always configure with --with-pic #29488

Code Coverage

Reviews

Conflicts

Guix builds (on x86_64)

depends: always configure with `--with-pic` #29488