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

Fix cli invocation from nimscript #109

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

etan-status
Copy link
Contributor

When calling cli macro from nimscript, there are compilation issues:

  • nim-faststreams is not available, therefore, nim-serialization does not work, due to equalMem being gated behind notJSnotNims. Dropping support for config files in nimscript contexts fixes that.

  • std/strformat raises ValueError for invalid format strings, but does so at runtime rather than checking types at compiletime. As it is only used for simple string concatenation in error cases, changing to simple concatenation avoids verbose error handling.

  • getAppFilename() is unavailable in nimscript. This was already fixed by replacing it with appInvocation() but two instances of direct getAppFilename() calls remained in default arguments. This is fixed by changing those default arguments as well.

  • The != nil check on the proc in loadImpl does not work when called from nimscript. This is fixed by changing to isNil.

  • Passing addr result around to internal templates correctly creates the config, but the object ultimately being returned is not the same. Passing var result directly through the templates ensures that the correct result gets modified and is clearer than implicit capture.

Applying these fixes fixes running .nims files with cli macro.

When calling `cli` macro from nimscript, there are compilation issues:

- `nim-faststreams` is not available, therefore, `nim-serialization`
  does not work, due to `equalMem` being gated behind `notJSnotNims`.
  Dropping support for config files in nimscript contexts fixes that.

- `std/strformat` raises `ValueError` for invalid format strings, but
  does so at runtime rather than checking types at compiletime. As it
  is only used for simple string concatenation in error cases, changing
  to simple concatenation avoids verbose error handling.

- `getAppFilename()` is unavailable in `nimscript`. This was already
  fixed by replacing it with `appInvocation()` but two instances of
  direct `getAppFilename()` calls remained in default arguments.
  This is fixed by changing those default arguments as well.

- The `!= nil` check on the `proc` in `loadImpl` does not work when
  called from nimscript. This is fixed by changing to `isNil`.

- Passing `addr result` around to internal templates correctly creates
  the config, but the object ultimately being returned is not the same.
  Passing `var result` directly through the templates ensures that the
  correct `result` gets modified and is clearer than implicit capture.

Applying these fixes fixes running `.nims` files with `cli` macro.
@etan-status
Copy link
Contributor Author

Related: status-im/nimbus-eth2#6681 (comment)

There, the import is incorrect (std/confutils instead of confutils), but after fixing that the issues fixed by this PR here are revealed. With the fixes, the fuzzer tool works once more (pending retarget to proper test vectors and fuzzing tools).

confutils.nimble Outdated Show resolved Hide resolved

if secondarySources != nil:
if not isNil(secondarySources): # Nim v2.0.10: `!= nil` broken in nimscript
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type SecondarySources = object

proc loadImpl[C, SecondarySources](
    Configuration: typedesc[C],
    secondarySourcesRef: ref SecondarySources,
    secondarySources: proc (
        config: Configuration, sources: ref SecondarySources
    ) {.gcsafe.} = nil): Configuration =
  if secondarySources != nil:
    secondarySources(result, secondarySourcesRef)

template load(
    Configuration: type,
    secondarySources: untyped = nil): untyped =
  block:
    let secondarySourcesRef = new SecondarySources
    loadImpl(Configuration, secondarySourcesRef, secondarySources)

type
  CliConfig_33558379 = object
let config_33558380 = load(CliConfig_33558379)

is repro, and it's an issue in v2.0.10 and version-2-0 but not version-2-2 or devel. Using isNil seems fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it worked for me in nim c mode and only bugs out in nim e nimscript mode.
do you have a link that I should refer to in the comment here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't, because it seems to already be fixed in Nim upstream, and it's not really worth a backport if it's just allowing != nil instead of not isNil.

Can link to this comment in GitHub perhaps.

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.

2 participants