Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bump nimbus-build-system to use Nim v2.0.10 #2684

Merged
merged 7 commits into from
Oct 6, 2024
Merged

bump nimbus-build-system to use Nim v2.0.10 #2684

merged 7 commits into from
Oct 6, 2024

Conversation

tersec
Copy link
Contributor

@tersec tersec commented Oct 3, 2024

No description provided.

@arnetheduck
Copy link
Member

newPayload issue is from #2685

@tersec
Copy link
Contributor Author

tersec commented Oct 3, 2024

newPayload issue is from #2685

Yeah, it's a type alias confusion:

type
  FixedBytes[N: static int] = distinct array[N, byte]

template makeFixedBytesN(N: static int) =
  # Create specific numbered instantiations along with helpers
  type `Bytes N` = FixedBytes[N]

  const `zeroBytes N` = system.default(`Bytes N`)
  template default(T: type `Bytes N`): `Bytes N` =
    # reuse single constant for precomputed N
    `zeroBytes N`

  template `bytes N`(s: static string): `Bytes N` =
    `Bytes N`.fromHex(s)

makeFixedBytesN(32)

type Hash32 = distinct Bytes32

const zeroHash32 = system.default(Hash32) ## Hash32 value consisting of all zeroes

template hash32(s: static string): Hash32 =
  s.to(Hash32)

proc engine_newPayloadV3(parentBeaconBlockRoot: Hash32): int = 1

template newPayload(
    parentBeaconBlockRoot: Bytes32): int =
  engine_newPayloadV3(
    parentBeaconBlockRoot)
  0

#discard newPayload(default(Hash32))
discard newPayload(default(Bytes32))

@tersec
Copy link
Contributor Author

tersec commented Oct 3, 2024

If one tries to compile fluffy/tests/rpc_tests/test_portal_rpc_client.nim, one gets:

nimbus-eth1/fluffy/tests/rpc_tests/test_portal_rpc_client.nim(119, 1) template/generic instantiation of `procSuite` from here
nimbus-eth1/vendor/nim-testutils/testutils/unittests.nim(9, 5) template/generic instantiation of `suite` from here
nimbus-eth1/fluffy/tests/rpc_tests/test_portal_rpc_client.nim(268, 3) template/generic instantiation of `asyncTest` from here
nimbus-eth1/vendor/nim-unittest2/unittest2.nim(1128, 24) template/generic instantiation of `failingOnExceptions` from here
nimbus-eth1/vendor/nim-unittest2/unittest2.nim(1132, 26) template/generic instantiation of `failingOnExceptions` from here
nimbus-eth1/vendor/nim-testutils/testutils/unittests.nim(15, 21) template/generic instantiation of `async` from here
nimbus-eth1/fluffy/tests/rpc_tests/test_portal_rpc_client.nim(290, 7) template/generic instantiation of `check` from here
nimbus-eth1/fluffy/tests/rpc_tests/test_portal_rpc_client.nim(292, 29) template/generic instantiation of `check` from here
nimbus-eth1/vendor/nimbus-build-system/vendor/Nim/lib/system/comparisons.nim(334, 13) template/generic instantiation of `==` from here
nimbus-eth1/vendor/nimbus-build-system/vendor/Nim/lib/system.nim(1719, 13) Warning: toMDigest is deprecated [Deprecated]
nimbus-eth1/vendor/nim-json-rpc/json_rpc/private/server_handler_wrapper.nim(93, 30) Error: request to generate code for .compileTime proc: $

nim-lang/Nim#24228

@tersec
Copy link
Contributor Author

tersec commented Oct 4, 2024

If one tries to compile fluffy/tests/rpc_tests/test_portal_rpc_client.nim, one gets:

nimbus-eth1/fluffy/tests/rpc_tests/test_portal_rpc_client.nim(119, 1) template/generic instantiation of `procSuite` from here
nimbus-eth1/vendor/nim-testutils/testutils/unittests.nim(9, 5) template/generic instantiation of `suite` from here
nimbus-eth1/fluffy/tests/rpc_tests/test_portal_rpc_client.nim(268, 3) template/generic instantiation of `asyncTest` from here
nimbus-eth1/vendor/nim-unittest2/unittest2.nim(1128, 24) template/generic instantiation of `failingOnExceptions` from here
nimbus-eth1/vendor/nim-unittest2/unittest2.nim(1132, 26) template/generic instantiation of `failingOnExceptions` from here
nimbus-eth1/vendor/nim-testutils/testutils/unittests.nim(15, 21) template/generic instantiation of `async` from here
nimbus-eth1/fluffy/tests/rpc_tests/test_portal_rpc_client.nim(290, 7) template/generic instantiation of `check` from here
nimbus-eth1/fluffy/tests/rpc_tests/test_portal_rpc_client.nim(292, 29) template/generic instantiation of `check` from here
nimbus-eth1/vendor/nimbus-build-system/vendor/Nim/lib/system/comparisons.nim(334, 13) template/generic instantiation of `==` from here
nimbus-eth1/vendor/nimbus-build-system/vendor/Nim/lib/system.nim(1719, 13) Warning: toMDigest is deprecated [Deprecated]
nimbus-eth1/vendor/nim-json-rpc/json_rpc/private/server_handler_wrapper.nim(93, 30) Error: request to generate code for .compileTime proc: $

