Compiler options for nimble run are not passed (again)

[shunf4]

Reproduction:
nimble-run-repro.zip

# assert_fail.nim
assert false

# nimble_run_repro.nimble
# Package

version       = "0.1.0"
author        = "shunf4"
description   = "Nimble run with compilation arguments reproduction"
license       = "MIT"
srcDir        = "src"
bin           = @["assert_fail"]

nimble --assertions:off run assert_fail succeeds (which is expected), while
nimble run --assertions:off assert_fail throws (which is not expected),

i.e. #889 appears again.

Tested with nimble built with the latest commit (752f88e).

[shunf4]

The patch for 752f88e solves the issue:

diff --git a/src/nimblepkg/options.nim b/src/nimblepkg/options.nim
index ac37bbc..eca0ede 100644
--- a/src/nimblepkg/options.nim
+++ b/src/nimblepkg/options.nim
@@ -518,14 +518,16 @@ proc parseMisc(options: var Options) =
 proc handleUnknownFlags(options: var Options) =
   if options.action.typ == actionRun:
     # actionRun uses flags that come before the command as compilation flags.
-    options.action.compileFlags =
+    options.action.compileFlags.add(
       map(options.unknownFlags, x => getFlagString(x[0], x[1], x[2]))
+    )
     options.unknownFlags = @[]
   elif options.action.typ == actionCustom:
     # actionCustom uses flags that come before the command as compilation flags
     # and flags that come after as run flags.
-    options.action.custCompileFlags =
+    options.action.custCompileFlags.add(
       map(options.unknownFlags, x => getFlagString(x[0], x[1], x[2]))
+    )
     options.unknownFlags = @[]
   else:
     # For everything else, handle the flags that came before the command

But I am not sure if it is good enough.

[dom96]

We've got many tests for this, I guess this is argument ordering that we didn't consider (or maybe there is a reason it isn't allowed, judging by the comment in the code it seems likely, but maybe we should still allow it). Please create a PR and tests :)

[shunf4]

Yeah from comments in code it seems this is by intention (# actionRun uses flags that come before the command as compilation flags.), but it contradicts with what is said in readme (it 's only required that compilation flags come before the binary).

In thread #889 it seems like you and konsumlamm reached a consensus and agreed that this should change, but konsumlamm did not change this in #896 (only the delimiter -- was introduced).

I will look at the tests later.

[shunf4]

nimble/tests/tester.nim

Lines 780 to 807 in 752f88e

	test "Compile flags before executable name":
	cd "run":
	let (output, exitCode) = execNimble(
	"run", # Run command invokation
	"--debug", # Flag to enable debug verbosity in Nimble
	"run", # The executable to run
	"--test" # First argument passed to the executed command
	)
	check exitCode == QuitSuccess
	check output.contains("tests$1run$1$2 --test" %
	[$DirSep, "run".changeFileExt(ExeExt)])
	echo output
	check output.contains("""Testing `nimble run`: @["--test"]""")
	test "Compile flags before --":
	cd "run":
	let (output, exitCode) = execNimble(
	"run", # Run command invokation
	"--debug", # Flag to enable debug verbosity in Nimble
	"--", # Separator for arguments
	"--test" # First argument passed to the executed command
	)
	check exitCode == QuitSuccess
	check output.contains("tests$1run$1$2 --test" %
	[$DirSep, "run".changeFileExt(ExeExt)])
	echo output
	check output.contains("""Testing `nimble run`: @["--test"]""")

I think the two tests are not good enough? --debug is not a compilation flag. It's for nimble.

[dom96]

I think the two tests are not good enough?

Feel free to add more tests.

[konsumlamm]

The patch for 752f88e solves the issue:

diff --git a/src/nimblepkg/options.nim b/src/nimblepkg/options.nim
index ac37bbc..eca0ede 100644
--- a/src/nimblepkg/options.nim
+++ b/src/nimblepkg/options.nim
@@ -518,14 +518,16 @@ proc parseMisc(options: var Options) =
 proc handleUnknownFlags(options: var Options) =
   if options.action.typ == actionRun:
     # actionRun uses flags that come before the command as compilation flags.
-    options.action.compileFlags =
+    options.action.compileFlags.add(
       map(options.unknownFlags, x => getFlagString(x[0], x[1], x[2]))
+    )
     options.unknownFlags = @[]
   elif options.action.typ == actionCustom:
     # actionCustom uses flags that come before the command as compilation flags
     # and flags that come after as run flags.
-    options.action.custCompileFlags =
+    options.action.custCompileFlags.add(
       map(options.unknownFlags, x => getFlagString(x[0], x[1], x[2]))
+    )
     options.unknownFlags = @[]
   else:
     # For everything else, handle the flags that came before the command

But I am not sure if it is good enough.

That looks good to me, it should be enough to fix the issue.

In thread #889 it seems like you and konsumlamm reached a consensus and agreed that this should change, but konsumlamm did not change this in #896 (only the delimiter -- was introduced).

It was supposed to change, that was the whole point of my PR. -- already existed before, I just changed how it works.

nimble/tests/tester.nim

Lines 780 to 807 in 752f88e

	test "Compile flags before executable name":
	cd "run":
	let (output, exitCode) = execNimble(
	"run", # Run command invokation
	"--debug", # Flag to enable debug verbosity in Nimble
	"run", # The executable to run
	"--test" # First argument passed to the executed command
	)
	check exitCode == QuitSuccess
	check output.contains("tests$1run$1$2 --test" %
	[$DirSep, "run".changeFileExt(ExeExt)])
	echo output
	check output.contains("""Testing `nimble run`: @["--test"]""")
	test "Compile flags before --":
	cd "run":
	let (output, exitCode) = execNimble(
	"run", # Run command invokation
	"--debug", # Flag to enable debug verbosity in Nimble
	"--", # Separator for arguments
	"--test" # First argument passed to the executed command
	)
	check exitCode == QuitSuccess
	check output.contains("tests$1run$1$2 --test" %
	[$DirSep, "run".changeFileExt(ExeExt)])
	echo output
	check output.contains("""Testing `nimble run`: @["--test"]""")

I think the two tests are not good enough? --debug is not a compilation flag. It's for nimble.

Ye, I didn't realize that --debug is special cased when writing the tests. It should be replaced by some other flag.