build: Check usages of #if defined(...) #25302

brokenprogrammer · 2022-06-08T06:39:23Z

I tried to go over #16419.

Based on #16344 I interpreted that the AC_DEFINE like the following:

AC_DEFINE(USE_EXTERNAL_ASM, 1, [Define this symbol if an external (non-inline) assembly implementation is used])

Results in value defines which also #16547 suggests.

If I've missunderstood anything or if there is more #if defined or #ifdef that should be #if then please let me know and I will update the pull request.

brokenprogrammer · 2022-06-08T06:57:20Z

The linting failed because of FAIL: subtree directory was touched without subtree merge. I guess this is because I touched some external library code. Should I just revert the changes that affected secp256k1?

laanwj · 2022-06-08T07:56:27Z

The linting failed because of FAIL: subtree directory was touched without subtree merge. I guess this is because I touched some external library code. Should I just revert the changes that affected secp256k1?

Yes, it's not possible to make changes to subtrees through a PR in this repository, it needs to go through the appropriate upstream.

brokenprogrammer · 2022-06-08T08:22:54Z

The linting failed because of FAIL: subtree directory was touched without subtree merge. I guess this is because I touched some external library code. Should I just revert the changes that affected secp256k1?

Yes, it's not possible to make changes to subtrees through a PR in this repository, it needs to go through the appropriate upstream.

I see, thank you for the information. I have reverted the files for secp256k1.

fanquake · 2022-06-08T08:31:29Z

I have reverted the files for secp256k1.

You need to remove the changes entirely, no reversion commits. You'll also need to write a proper / useful commit message for the actual change. I'd suggest reading through https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md.

brokenprogrammer · 2022-06-08T09:02:00Z

I have reverted the files for secp256k1.

You need to remove the changes entirely, no reversion commits. You'll also need to write a proper / useful commit message for the actual change. I'd suggest reading through https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md.

Sorry about that, I've published a new change which I hope corrects my mistakes.

DrahtBot · 2022-06-08T15:13:04Z

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

Reviews

See the guideline for information on the review process.
A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#27607 (init: verify blocks data existence only once for all the indexers by furszy)
#27598 (bench: Add SHA256 implementation specific benchmarks by hebasto)

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.

luke-jr · 2022-06-18T16:55:28Z

AC_DEFINE only defines if it's actually called. Conditional calls to AC_DEFINE (such as those setting it to a constant 1, or not setting a value at all) need defined()/#ifdef AIUI

~~Looking through our defines, I don't see anything that requires changes.~~ Edit: was looking at an old commit

luke-jr · 2022-06-18T17:03:17Z

Correction: LevelDB is using #if defined(HAVE_O_CLOEXEC), which we are defining to 0 on systems without it - this leads to a build failure, and should be fixed.

brokenprogrammer · 2022-06-19T09:51:28Z

AC_DEFINE only defines if it's actually called. Conditional calls to AC_DEFINE (such as those setting it to a constant 1, or not setting a value at all) need defined()/#ifdef AIUI

~~Looking through our defines, I don't see anything that requires changes.~~ Edit: was looking at an old commit

With that said I think I've missunderstood the task? Should I remove all my previous changes and perhaps only address what you mentioned with HAVE_O_CLOEXEC?

fanquake · 2022-08-15T16:04:01Z

Should I remove all my previous changes and perhaps only address what you mentioned with HAVE_O_CLOEXEC?

No, that issue has now been resolved in master. However you can rebase / squash your changes here.

fanquake · 2022-10-05T10:32:37Z

@brokenprogrammer interested in following?

brokenprogrammer · 2022-10-05T12:03:29Z

@fanquake So the changes are fine and what is left is simply to merge latest master and squash the changes?

fanquake · 2023-02-13T13:45:38Z

@brokenprogrammer sorry for not following up. Changes should always be squashed. I will get to reviewing this shortly.

hebasto · 2023-02-13T15:19:30Z

src/bench/verify_script.cpp

-#if defined(HAVE_CONSENSUS_LIB)
+#if HAVE_CONSENSUS_LIB


Conditional calls to AC_DEFINE (such as those setting it to a constant 1, or not setting a value at all) need defined()/#ifdef AIUI

Agree with @luke-jr.

In this particular case, it is conditional:

bitcoin/configure.ac

Lines 1707 to 1710 in 8126551

if test "$build_bitcoin_libs" = "yes"; then

AC_DEFINE([HAVE_CONSENSUS_LIB], [1], [Define this symbol if the consensus lib has been built])

AC_CONFIG_FILES([libbitcoinconsensus.pc:libbitcoinconsensus.pc.in])

fi

therefore, the current code (in the master branch) is fine.

brokenprogrammer · 2023-02-13T16:20:10Z

@fanquake No worries, I've squashed and updated the commit message according to the contributing guidelines.

hebasto · 2023-02-14T09:43:48Z

@brokenprogrammer

I've squashed and updated the commit message according to the contributing guidelines.

A #25302 (comment) still needs to be addressed.

hebasto · 2023-02-16T15:42:14Z

~~Another bug found (introduced in #20358), which needs to be addressed.~~

UPD. False suggestion. Sorry.

TheCharlatan · 2023-04-30T20:12:37Z

src/crypto/sha256.cpp

@@ -604,7 +604,7 @@ std::string SHA256AutoDetect()
        have_x86_shani = (ebx >> 29) & 1;
    }

-#if defined(ENABLE_X86_SHANI) && !defined(BUILD_BITCOIN_INTERNAL)
+#if ENABLE_X86_SHANI && !defined(BUILD_BITCOIN_INTERNAL)


I think this is only defined conditionally:

https://github.com/bitcoin/bitcoin/blob/master/configure.ac#L569-L581

~~Edit: Sorry, I think I made a mistake here.~~
And another edit: I think I confused myself here. Shouldn't any variable that is not defined in config.status with either a "0" or "1" be checked with #if defined(...)?

DrahtBot · 2023-05-17T19:23:21Z

🐙 This pull request conflicts with the target branch and needs rebase.

hebasto · 2023-05-18T10:54:35Z

I've re-reviewed this PR and none of the suggested changes seem required to me.

More details see in #16419 (comment).

fanquake · 2023-05-18T11:06:56Z

Going to close for now. We can continue any discussion in #16419.

…MQ is not available fa5831b build: Do not define `ENABLE_ZMQ` when ZMQ is not available (Hennadii Stepanov) Pull request description: A new behavior is consistent with the other optional dependencies. The source code contains `#if ENABLE_ZMQ` lines only: ``` $ git grep ENABLE_ZMQ -- src/*.cpp src/init.cpp:#if ENABLE_ZMQ src/init.cpp:#if ENABLE_ZMQ src/init.cpp:#if ENABLE_ZMQ src/init.cpp:#if ENABLE_ZMQ src/init.cpp:#if ENABLE_ZMQ ``` Change in description line -- "Define to 1..." --> "Define this symbol.." -- is motivated by the fact that the actual value of the defined `ENABLE_ZMQ` macro does not matter at all. Related to: - bitcoin/bitcoin#16419 - bitcoin/bitcoin#25302 ACKs for top commit: TheCharlatan: ACK fa5831b jarolrod: ACK fa5831b Tree-SHA512: 5e72ff0d34c4b33205338daea0aae8d7aa0e48fd633e21af01af32b7ddb0532ef68dd3dd74deb2c1d2599691929617e8c09676bcbaaf7d669b88816f866f1db2

fanquake added the Build system label Jun 8, 2022

brokenprogrammer force-pushed the build-ifdef branch from e672edd to 8a4a796 Compare June 8, 2022 09:00

This was referenced Jun 8, 2022

indexes: Read the locator's top block during init, allow interaction with reindex-chainstate #25193

Merged

build, refactor: Rely on AC_DEFINE macro #24774

Closed

Support ignoring "opt-in" flag for RBF (aka full RBF) #25373

Closed

fanquake mentioned this pull request Feb 13, 2023

Check usages of #if defined(...) #16419

Closed

hebasto reviewed Feb 13, 2023

View reviewed changes

build: Updated value based #ifdef to use #if