nim-lang/Nim#24228

Addressed by

@tersec
Copy link
Contributor Author

tersec commented Oct 4, 2024

2024-10-04T05:46:32.3318985Z /home/runner/work/nimbus-eth1/nimbus-eth1/nimbus/db/ledger/backend/accounts_ledger.nim(458, 44) template/generic instantiation of `valueOr` from here
2024-10-04T05:46:32.3341242Z /home/runner/work/nimbus-eth1/nimbus-eth1/nimbus/db/ledger/backend/accounts_ledger.nim(465, 20) template/generic instantiation of `put` from here
2024-10-04T05:46:34.3562192Z /home/runner/work/nimbus-eth1/nimbus-eth1/fluffy/tools/eth_data_exporter/cl_data_exporter.nim(68, 5) template/generic instantiation of `async` from here
2024-10-04T05:46:34.3569794Z /home/runner/work/nimbus-eth1/nimbus-eth1/fluffy/tools/eth_data_exporter/cl_data_exporter.nim(79, 23) template/generic instantiation of `awaitWithTimeout` from here
2024-10-04T05:46:34.3580645Z /home/runner/work/nimbus-eth1/nimbus-eth1/fluffy/tools/eth_data_exporter/cl_data_exporter.nim(83, 9) Error: ambiguous call; both chronicles.error(eventName: static[string], props: varargs[untyped]) [template declared in /home/runner/work/nimbus-eth1/nimbus-eth1/vendor/nim-chronicles/chronicles.nim(378, 12)] and macros.error(msg: string, n: NimNode) [proc declared in /home/runner/work/nimbus-eth1/nimbus-eth1/vendor/nimbus-build-system/vendor/Nim/lib/core/macros.nim(436, 6)] match for: (string)
2024-10-04T05:46:34.4725122Z make: *** [Makefile:301: eth_data_exporter] Error 1
2024-10-04T05:46:34.4729188Z make: *** Waiting for unfinished jobs....
2024-10-04T05:46:40.4031761Z /home/runner/work/nimbus-eth1/nimbus-eth1/fluffy/tools/eth_data_exporter/cl_data_exporter.nim(68, 5) template/generic instantiation of `async` from here
2024-10-04T05:46:40.4038231Z /home/runner/work/nimbus-eth1/nimbus-eth1/fluffy/tools/eth_data_exporter/cl_data_exporter.nim(79, 23) template/generic instantiation of `awaitWithTimeout` from here
2024-10-04T05:46:40.4047242Z /home/runner/work/nimbus-eth1/nimbus-eth1/fluffy/tools/eth_data_exporter/cl_data_exporter.nim(83, 9) Error: ambiguous call; both chronicles.error(eventName: static[string], props: varargs[untyped]) [template declared in /home/runner/work/nimbus-eth1/nimbus-eth1/vendor/nim-chronicles/chronicles.nim(378, 12)] and macros.error(msg: string, n: NimNode) [proc declared in /home/runner/work/nimbus-eth1/nimbus-eth1/vendor/nimbus-build-system/vendor/Nim/lib/core/macros.nim(436, 6)] match for: (string)
2024-10-04T05:46:40.5138720Z make: *** [Makefile:301: portal_bridge] Error 1

Related to nim-lang/Nim#24231 but distinct: it persists in the version-2-2/devel branches.

There's an obvious workaround of using chronicles.error ... in relevant places but would be nice to be able to avoid that.

@metagn
Copy link

metagn commented Oct 4, 2024

Error: ambiguous call; both chronicles.error(eventName: static[string], props: varargs[untyped]) and macros.error(msg: string, n: NimNode) match for: (string)

I think the issue here is that not-provided arguments count for the disambiguation (in sumGeneric: NimNode = ref object = ref + object = 2, varargs[untyped] = varargs + untyped = 1 + 0 but static[string] = static + string = 2). There would still be a problem if chronicles.error used string instead of static[string] but this is expected. Edit: Opened nim-lang/Nim#24232 for this

I think macros.error is too broadly named, import macros except error then qualifying macros.error if necessary would probably be easier. Renaming it in Nim would be nice too.

@tersec
Copy link
Contributor Author

tersec commented Oct 4, 2024

I think the issue here is that not-provided arguments count for the disambiguation (in sumGeneric: NimNode = ref object = ref + object = 2, varargs[untyped] = varargs + untyped = 1 + 0 but static[string] = static + string = 2). There would still be a problem if chronicles.error used string instead of static[string] but this is expected.

I think macros.error is too broadly named, import macros except error then qualifying macros.error if necessary would probably be easier. Renaming it in Nim would be nice too.

The except approach seems reasonable, and can be done where it's imported rather than throughout the codebase. One challenge with these quite general symbols is that they end up visible throughout both nimbus-eth1 and nimbus-eth2 repos' code because both use import/export networks. e.g., to avoid generics sandwich issues. So one ends up with some code which doesn't directly know or care about macros accidentally having macros symbols from 3 import layers down bubble up. A solution (such as chronicles.foo) isn't just ugly, it scales badly too. By contrast, the except approach seems reasonable.

But, yeah, it would be nicer not to require. error is some prime symbol real estate macros is using there.

@tersec tersec merged commit 845f327 into master Oct 6, 2024
15 checks passed
@tersec tersec deleted the SnJ branch October 6, 2024 12:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants