feat: use golang 1.25.0 #1927
feat: use golang 1.25.0 #1927
Conversation
WalkthroughBumps Go toolchains to 1.25.x and upgrades nixpkgs to release-25.11 across CI and Nix; adds a gomod-overlay/gomod2nix wiring and go_1_25 derivation; updates ethermint pins; adds TypeScript tooling for integration tests; and adjusts macOS SDK inputs and several package/version hashes. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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 |
songgaoye
force-pushed
the
song/go_1.25.0
branch
from
1270626 to
ccc4fd3
Compare
songgaoye
force-pushed
the
song/go_1.25.0
branch
2 times, most recently
from
d78ab09 to
94eb5dc
Compare
songgaoye
force-pushed
the
song/go_1.25.0
branch
from
94eb5dc to
f42e703
Compare
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
songgaoye
force-pushed
the
song/go_1.25.0
branch
from
f42e703 to
d78ab09
Compare
songgaoye
requested review from
calvinaco and
thomas-nguy
and removed request for
a team
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
nix/default.nix (1)
33-45: Duplicate gomodOverlay pattern.This is the same pattern as in
flake.nix. As noted there, consider extracting to a shared module.
🧹 Nitpick comments (5)
integration_tests/contracts/tsconfig.json (1)
1-11: TS compiler options are sensible for Hardhat; consider stricter settings laterThis tsconfig is reasonable for a Hardhat/Node test harness (ES2020 + CommonJS,
esModuleInterop,skipLibCheck). Once things stabilize, you might optionally:
- Flip
"strict"totrueto catch more type issues.- Add explicit
"exclude"fornode_modules,artifacts,cacheto keep the TS project surface small.These are nice-to-have and not blockers for this PR.
.github/workflows/lint.yml (1)
26-26: Update actions/checkout to v4.The linting workflow uses
actions/checkout@v3, which the static analysis tool flags as too old for modern GitHub Actions. Consider updating tov4for consistency with other workflows in this PR (e.g., dependabot-update-all.yml already uses v4).- uses: actions/checkout@v3 + - uses: actions/checkout@v4 with: fetch-depth: 0go.mod (1)
3-3: Verify codebase compatibility with Go 1.25 behavioral changes.Go 1.25 introduces a fix for a compiler bug (present since 1.21) that could cause programs using a result before checking the error to now correctly panic. Review the codebase for patterns like:
f, err := os.Open("file") name := f.Name() // This will now panic if err != nil if err != nil { ... }Additionally, go1.25.5 (released 2025-12-02) includes two security fixes to the crypto/x509 package. Consider updating the toolchain directive to
go 1.25.5to include the latest security patches.flake.nix (1)
95-99: Consider extracting the duplicated gomodOverlay pattern.The gomodOverlay logic defined here (lines 32-43) is duplicated in
nix/default.nix(lines 33-45). If both files need this pattern, consider extracting it to a shared module (e.g.,nix/gomod-overlay.nix) to reduce maintenance burden and ensure consistency.nix/default.nix (1)
13-14: Inconsistent overlay parameter naming:finalvspkgs.The overlay uses
finalfor the first parameter but the body referencespkgs(the second parameter, which isprev/super). This is semantically confusing sincefinaltypically refers to the final fixed-point andpkgs/prevrefers to the previous overlay result.For clarity, consider using consistent naming:
- (final: pkgs: { - go = final.go_1_25; + (final: prev: { + go = final.go_1_25; go-ethereum = pkgs.callPackage ./go-ethereum.nix { - buildGoModule = pkgs.buildGoModule; + buildGoModule = final.buildGoModule; };Alternatively, keep the
_: pkgs:pattern used elsewhere in this file ifprevisn't needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
flake.lockis excluded by!**/*.lockgo.sumis excluded by!**/*.sumintegration_tests/contracts/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (27)
.github/workflows/build.yml(4 hunks).github/workflows/codeql-analysis.yml(1 hunks).github/workflows/dependabot-update-all.yml(1 hunks).github/workflows/dependencies-review.yml(1 hunks).github/workflows/lint.yml(3 hunks).github/workflows/release.yml(2 hunks).github/workflows/sims.yml(6 hunks).github/workflows/test.yml(4 hunks).gitignore(1 hunks)Makefile(1 hunks)default.nix(3 hunks)flake.nix(3 hunks)go.mod(2 hunks)gomod2nix.toml(1 hunks)integration_tests/contracts/package.json(1 hunks)integration_tests/contracts/tsconfig.json(1 hunks)integration_tests/pytest.ini(1 hunks)integration_tests/test_basic.py(1 hunks)nix/build_overlay.nix(1 hunks)nix/default.nix(3 hunks)nix/go-ethereum.nix(2 hunks)nix/golangci-lint.nix(1 hunks)nix/hermes.nix(2 hunks)nix/rocksdb.nix(3 hunks)nix/sources.json(2 hunks)nix/testenv.nix(2 hunks)scripts/run-integration-tests(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.9)
.github/workflows/lint.yml
26-26: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (200)
- GitHub Check: build (macos-latest)
- GitHub Check: unittest
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-14)
- GitHub Check: gomod2nix
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (mint)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (evm)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: Lint python
- GitHub Check: Run golangci-lint
- GitHub Check: build (macos-latest)
- GitHub Check: unittest
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-14)
- GitHub Check: gomod2nix
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (mint)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (evm)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: Lint python
- GitHub Check: Run golangci-lint
- GitHub Check: build (macos-latest)
- GitHub Check: unittest
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-14)
- GitHub Check: gomod2nix
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (mint)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (evm)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: Lint python
- GitHub Check: Run golangci-lint
- GitHub Check: build (macos-latest)
- GitHub Check: unittest
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-14)
- GitHub Check: gomod2nix
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (mint)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (evm)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: Lint python
- GitHub Check: Run golangci-lint
- GitHub Check: build (macos-latest)
- GitHub Check: unittest
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-14)
- GitHub Check: gomod2nix
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (mint)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (evm)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: Lint python
- GitHub Check: Run golangci-lint
- GitHub Check: build (macos-latest)
- GitHub Check: unittest
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-14)
- GitHub Check: gomod2nix
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (mint)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (evm)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: Lint python
- GitHub Check: Run golangci-lint
- GitHub Check: build (macos-latest)
- GitHub Check: unittest
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-14)
- GitHub Check: gomod2nix
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (mint)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (evm)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: Lint python
- GitHub Check: Run golangci-lint
- GitHub Check: build (macos-latest)
- GitHub Check: unittest
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-14)
- GitHub Check: gomod2nix
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (mint)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (evm)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: Lint python
- GitHub Check: Run golangci-lint
- GitHub Check: build (macos-latest)
- GitHub Check: unittest
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-14)
- GitHub Check: gomod2nix
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (mint)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (evm)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: Lint python
- GitHub Check: Run golangci-lint
- GitHub Check: build (macos-latest)
- GitHub Check: unittest
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-14)
- GitHub Check: gomod2nix
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (mint)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (evm)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: Lint python
- GitHub Check: Run golangci-lint
🔇 Additional comments (34)
integration_tests/test_basic.py (1)
114-121: Verify that the new gas expectation reflects actual execution with Go 1.25.0.The expected gas usage was updated to 564724. While this change is plausible given the Go version upgrade mentioned in the PR, there is no evidence that these tests were executed with Go 1.25.0 to validate the new value.
Please confirm:
- Was this test executed with Go 1.25.0, and does it consistently produce a
gasUsedof 564724?- What caused the gas increase? (e.g., compiled bytecode differences, runtime changes, ethermint version updates)
- Only one other hard-coded
exp_gas_usedwas found in the test suite (test_pruned_node.py:49); are there any other integration tests that may need similar updates?integration_tests/pytest.ini (1)
4-17: Pytest marker definitions are clear and consistent with integration test usageRegistering these markers avoids
PytestUnknownMarkWarningand gives a good taxonomy for selecting subsets (ibc, gov, upgrade, gas, etc.). This looks correct and well-scoped.scripts/run-integration-tests (1)
10-10: Switch tonpm ciimproves determinism; ensure lockfile is present and maintainedUsing
npm cihere is good for reproducible, clean installs, especially under CI. Just make sureintegration_tests/contractshas a committed, up‑to‑date lockfile (e.g.,package-lock.json), otherwise this step will now fail wherenpm installpreviously succeeded.integration_tests/contracts/package.json (1)
12-20: Hardhat + TypeScript tooling additions align with the existing typechain workflowAdding
hardhatplusts-node/typescriptis consistent with thenpx hardhat typechainscript and the new TS config. From a config perspective this looks correct; just ensure:
- The Node version used in
integration_tests/shell.nixmatches the supported range for this Hardhat + TypeScript combo.- The lockfile in this directory is regenerated so CI’s
npm cipicks up these exact versions..github/workflows/dependencies-review.yml (2)
18-18: Version bump looks good.The go-version upgrade from 1.24 to 1.25 is straightforward and properly configured.
39-45: Hardcoded vulnerability exception GO-2025-3443 is justified and properly tracked.This exception references a real CometBFT vulnerability (network stalling via malicious peers) that was fixed in v0.38.17 and v1.0.1. The fix was applied in PR #1736 (Dec 19, 2024), and the codebase currently uses v0.38.19, which contains the patch. The exception is documented in both the CHANGELOG and the workflow comment. No action required.
.github/workflows/codeql-analysis.yml (1)
47-47: Go version upgrade looks good.The version constraint from
^1.24.0to^1.25.0is appropriately updated and allows patch releases..gitignore (1)
28-28: Git ignore pattern added.The
chain-main-*pattern correctly excludes chain-main build artifacts from version control, aligning with Nix build tooling changes elsewhere in the PR..github/workflows/build.yml (1)
25-25: Nixpkgs channel upgrade applied consistently.All four jobs (build, unittest, gomod2nix, contracts) are updated from
nixos-22.11tonixos-25.11consistently.Also applies to: 58-58, 108-108, 142-142
.github/workflows/release.yml (1)
19-19: Nixpkgs channel upgrade applied to release workflows.Both release jobs (Linux and macOS) properly updated from
nixos-22.11tonixos-25.11.Also applies to: 60-60
.github/workflows/lint.yml (1)
25-25: Go and Nixpkgs versions properly updated.Go version upgraded to 1.25.0, and nixpkgs channel updated to nixos-25.11 across all three linting jobs (golangci, lint-python, lint-nix).
Also applies to: 29-29, 77-77, 103-103
.github/workflows/dependabot-update-all.yml (1)
29-29: Go version upgrade looks good.The go-version is properly updated from
1.24to1.25for dependency updates.nix/sources.json (2)
101-110: Nixpkgs source pinning updated to release-25.11.The nixpkgs entry is properly updated with:
- Branch:
release-25.11- New revision:
f27d6578053dfba71ffe5156a8274a552340b730- Updated SHA256 hash and tarball URL
Ensure these values match the official release-25.11 branch upstream before merging.
118-122: Poetry2nix entry updated.Poetry2nix is updated to revision
ce2369db77f45688172384bbeb962bc6c2ea6f94with matching SHA256 hash. This appears consistent with the broader tooling updates.nix/rocksdb.nix (2)
14-14: Formatting looks good.The inline comment referencing the nixpkgs issue for the jemalloc condition is helpful for maintainability.
103-123: Clean reformatting of shell script blocks.The preInstall and postFixup blocks are now properly formatted with cleaner multi-line string handling. The logic remains unchanged.
.github/workflows/test.yml (2)
40-40: Nix channel update aligns with the Go toolchain upgrade.The nixos-25.11 channel is consistent with the PR's goal of modernizing the build environment.
67-67: Go 1.25 for coverage tooling.The coverage tool update to
go_1_25aligns with the rest of the toolchain upgrade. Ensure integration tests pass to validate compatibility.go.mod (1)
306-306: Ethermint dependency update looks consistent.The ethermint fork version is updated to a newer pseudo-version and is synchronized with the gomod2nix.toml entry.
gomod2nix.toml (1)
327-329: Ethermint mapping correctly synchronized with go.mod.The version and hash updates are consistent with the replace directive in go.mod, ensuring the Nix build will use the correct ethermint fork version.
.github/workflows/sims.yml (2)
111-113: Consistent Go 1.25 upgrade across simulation tests.The
^1.25.0semver constraint allows automatic patch updates, which is good for picking up security fixes while maintaining compatibility.
132-134: Runsim installation job updated.The Go version is consistently updated to 1.25.0.
nix/hermes.nix (1)
10-10: LGTM! Darwin SDK migration looks correct.The switch to
apple-sdk_15consolidates the individual framework dependencies (Security, SystemConfiguration) into the unified SDK approach used in nixpkgs 25.11. Retainingdarwin.libiconvseparately is appropriate since it's not part of the SDK.Also applies to: 48-51
nix/go-ethereum.nix (1)
6-6: LGTM! Consistent Darwin SDK update.The migration to
apple-sdk_15aligns with the nixpkgs 25.11 changes and follows the same pattern as other Nix files in this PR. The comment documenting the rationale is appreciated.Also applies to: 59-63
nix/testenv.nix (2)
7-30: LGTM! Proper rpds-py Rust build override.The custom override scope correctly handles
rpds-pysource builds by adding Cargo dependencies and Rust tooling. The conditional check!(old.src.isWheel or false)ensures this only applies to non-wheel sources.Verify the cargo vendor hash is still valid if
rpds-pyversion changes in the future. A version bump would require updating the hash on line 18.
61-74: LGTM! License format normalization for pyproject.toml.The postPatch scripts handle the PEP 639 license format migration by removing
license-filesand converting the license string to the object format.default.nix (2)
5-5: LGTM! Explicit Go toolchain wiring.Adding
goto inputs and inheriting it in the derivation ensures the build uses the explicitly configuredgo_1_25from the overlay rather than relying on implicit resolution.Also applies to: 43-43
60-60: LGTM! Required for gomod2nix configuration.The
modRoot = "."setting aligns with the updated gomod overlay configuration per the AI summary.nix/build_overlay.nix (1)
41-76: LGTM! Reasonable workaround for Darwin solc builds.The Darwin-specific workaround using pre-built macOS binaries is a pragmatic solution for the nixpkgs 25.11 SDK compatibility issues. The fallback to
super.solc-static-versionson Linux maintains expected behavior. The two versions defined (solc_0_6_8andsolc_0_8_21) are exactly those referenced in the project's nix files (contracts/shell.nixandnix/gen-binding-shell.nix), so the workaround is complete and sufficient.flake.nix (2)
32-43: LGTM on the gomodOverlay pattern with modRoot workaround.The
modRoot = "."override addresses the gomod2nix builder issue where a null modRoot causes build failures. This approach correctly exposesbuildGoApplication,mkGoEnv, andmkVendorEnvwhile applying the necessary fix.
3-3: Be aware of known Go 1.25 compatibility issues on macOS.The
release-25.11branch andgo_1_25package are both available in nixpkgs. However, Go 1.25 has known linking failures on macOS (macOS 15.6+) and CGO behavior changes (CGO_ENABLED=1). Test thoroughly on macOS if that's a supported platform.Likely an incorrect or invalid review comment.
nix/default.nix (3)
19-30: Good approach for chain-maind Go version compatibility.The mapping of
go_1_23togo_1_25viabuildPackagesoverride maintains compatibility with chain-main's expected interface while upgrading the actual toolchain. ThemodRoot = "."fix with the linked PR reference provides good traceability.
85-85: Correct update frombuildGo123ModuletobuildGoModule.Using the generic
buildGoModuleis appropriate here since the Go version is now set at the overlay level viago = final.go_1_25. This avoids hardcoding Go version in individual package builds.
67-69: apple-sdk_15 is available in nixpkgs 25.11.Verified that
apple-sdk_15is defined in all-packages.nix ascallPackage ../by-name/ap/apple-sdk/package.nix { darwinSdkMajorVersion = "15"; }. This consolidation of Darwin framework dependencies toapple-sdk_15follows the correct pattern for nixpkgs 25.11.
|
|
||
| lint-nix: | ||
| find . -name "*.nix" ! -path './integration_tests/contracts/*' ! -path "./contracts/*" | xargs nixfmt -c | ||
| find . \( -path './integration_tests/contracts' -o -path './contracts' -o -path './chain-main-*' \) -prune -o -name "*.nix" -print | xargs nixfmt -c |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Excellent refactor—use of -prune is more efficient and idiomatic.
The changes to lint-nix and lint-nix-fix improve the find command by using the -prune predicate to exclude directories before traversing them, rather than filtering results downstream. This reduces filesystem I/O and is the idiomatic approach in find(1).
The syntax is correct: grouping multiple -path patterns with -o, followed by -prune, then -o -name "*.nix" -print to match .nix files in non-pruned directories. The paths being excluded (integration_tests/contracts, contracts, chain-main-*) are sensible for the project.
Also applies to: 143-143
🤖 Prompt for AI Agents
Makefile around lines 140 and 143: apply the same -prune-based find invocation
to both lint-nix and lint-nix-fix targets so they consistently exclude
./integration_tests/contracts, ./contracts and ./chain-main-* before traversal;
replace the previous post-filtering find pipeline with a single find that groups
the three -path patterns joined by -o, then -prune, then -o -name "*.nix"
-print, and pass those results to nixfmt -c (and to nixfmt for the fix target)
to reduce filesystem I/O and match the approved refactor.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
nix/build_overlay.nix (1)
31-36: Use--replace-failfor safer builds.The previous review suggested using
--replace-failto ensure the build fails if the patterns aren't found, rather than silently succeeding. This hasn't been addressed yet.postPatch = (old.postPatch or "") + '' substituteInPlace src/net/lookup_unix.go \ - --replace 'open("/etc/protocols")' 'open("${final.iana-etc}/etc/protocols")' + --replace-fail 'open("/etc/protocols")' 'open("${final.iana-etc}/etc/protocols")' substituteInPlace src/net/port_unix.go \ - --replace 'open("/etc/services")' 'open("${final.iana-etc}/etc/services")' + --replace-fail 'open("/etc/services")' 'open("${final.iana-etc}/etc/services")' '';
🧹 Nitpick comments (1)
nix/build_overlay.nix (1)
46-59: Consider extracting common derivation logic to reduce duplication.Both
solc_0_6_8andsolc_0_8_21share nearly identical structure. A helper function would improve maintainability if more versions are needed.# Example refactor using a helper function: solc-static-versions = if final.stdenv.isDarwin then let mkSolc = version: sha256: final.stdenv.mkDerivation { pname = "solc"; inherit version; src = final.fetchurl { url = "https://github.com/ethereum/solidity/releases/download/v${version}/solc-macos"; inherit sha256; }; dontUnpack = true; installPhase = '' mkdir -p $out/bin cp $src $out/bin/solc-${version} chmod +x $out/bin/solc-${version} ''; }; in { solc_0_6_8 = mkSolc "0.6.8" "sha256-W0qCSwXC9nb1a6pRKPeczT9RZ+1RpVMVCD/vl8xEaEc="; solc_0_8_21 = mkSolc "0.8.21" "sha256-mzXHa30o2kCPW9XhBfr0XsCXS0+7VdPN3bZ4pJG3PU0="; } else super.solc-static-versions;Also applies to: 60-73
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.github/workflows/codeql-analysis.yml(1 hunks).github/workflows/dependabot-update-all.yml(1 hunks).github/workflows/dependencies-review.yml(1 hunks).github/workflows/lint.yml(3 hunks).github/workflows/sims.yml(6 hunks)Makefile(2 hunks)go.mod(2 hunks)nix/build_overlay.nix(1 hunks)nix/golangci-lint.nix(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- go.mod
- .github/workflows/codeql-analysis.yml
- .github/workflows/dependabot-update-all.yml
- .github/workflows/dependencies-review.yml
- Makefile
- .github/workflows/sims.yml
- nix/golangci-lint.nix
🧰 Additional context used
🪛 actionlint (1.7.9)
.github/workflows/lint.yml
26-26: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Run golangci-lint
- GitHub Check: build (ubuntu-latest)
- GitHub Check: unittest
- GitHub Check: Lint python
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-14)
- GitHub Check: gomod2nix
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (2)
.github/workflows/lint.yml (1)
25-25: Go version is only updated in the golangci job; nixos-25.11 is consistent across all three lint jobs.The go-version setup (1.25.5) appears only in the golangci job (line 25) via
actions/setup-go, which is correct since only that job compiles and lints Go code. The lint-python and lint-nix jobs use nix-shell environments instead and do not require explicit Go setup in the workflow.The nixos-25.11 channel is consistently applied across all three jobs (lines 29, 77, 103). The nix overlay in
nix/default.nixalready specifiesgo = final.go_1_25, andnix/build_overlay.nixdefines the Go 1.25.5 build itself, ensuring consistency via the nix package definitions rather than workflow duplication.Likely an incorrect or invalid review comment.
nix/build_overlay.nix (1)
12-21: Go 1.25.5 update looks good.The version has been updated to 1.25.5 with the corresponding hash, and the comments clearly explain why
go_1_24is used as the base (Windows platform support). The patch filtering foriana-etc-1.17.patchis a reasonable approach.
| with: | ||
| go-version: '1.22.7' | ||
| go-version: '1.25.5' | ||
| - uses: actions/checkout@v3 |
There was a problem hiding this comment.
Update actions/checkout to v4.
The actions/checkout@v3 action has been flagged as outdated by actionlint and is no longer actively maintained. Migrate to actions/checkout@v4 to ensure security patches and GitHub Actions compatibility.
Apply this diff to update the action:
- - uses: actions/checkout@v3
+ - uses: actions/checkout@v4📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - uses: actions/checkout@v3 | |
| - uses: actions/checkout@v4 |
🧰 Tools
🪛 actionlint (1.7.9)
26-26: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🤖 Prompt for AI Agents
.github/workflows/lint.yml around line 26: the workflow uses actions/checkout@v3
which is outdated; update the checkout action to actions/checkout@v4 by
replacing the version reference on that line, then commit the change and run
your CI/linter to verify there are no remaining actionlint warnings.
| solc-static-versions = | ||
| if final.stdenv.isDarwin then | ||
| { | ||
| solc_0_6_8 = final.stdenv.mkDerivation rec { | ||
| pname = "solc"; | ||
| version = "0.6.8"; | ||
| src = final.fetchurl { | ||
| url = "https://github.com/ethereum/solidity/releases/download/v${version}/solc-macos"; | ||
| sha256 = "sha256-W0qCSwXC9nb1a6pRKPeczT9RZ+1RpVMVCD/vl8xEaEc="; | ||
| }; | ||
| dontUnpack = true; | ||
| installPhase = '' | ||
| mkdir -p $out/bin | ||
| cp $src $out/bin/solc-0.6.8 | ||
| chmod +x $out/bin/solc-0.6.8 | ||
| ''; | ||
| }; | ||
| solc_0_8_21 = final.stdenv.mkDerivation rec { | ||
| pname = "solc"; | ||
| version = "0.8.21"; | ||
| src = final.fetchurl { | ||
| url = "https://github.com/ethereum/solidity/releases/download/v${version}/solc-macos"; | ||
| sha256 = "sha256-mzXHa30o2kCPW9XhBfr0XsCXS0+7VdPN3bZ4pJG3PU0="; | ||
| }; | ||
| dontUnpack = true; | ||
| installPhase = '' | ||
| mkdir -p $out/bin | ||
| cp $src $out/bin/solc-0.8.21 | ||
| chmod +x $out/bin/solc-0.8.21 | ||
| ''; | ||
| }; | ||
| } | ||
| else | ||
| super.solc-static-versions; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check available solc-macos binaries for the specified versions
# by examining GitHub release assets
# Function to safely fetch GitHub API info
check_release_assets() {
local version=$1
echo "=== Checking solc v${version} release assets ==="
# Using curl instead of gh CLI for better compatibility
curl -s "https://api.github.com/repos/ethereum/solidity/releases/tags/v${version}" \
| jq -r '.assets[] | select(.name | contains("macos")) | .name' 2>/dev/null || echo "Failed to fetch or no macos assets"
echo ""
}
check_release_assets "0.6.8"
check_release_assets "0.8.21"Repository: crypto-org-chain/cronos
Length of output: 227
🏁 Script executed:
#!/bin/bash
# Try fetching raw JSON output from GitHub API with better error handling
echo "=== Fetching solc 0.6.8 release info ==="
curl -s "https://api.github.com/repos/ethereum/solidity/releases/tags/v0.6.8" | jq '.assets | length' 2>/dev/null && \
curl -s "https://api.github.com/repos/ethereum/solidity/releases/tags/v0.6.8" | jq -r '.assets[].name' 2>/dev/null | head -20
echo ""
echo "=== Fetching solc 0.8.21 release info ==="
curl -s "https://api.github.com/repos/ethereum/solidity/releases/tags/v0.8.21" | jq '.assets | length' 2>/dev/null && \
curl -s "https://api.github.com/repos/ethereum/solidity/releases/tags/v0.8.21" | jq -r '.assets[].name' 2>/dev/null | head -20Repository: crypto-org-chain/cronos
Length of output: 154
🌐 Web query:
solc 0.6.8 0.8.21 macos binary architecture arm64 aarch64 apple silicon
💡 Result:
Short answer: Official macOS Apple‑Silicon (arm64 / aarch64) solc binaries do NOT exist for v0.6.8 or v0.8.21. Official macOS releases only added Apple‑Silicon support in v0.8.24, so older macOS solc builds are x86_64 (Intel) and must be run under Rosetta or recompiled for arm64. [1][2]
Options
- Run the shipped macOS x86_64 solc under Rosetta (install Rosetta, then use the Intel binary). (solc-select FAQ / docs). [1]
- Use Docker (run an official solc image) — isolates architecture differences. [2]
- Use solc-js / the WASM builds from solc-bin (works on Apple Silicon in JS environments). [3]
- Build solc from source on your Apple Silicon machine to produce an arm64 binary (or use a community arm64 Linux build if you target Linux). [2][4]
References
- solc-select (notes about macOS Intel-only for <0.8.24 / Rosetta). [1]
- Official Solidity releases (Apple Silicon support introduced in v0.8.24). [2]
- solc-bin / solc-js distribution (WASM/js builds). [3]
- community arm64 builds (example: 0xidm arm64 builds for Linux — not macOS official). [4]
If you want, I can:
- give exact commands to run an x86_64 solc under Rosetta on macOS, or
- show how to build v0.6.8 or v0.8.21 from source for arm64, or
- fetch a specific download URL for the Intel macOS binary for those versions. Which do you want?
🌐 Web query:
solidity solc-macos x86_64 arm64 release downloads GitHub
💡 Result:
You can download macOS solc binaries (x86_64 and Apple Silicon/arm64) from the Solidity GitHub Releases page — recent releases include a macOS asset (a universal/fat binary named solc-macos or solc-macos-{version}) that runs on both Intel and ARM Macs. See the official releases list and the macOS/Apple‑Silicon note in the Solidity release announcement. [1][2]
Quick links:
- Solidity GitHub Releases (download assets per tag). [1]
- Solidity release announcement noting macOS universal binary (Apple Silicon support). [2]
If you want, tell me which Solidity version (or architecture) you need and I’ll fetch the direct asset download link for that release.
Add a comment documenting Rosetta 2 requirement for Apple Silicon.
The solc-macos binaries for v0.6.8 and v0.8.21 are x86_64-only; official Apple Silicon (arm64) binaries were not released until v0.8.24. On Apple Silicon Macs, these binaries will run under Rosetta 2 translation. Add a comment clarifying this requirement, or consider using alternative solutions (Docker, WASM builds) for platforms that cannot use Rosetta 2.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
nix/build_overlay.nix (2)
32-35: Apply--replace-failas suggested in the previous review.The past review comment suggested using
--replace-failin thesubstituteInPlacecalls to ensure build failure if the expected patterns are not found. This recommendation has not been implemented.Apply this diff to add
--replace-fail:postPatch = (old.postPatch or "") + '' substituteInPlace src/net/lookup_unix.go \ - --replace 'open("/etc/protocols")' 'open("${final.iana-etc}/etc/protocols")' + --replace-fail 'open("/etc/protocols")' 'open("${final.iana-etc}/etc/protocols")' substituteInPlace src/net/port_unix.go \ - --replace 'open("/etc/services")' 'open("${final.iana-etc}/etc/services")' + --replace-fail 'open("/etc/services")' 'open("${final.iana-etc}/etc/services")' '';
46-81: Document the Rosetta 2 requirement for Apple Silicon as suggested in the previous review.The solc binaries for versions 0.6.8 and 0.8.21 are x86_64-only (Apple Silicon support was added in v0.8.24). On Apple Silicon Macs, these binaries will run under Rosetta 2 translation. As noted in the previous review, this requirement should be documented.
Apply this diff to add clarifying documentation:
# solc-static-versions is broken on Darwin in nixpkgs 25.11 due to legacy SDK usage - # On Darwin: use pre-built macOS binaries. On Linux: use the original nixpkgs derivations. + # On Darwin: use pre-built macOS binaries (x86_64; requires Rosetta 2 on Apple Silicon for v0.6.8 and v0.8.21). + # On Linux: use the original nixpkgs derivations. solc-static-versions =
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
nix/build_overlay.nix(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: gomod2nix
- GitHub Check: build (macos-latest)
- GitHub Check: unittest
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-14)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (evm)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (mint)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (gov)
- GitHub Check: Run golangci-lint
- GitHub Check: semgrep-cloud-platform/scan
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
nix/build_overlay.nix (1)
50-68: Add comment documenting Rosetta 2 requirement for Apple Silicon.The solc-macos binaries for v0.6.8 and v0.8.21 are x86_64-only; official Apple Silicon (arm64) binaries were not released until v0.8.24. On Apple Silicon Macs, these binaries run under Rosetta 2 translation. A brief comment would help future maintainers understand this platform constraint.
# solc-static-versions is broken on Darwin in nixpkgs 25.11 due to legacy SDK usage # On Darwin: use pre-built macOS binaries. On Linux: use the original nixpkgs derivations. + # Note: These macOS binaries are x86_64-only. On Apple Silicon, Rosetta 2 is required. solc-static-versions =
🧹 Nitpick comments (1)
nix/build_overlay.nix (1)
35-40: Consider using--replace-failfor safer substitutions.Using
--replacesilently succeeds even if the pattern isn't found. If Go source files change in future patch releases, the build would succeed but produce incorrect binaries that still reference/etc/protocolsand/etc/services.postPatch = (old.postPatch or "") + '' substituteInPlace src/net/lookup_unix.go \ - --replace 'open("/etc/protocols")' 'open("${final.iana-etc}/etc/protocols")' + --replace-fail 'open("/etc/protocols")' 'open("${final.iana-etc}/etc/protocols")' substituteInPlace src/net/port_unix.go \ - --replace 'open("/etc/services")' 'open("${final.iana-etc}/etc/services")' + --replace-fail 'open("/etc/services")' 'open("${final.iana-etc}/etc/services")' '';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
nix/build_overlay.nix(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: unittest
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-latest)
- GitHub Check: gomod2nix
- GitHub Check: build (macos-14)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (evm)
- GitHub Check: integration_tests (mint)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: Run golangci-lint
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (1)
nix/build_overlay.nix (1)
12-15: LGTM!The backwards compatibility alias for
windows.threads→pthreadsis correctly implemented and well-documented.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
nix/rocksdb.nix (1)
24-24: Consider using the finalAttrs pattern instead of rec.The change from
stdenv.mkDerivation (finalAttrs: { ... })tostdenv.mkDerivation rec { ... }moves away from the modern nixpkgs pattern. ThefinalAttrspattern is preferred because it:
- Provides better error messages for infinite recursion
- Makes the derivation more maintainable
- Follows current nixpkgs conventions
Revert to the finalAttrs pattern:
-stdenv.mkDerivation rec { +stdenv.mkDerivation (finalAttrs: {And update references to use
finalAttrs.versionwhere needed.nix/build_overlay.nix (1)
77-104: Consider extracting a helper function to reduce duplication.The
solc_0_6_8andsolc_0_8_21derivations follow an identical pattern. Consider extracting a helper function to reduce duplication:let mkSolcMacos = version: sha256: final.stdenv.mkDerivation { pname = "solc"; inherit version; src = final.fetchurl { url = "https://github.com/ethereum/solidity/releases/download/v${version}/solc-macos"; inherit sha256; }; dontUnpack = true; installPhase = '' mkdir -p $out/bin cp $src $out/bin/solc-${version} chmod +x $out/bin/solc-${version} ''; }; in { solc_0_6_8 = mkSolcMacos "0.6.8" "sha256-W0qCSwXC9nb1a6pRKPeczT9RZ+1RpVMVCD/vl8xEaEc="; solc_0_8_21 = mkSolcMacos "0.8.21" "sha256-mzXHa30o2kCPW9XhBfr0XsCXS0+7VdPN3bZ4pJG3PU0="; }Note: An existing review comment already covers the Rosetta 2 requirement for these x86_64-only binaries on Apple Silicon.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
nix/build_overlay.nix(1 hunks)nix/rocksdb.nix(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (evm)
- GitHub Check: integration_tests (mint)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: build (macos-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (macos-14)
- GitHub Check: unittest
- GitHub Check: gomod2nix
- GitHub Check: Run golangci-lint
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (11)
nix/rocksdb.nix (7)
8-8: LGTM: pkg-config correctly added for dependency resolution.The addition of
pkg-configto imports andnativeBuildInputsis appropriate for the MinGW CMake configuration added later in the file (lines 93-96).Also applies to: 38-38
19-19: LGTM: enableLite parameter properly implemented.The new
enableLiteparameter with sensible default (false) provides optional ROCKSDB_LITE support without breaking existing behavior.Also applies to: 89-89
59-70: LGTM: Improved compiler-specific flag handling.The restructure properly separates GNU and Clang compiler flags, improving clarity and maintainability for multi-compiler builds.
89-102: LGTM: Comprehensive CMake configuration improvements.The additions handle:
- ROCKSDB_LITE support via
enableLite- Platform-specific warning handling for MinGW
- Proper pkg-config integration for MinGW/LZ4 resolution
The comment clearly explains the pkg-config requirement for MinGW builds.
113-113: LGTM: Dynamic version extraction improves maintainability.Using
lib.versions.major versioninstead of hardcoded version numbers makes the derivation more maintainable for future version updates.Also applies to: 129-129
135-135: LGTM: Consistent version interpolation.The changelog URL now uses
v${version}, consistent with therevfield change and improving maintainability.
30-32: LGTM: Improved maintainability with variable interpolation.Using
pnameforrepoandv${version}forrevmakes future version updates easier. Both variables are correctly defined and the sha256 hash format is valid.nix/build_overlay.nix (4)
22-36: Patch filtering and manual substitutions look correct.The patch filtering properly excludes
iana-etc-1.17.patchfor Go 1.25, and the manualpostPatchsubstitutions correctly rewrite the iana-etc paths. This approach is valid when upstream patches don't apply cleanly to newer versions.Note: An existing review comment already covers updating the Go version to 1.25.5 and using
--replace-fail.
37-51: LGTM: Cross-compilation threading dependencies handled correctly.The
depsTargetTargetlogic safely handles MinGW cross-compilation scenarios wherethreadsmay not be available intargetPackages. The fallback towindows.pthreadsand null-checks prevent evaluation errors.
52-58: LGTM: Windows platform support properly added.Extending
meta.platformswith Windows variants enables cross-compilation support. The use oflib.uniqueensures no duplicate entries.
60-68: LGTM: iana-etc platform support aligned with Go.The
iana-etcoverride is necessary because it's referenced in the Go 1.25 postPatch substitutions (lines 33, 35). Extending its platform support to match Go's Windows support ensures consistency.
songgaoye
force-pushed
the
song/go_1.25.0
branch
from
48de370 to
9a029b6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
nix/build_overlay.nix (3)
17-17: Consider updating to the latest Go 1.25 patch version.Go 1.25.0 was released in August 2025, but more recent patch versions may include important bug fixes and security updates. Verify that 1.25.0 is the intended version or update to the latest 1.25.x patch.
What is the latest Go 1.25 patch version as of December 2025?
31-36: Consider using--replace-failfor safer substitutions.Using
--replace-failinstead of--replaceensures the build fails if the target strings are not found (e.g., if Go's source changes in a future version), rather than silently succeeding with no substitution.postPatch = (old.postPatch or "") + '' substituteInPlace src/net/lookup_unix.go \ - --replace 'open("/etc/protocols")' 'open("${final.iana-etc}/etc/protocols")' + --replace-fail 'open("/etc/protocols")' 'open("${final.iana-etc}/etc/protocols")' substituteInPlace src/net/port_unix.go \ - --replace 'open("/etc/services")' 'open("${final.iana-etc}/etc/services")' + --replace-fail 'open("/etc/services")' 'open("${final.iana-etc}/etc/services")' '';
72-107: Document Rosetta 2 requirement for Apple Silicon Macs.The solc-macos binaries for v0.6.8 and v0.8.21 are x86_64-only; official Apple Silicon (arm64) binaries were not released until v0.8.24. On Apple Silicon Macs, these binaries will run under Rosetta 2 translation. Consider adding a comment to clarify this requirement:
# solc-static-versions is broken on Darwin in nixpkgs 25.11 due to legacy SDK usage # On Darwin: use pre-built macOS binaries. On Linux: use the original nixpkgs derivations. + # Note: solc-macos binaries for v0.6.8 and v0.8.21 are x86_64 only and require Rosetta 2 + # on Apple Silicon Macs (arm64 binaries not available until v0.8.24). solc-static-versions =Additionally, consider adding
metaattributes for consistency:meta = { description = "Solidity compiler"; platforms = [ "x86_64-darwin" "aarch64-darwin" ]; # aarch64 via Rosetta 2 };nix/rocksdb.nix (1)
48-48: lz4 duplication remains unresolved.
lz4appears in bothpropagatedBuildInputs(line 48) andbuildInputs(line 51), which is redundant. This issue was previously flagged in an earlier review. Typically,lz4should only be inpropagatedBuildInputsif downstream packages need to link against it, or only inbuildInputsif it's a private build-time dependency.Based on learnings, this duplication was flagged in a previous review and should be addressed.
Also applies to: 51-51
🧹 Nitpick comments (1)
nix/build_overlay.nix (1)
77-104: Consider extracting a helper function if more versions are needed.The two solc derivations follow an identical pattern. If additional versions are required in the future, consider extracting a helper function:
mkSolcDarwin = { version, sha256 }: final.stdenv.mkDerivation { pname = "solc"; inherit version; src = final.fetchurl { url = "https://github.com/ethereum/solidity/releases/download/v${version}/solc-macos"; inherit sha256; }; dontUnpack = true; installPhase = '' mkdir -p $out/bin cp $src $out/bin/solc-${version} chmod +x $out/bin/solc-${version} ''; };For the current two versions, the existing approach is acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
nix/build_overlay.nix(1 hunks)nix/bundle-win-exe.nix(1 hunks)nix/rocksdb.nix(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- nix/bundle-win-exe.nix
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: integration_tests (gov)
- GitHub Check: unittest
- GitHub Check: Run golangci-lint
- GitHub Check: build (macos-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: gomod2nix
- GitHub Check: build (macos-14)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (mint)
- GitHub Check: integration_tests (evm)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (7)
nix/build_overlay.nix (2)
37-58: Well-structured cross-compilation handling.The
depsTargetTargetlogic provides a robust fallback chain for MinGW cross builds, handling cases where the threads package may not be exposed intargetPackages. The platform extension for Windows variants aligns with the PR's cross-platform support goals.
60-68: LGTM!The
iana-etcplatform extension is consistent with thego_1_25changes and ensures the dependency is available for Windows cross-compilation targets.nix/rocksdb.nix (5)
24-24: Verify the derivation pattern change from finalAttrs to rec.The derivation signature changed from
stdenv.mkDerivation (finalAttrs: { ... })tostdenv.mkDerivation rec { ... }. ThefinalAttrspattern is generally preferred in modern nixpkgs for accessing final attribute values and composability. Confirm that this change to therecpattern is intentional and doesn't impact derivation overrides or attribute finalization.
109-113: Good addition: postPatch fixes memcpy type safety.The explicit
static_castconversions in thememcpycall improve compiler compatibility and address potential type safety warnings. This is a sensible fix for C++ strict aliasing requirements.
60-72: LGTM: Improved compiler flags organization.The restructured
NIX_CFLAGS_COMPILEwith separate conditional blocks for GCC and Clang is clearer and more maintainable than a single list.
182-182: LGTM: Improved version handling.The use of
lib.versions.majorfor extracting the major version number ininstall_name_toolcommands (lines 182, 198) and the consistentv${version}in the changelog URL (line 204) are good improvements for version string handling.Also applies to: 198-198, 204-204
19-19: Verify public API documentation for enableLite parameter.The
enableLiteparameter correctly maps to RocksDB's legitimateROCKSDB_LITEcompile flag used to reduce binary size for lightweight deployments. However, confirm that this parameter is intentionally exposed as public API and includes documentation for derivation consumers on its purpose and when to use it.
| pname = "rocksdb"; | ||
| version = "10.5.1"; | ||
|
|
||
| withLz4 = !stdenv.hostPlatform.isMinGW; |
There was a problem hiding this comment.
Critical logic error: MinGW LZ4 handling code is unreachable.
Line 28 defines withLz4 = !stdenv.hostPlatform.isMinGW, which sets withLz4 to false when building on MinGW. However, the preConfigure block on line 115 requires both stdenv.hostPlatform.isMinGW and withLz4 to be true. This condition can never be satisfied, making all the MinGW LZ4 import library generation code (lines 115-174) unreachable.
This contradicts the comments on lines 95-97 about MinGW LZ4 discovery via pkg-config and the extensive effort to generate proper import libraries.
Apply this diff to fix the logic error:
- withLz4 = !stdenv.hostPlatform.isMinGW;
+ withLz4 = true;Or, if LZ4 should genuinely be disabled on MinGW, remove the entire preConfigure block (lines 115-174) and the MinGW-specific cmake flags (lines 94-103).
Also applies to: 115-174
🤖 Prompt for AI Agents
nix/rocksdb.nix lines 28 and 115-174: the current line 28 sets withLz4 =
!stdenv.hostPlatform.isMinGW which makes the preConfigure branch conditioned on
stdenv.hostPlatform.isMinGW && withLz4 impossible to reach; to fix either (A)
enable LZ4 handling on MinGW by removing the negation so line 28 becomes withLz4
= stdenv.hostPlatform.isMinGW (so the MinGW && withLz4 condition can be true and
the import-library generation runs) or (B) if LZ4 should be disabled on MinGW,
delete the entire preConfigure block (lines 115-174) and remove the
MinGW-specific cmake flags at lines 94-103 to avoid dead code.
👮🏻👮🏻👮🏻 !!!! REFERENCE THE PROBLEM YOUR ARE SOLVING IN THE PR TITLE AND DESCRIBE YOUR SOLUTION HERE !!!! DO NOT FORGET !!!! 👮🏻👮🏻👮🏻
PR Checklist:
make)make test)go fmt)golangci-lint run)go list -json -m all | nancy sleuth)Thank you for your code, it's appreciated! :)
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.