d204332

brokenprogrammer force-pushed the build-ifdef branch from 6efd69a to d204332 Compare February 13, 2023 16:18

This was referenced Feb 16, 2023

src/randomenv.cpp: fix build on uclibc #20358

Merged

build: Add CMake-based build system (2 of N) hebasto/bitcoin#6

Merged

achow101 requested review from theuni, TheCharlatan and theStack April 25, 2023 16:21

achow101 requested a review from hebasto April 25, 2023 16:21

TheCharlatan reviewed Apr 30, 2023

View reviewed changes

DrahtBot mentioned this pull request May 9, 2023

bench: Add SHA256 implementation specific benchmarks #27598

Merged

DrahtBot mentioned this pull request May 17, 2023

index: make startup more efficient #27607

Merged

DrahtBot added the Needs rebase label May 17, 2023

hebasto mentioned this pull request May 18, 2023

build: Do not define ENABLE_ZMQ when ZMQ is not available #27696

Merged

fanquake closed this May 18, 2023

bitcoin locked and limited conversation to collaborators May 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: Check usages of #if defined(...) #25302

build: Check usages of #if defined(...) #25302

brokenprogrammer commented Jun 8, 2022

brokenprogrammer commented Jun 8, 2022

laanwj commented Jun 8, 2022

brokenprogrammer commented Jun 8, 2022

fanquake commented Jun 8, 2022

brokenprogrammer commented Jun 8, 2022

DrahtBot commented Jun 8, 2022 •

edited

Loading

luke-jr commented Jun 18, 2022 •

edited

Loading

luke-jr commented Jun 18, 2022

brokenprogrammer commented Jun 19, 2022

fanquake commented Aug 15, 2022

fanquake commented Oct 5, 2022

brokenprogrammer commented Oct 5, 2022

fanquake commented Feb 13, 2023

hebasto Feb 13, 2023 •

edited

Loading

brokenprogrammer commented Feb 13, 2023

hebasto commented Feb 14, 2023

hebasto commented Feb 16, 2023 •

edited

Loading

TheCharlatan Apr 30, 2023 •

edited

Loading

DrahtBot commented May 17, 2023

hebasto commented May 18, 2023

fanquake commented May 18, 2023

	if test "$build_bitcoin_libs" = "yes"; then
	AC_DEFINE([HAVE_CONSENSUS_LIB], [1], [Define this symbol if the consensus lib has been built])
	AC_CONFIG_FILES([libbitcoinconsensus.pc:libbitcoinconsensus.pc.in])
	fi

build: Check usages of #if defined(...) #25302

build: Check usages of #if defined(...) #25302

Conversation

brokenprogrammer commented Jun 8, 2022

brokenprogrammer commented Jun 8, 2022

laanwj commented Jun 8, 2022

brokenprogrammer commented Jun 8, 2022

fanquake commented Jun 8, 2022

brokenprogrammer commented Jun 8, 2022

DrahtBot commented Jun 8, 2022 • edited Loading

Reviews

Conflicts

luke-jr commented Jun 18, 2022 • edited Loading

luke-jr commented Jun 18, 2022

brokenprogrammer commented Jun 19, 2022

fanquake commented Aug 15, 2022

fanquake commented Oct 5, 2022

brokenprogrammer commented Oct 5, 2022

fanquake commented Feb 13, 2023

hebasto Feb 13, 2023 • edited Loading

Choose a reason for hiding this comment

brokenprogrammer commented Feb 13, 2023

hebasto commented Feb 14, 2023

hebasto commented Feb 16, 2023 • edited Loading

TheCharlatan Apr 30, 2023 • edited Loading

Choose a reason for hiding this comment

DrahtBot commented May 17, 2023

hebasto commented May 18, 2023

fanquake commented May 18, 2023

DrahtBot commented Jun 8, 2022 •

edited

Loading

luke-jr commented Jun 18, 2022 •

edited

Loading

hebasto Feb 13, 2023 •

edited

Loading

hebasto commented Feb 16, 2023 •

edited

Loading

TheCharlatan Apr 30, 2023 •

edited

Loading