Skip to content

std/cmdline.commandLineParams NimScript fix#25573

Open
ZoomRmc wants to merge 1 commit intonim-lang:develfrom
ZoomRmc:cmdline
Open

std/cmdline.commandLineParams NimScript fix#25573
ZoomRmc wants to merge 1 commit intonim-lang:develfrom
ZoomRmc:cmdline

Conversation

@ZoomRmc
Copy link
Contributor

@ZoomRmc ZoomRmc commented Mar 3, 2026

cmdline.commandLineParams() now returns only the script arguments, skipping compiler arguments.

Closes #23554

The logic is extracted from parseopt.initOptParser and is now duplicated. Though there it triggers only when there are no arguments provided so double reads are an edge case:

Read commandLineParams() from a NimScript and get none -> pass them explicitly to initOptParser -> Whole argv reread again and stripped.

Should be fixed if #25571 or an alternative is merged.

Now consistently returns only the script arguments,
skipping compiler arguments.
result = @[]
for i in 1..paramCount():
result.add(paramStr(i))
when defined(nimscript):
Copy link
Member

Choose a reason for hiding this comment

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

The compiler as it evaluates paramCount and paramStr at compile-time should do these fixups.

Copy link
Contributor Author

@ZoomRmc ZoomRmc Mar 4, 2026

Choose a reason for hiding this comment

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

Well, maybe it should but it doesn't. These procs are builtins for NimScript, as far as I see. Not sure what you want.

This code is not new, it's just moved copied from parseopt to a more fitting place.

Copy link
Member

Choose a reason for hiding this comment

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

I want you to fix the root cause which is in compiler/scriptconfig.nim, lines:

  cbconf paramStr:
    setResult(a, os.paramStr(int a.getInt 0))
  cbconf paramCount:
    setResult(a, os.paramCount())

Copy link
Contributor Author

@ZoomRmc ZoomRmc Mar 4, 2026

Choose a reason for hiding this comment

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

First of all, I wouldn't know how to, I found the source but I'm not very confident at that level.

Secondly, wouldn't this break scripts inspecting the command line launched intrinsically from config.nims on compilation?

# in config.nims:
for i in 1..paramCount():
  if paramStr(i) == "-foo":
    raise newException(Defect, "Build with 'foo' is not supported")

@ZoomRmc
Copy link
Contributor Author

ZoomRmc commented Mar 6, 2026

So, continuing the discussion above.

As we have an additional case of implicitly run scripts, I think it would be beneficial to leave the paramCount/paramStr "dumb", being able to access the whole command line. These names bring back Pascal memories so feel more lower-level.

The commandLineParams could be considered a convenience wrapper then. However, if the script is launched implicitly, we don't have the script name in the arguments so we strip everything and get an empty seq. Do we want to return all the arguments or none in this case? Probably the first.

@ZoomRmc
Copy link
Contributor Author

ZoomRmc commented Mar 13, 2026

@Araq I'd like to proceed with this, but I think my concerns above are legit.

To repeat myself, two things need clarifications:

  1. Do we really want changing paramStr/paramCount behaviour?
  2. What do we want commandLineParams to return if invoked from implicitly launched scripts (config.nims) where no script filename is present in argv?

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.

In NimScript mode, make commandLineParams() more useful

2 participants