Makefile and build fixes#8355
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Files
|
📝 WalkthroughWalkthroughUpdated Makefile lint invocation to call golangci-lint from GOPATH bin; changed Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crypto_adx_flag.mk`:
- Around line 26-33: The Makefile is omitting the C_STD_FLAG when building
non-ADX images; update the build flags so DISABLE_ADX and targets that pass
CGO_FLAG include the C standard flag. Either (A) append C_STD_FLAG to the
DISABLE_ADX variable (and adjust CRYPTO_FLAG's non-ADX branch to avoid
duplicating C_STD_FLAG), or (B) modify each docker-build-*-without-adx and
docker-build-*-without-netgo-without-adx target to pass CGO_FLAG as
"$(C_STD_FLAG) $(DISABLE_ADX)"; ensure you update references to DISABLE_ADX,
C_STD_FLAG and CRYPTO_FLAG so builds get "-std=gnu17" along with the portable
ADX disable flags.
| # Then, set `CRYPTO_FLAG` | ||
| # the crypto package uses BLST source files underneath which may use ADX instructions. | ||
| ifeq ($(ADX_SUPPORT), 1) | ||
| # if ADX instructions are supported on the current machine, default is to use a fast ADX implementation | ||
| CRYPTO_FLAG := "" | ||
| # if ADX instructions are supported on the current machine, default is to use a fast ADX implementation | ||
| CRYPTO_FLAG := "$(C_STD_FLAG)" | ||
| else | ||
| # if ADX instructions aren't supported, this CGO flags uses a slower non-ADX implementation | ||
| CRYPTO_FLAG := $(DISABLE_ADX) | ||
| # if ADX instructions aren't supported, this CGO flags uses a slower non-ADX implementation | ||
| CRYPTO_FLAG := "$(C_STD_FLAG) $(DISABLE_ADX)" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check structure of the repo
cd /tmp && git ls-files | head -20Repository: onflow/flow-go
Length of output: 126
🏁 Script executed:
#!/bin/bash
# Find Makefile
find . -name "Makefile" -o -name "*.mk" | head -20Repository: onflow/flow-go
Length of output: 224
🏁 Script executed:
#!/bin/bash
# Verify which targets use DISABLE_ADX directly
echo "=== Checking for DISABLE_ADX definition ==="
rg -n 'DISABLE_ADX\s*:=' . --type makefile
echo ""
echo "=== Checking for CGO_FLAG=$(DISABLE_ADX) usage ==="
rg -n 'CGO_FLAG.*DISABLE_ADX' . --type makefile
echo ""
echo "=== Checking for CRYPTO_FLAG usage in docker-build targets ==="
rg -n 'docker-build.*ADX' . --type makefile -A 3Repository: onflow/flow-go
Length of output: 369
🏁 Script executed:
#!/bin/bash
# Get context around CRYPTO_FLAG and DISABLE_ADX definitions
echo "=== Lines around CRYPTO_FLAG definition (lines 26-35) ==="
sed -n '20,40p' crypto_adx_flag.mk
echo ""
echo "=== Looking at entire crypto_adx_flag.mk ==="
wc -l crypto_adx_flag.mk
cat -n crypto_adx_flag.mkRepository: onflow/flow-go
Length of output: 2574
🏁 Script executed:
#!/bin/bash
# Search for all docker-build-*-without-adx targets
echo "=== All docker-build targets with 'without-adx' ==="
rg -n 'docker-build.*without-adx' Makefile -A 5 | head -100Repository: onflow/flow-go
Length of output: 8365
🏁 Script executed:
#!/bin/bash
# Count exact matches of CGO_FLAG=$(DISABLE_ADX) pattern
echo "=== Exact matches of CGO_FLAG=\$(DISABLE_ADX) ==="
rg 'CGO_FLAG=\$\(DISABLE_ADX\)' Makefile -n -c
rg 'CGO_FLAG=\$\(DISABLE_ADX\)' Makefile -nRepository: onflow/flow-go
Length of output: 3156
Update Makefile targets to include C_STD_FLAG when using DISABLE_ADX.
The docker-build-*-without-adx and docker-build-*-without-netgo-without-adx targets pass CGO_FLAG=$(DISABLE_ADX) directly, which only includes -O2 -D__BLST_PORTABLE__ but omits -std=gnu17. This means 13 targets will fail on GCC 15+ which defaults to C23.
Consider either:
- Adding
C_STD_FLAGto theDISABLE_ADXdefinition, or - Updating those Makefile targets to use a combined flag
Option 1: Include C_STD_FLAG in DISABLE_ADX
# Flags to disable ADX instructions for older CPUs
-DISABLE_ADX := -O2 -D__BLST_PORTABLE__
+DISABLE_ADX := $(C_STD_FLAG) -O2 -D__BLST_PORTABLE__Then update the CRYPTO_FLAG assignment for non-ADX case to avoid duplication:
# if ADX instructions aren't supported, this CGO flags uses a slower non-ADX implementation
- CRYPTO_FLAG := "$(C_STD_FLAG) $(DISABLE_ADX)"
+ CRYPTO_FLAG := "$(DISABLE_ADX)"Affected targets (12 docker-build targets + 1 docker-build-util): collection, consensus, execution, verification, access, and observer (each with both -without-adx and -without-netgo-without-adx variants).
🤖 Prompt for AI Agents
In `@crypto_adx_flag.mk` around lines 26 - 33, The Makefile is omitting the
C_STD_FLAG when building non-ADX images; update the build flags so DISABLE_ADX
and targets that pass CGO_FLAG include the C standard flag. Either (A) append
C_STD_FLAG to the DISABLE_ADX variable (and adjust CRYPTO_FLAG's non-ADX branch
to avoid duplicating C_STD_FLAG), or (B) modify each docker-build-*-without-adx
and docker-build-*-without-netgo-without-adx target to pass CGO_FLAG as
"$(C_STD_FLAG) $(DISABLE_ADX)"; ensure you update references to DISABLE_ADX,
C_STD_FLAG and CRYPTO_FLAG so builds get "-std=gnu17" along with the portable
ADX disable flags.
|
Thanks for flagging this. I think we should fix this from within the crypto library. |
|
I added a general |
|
I moved the fix to the crypto library (onflow/crypto#38) where the issue comes from, instead of the application layer. In general, I wanted to abstract the C build details from flow-go. In the future, you can find the CFLAGS of the crypto build defined here, and the flags can be adjusted if needed. |
|
Thanks @tarakby. I can't approve, but it looks good to me. |
6 participants
I had some issue on my setup and I added fixes.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.