c-blake / cligen

Nim library to infer/generate command-line-interfaces / option / argument parsing; Docs at
https://c-blake.github.io/cligen/
ISC License
507 stars 24 forks source link

Enable most warnings for tests #227

Closed SirNickolas closed 1 year ago

SirNickolas commented 1 year ago

So I did what we discussed in #225. I put GcUnsafe and GlobalVar in nim.cfg since cligen uses global variables; there’s nothing a person who wishes to run tests can do with it. Deprecated remained in GNUMakefile/test.sh so you can easily run without it via w= ./test.sh; and so did ProveField (arguable; might be better to move it to nim.cfg too).

As to a strange cat in the pipeline, I suspect you tried 2>&1 >$o, and it didn’t work because the order of redirections is important. They are processed from left to right: first, stderr is sent to where stdout is currently connected to; then, stdout is redirected to a file. And we want this to happen vice versa. (I ran test.sh without final &, and the tests passed.)

hint[Performance] is indeed issued at a later stage and cannot be scoped, as you told.

c-blake commented 1 year ago

This looks pretty good to me, but I'd like to run it against my set of old Nim's before merging. That may have to wait until tomorrow.

c-blake commented 1 year ago

I suspect that --verbosity:2 (EDIT: or something) causes some trouble with older Nim's (< Nim-1.4) printing out the C compile lines (which induces big diff's). I doubt anyone but me runs the test suite on such old Nims, but I may change that back in a quick follow-up commit...

SirNickolas commented 1 year ago

Strange. --hint[CC]:off --hint[Exec]:off should have disabled that.

Upd.: But it doesn’t, shame on me. I’ll get this to work.

c-blake commented 1 year ago

I was not very clear.. for me it was not printing out of gmake, but it was printing the CC line out of test.sh..

SirNickolas commented 1 year ago

Yeah, just figured that out.

SirNickolas commented 1 year ago

Another mystery: old Nims emit those compilation lines even on gmake — if you tell it to build a specific target (say, gmake J=0 test/AllSeqTypes.out). Seems like Nim did some non-obvious check. No idea what it can be (adding/removing < /dev/null and | cat doesn’t help).

For now, the best I can think of is adding

@if nimHasInvariant: # 1.2+
  verbosity = 2
@end

and removing that switch from GNUmakefile and test.sh.

c-blake commented 1 year ago

Yeah. Or we could have it be verbosity:1 in the test.sh & GNUmakefile and then conditionally bump it up to 2 in the config. I'm fine with either. (I almost never run it changing $v anyway.) I toyed with the idea of just moving only test.sh back to verbosity:1, but probably best to keep it consistent with the GNUmakefile. Happy to merge any reasonable PR along any of these lines.

(BTW, don't worry - I am still investigating your tmplParse2 as to whether the perf really justifies needing to change the API and not just the impl, though I do truly doubt anyone but me uses the API. I only got full electricity restored last night.)

SirNickolas commented 1 year ago

Or we could have it be verbosity:1 in the test.sh & GNUmakefile and then conditionally bump it up to 2 in the config.

Not exactly, --verbosity on the command line overrides one in the config. To conditionally bump it, we have to not pass it explicitly.

(You are doing great job. It’s the fastest support here among all the open-source projects I’ve encountered, and I’m not pressed for time on this issue.)

c-blake commented 1 year ago

Well, for now I just did https://github.com/c-blake/cligen/commit/07b7a8d4fa8a2b8ed569e3d30cac5eb95b3c60cf as a kind of ham-fisted override to go back to --verbosity:1 for those older Nim's. Definitely let me know if you ever figure this out.