search

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

Respect `-j` in the Makefile #225

Closed SirNickolas closed 1 year ago

SirNickolas commented 1 year ago

--warning[all]:off has been added in one of later versions; should use --warnings:off instead for compatibility (and it’s shorter, by the way).

By default, Nim compiles generated C code in multiple processes. Since we’d like to give the user invoking our Makefile control over how much concurrency they want (-j), we pass --parallelBuild:1 (i.e., one-threaded) to Nim.

SirNickolas commented 1 year ago

Sorry, turned out it was impossible to enable a specific warning after --warnings:off. So only the second part of this PR stays.

By the way, I wonder whether it was a good idea to choose a lowercase a for extra arguments to the Nim compiler. Idiomatic makefiles use uppercase variables.

c-blake commented 1 year ago

First - you are right that we should not double-up on parallelism. It should be either from make or nim. I am fine with either make -j1 with the nim doing its parallel default or with your parallelBuild:1 idea.

I am fine changing a to A. (X for eXtra might also work, but lower & upper X are more visually similar.) I think that came from your initial version.

I don't personally use the make/gmake thing you did, but if we change a->A we should add a line in RELEASE_NOTES.md for anyone who does.

It's fine to do that as part of this PR, if you like, or another one. Let me know.

SirNickolas commented 1 year ago

I discovered this when tried to lower the number of jobs below my nproc — and realized I couldn’t.

Indeed, there are two ways to achieve that, and each has downsides. If Make is responsible for parallelism, then the last compiler invocations may run on fewer cores than they theoretically could. If Nim is, then all linking tasks (also quite heavy) will run serially. The former has smaller impact so I chose it.

…And after some thinking it becomes clear we don’t have to choose. We can add another configuration variable, say J, pass it to --parallelBuild:$J, and invoke with either make J="$(nproc)" or make -j"$(nproc)".

I think that came from your initial version.

It did. I think it stood for additional arguments. Perhaps, I’ve seen an option named this way somewhere but not sure.

If we are talking about the makefile, I was surprised it silences all warnings and hints. I suppose a better way would be to add test/config.nims that tests Nim version and disables only unwanted compiler messages. That will affect test.sh too, though.

c-blake commented 1 year ago

You are right that we do not have to choose. I'm really fine with the whole variety of choices here. I only test on systems that can just run test.sh with its high memory requirements. :-)

Yeah. test.sh defaults (with shell overridable w=ActualW ./test.sh) to just silencing a few warnings, and your initial GNUmakefile also did that. https://github.com/c-blake/cligen/pull/222 changed it, and I'm not entirely sure why. I could guess that some specific version of Nim with some specific backend being tested emitted many annoying warnings. We should probably ask @CyberTailor before changing that back.

CyberTailor commented 1 year ago

should use --warnings:off instead for compatibility (and it’s shorter, by the way).

No, we need to whitelist UserWarning for tests. It is not possible with --warnings:off

c-blake commented 1 year ago

@CyberTailor - Ok. It's fine to stay as you had it. Variability of nim warnings over many versions seems like a good enough reason (EDIT: in an automated, nim-version-varying environment).

Do you use gmake a=ANYTHING or otherwise care if that lowercase a is renamed to capital A?

CyberTailor commented 1 year ago

whether it was a good idea to choose a lowercase a for extra arguments to the Nim compiler

Meaningful variable would be better

CyberTailor commented 1 year ago

I could guess that some specific version of Nim with some specific backend being tested emitted many annoying warnings

Yes

CyberTailor commented 1 year ago

Wrt parallelism, you can use -l (--load-average) option of Make

SirNickolas commented 1 year ago

It is not possible with --warnings:off

@CyberTailor, you are right; that change was a mistake. I reverted it.

However, I don’t think we are forced to whitelist warnings. We can instead blacklist unneeded ones. Having all but a few diagnostics enabled has the benefit that you’ll immediately know when something suspicious occurs.

I tried several compiler versions (every even release from 0.20 to devel) and came up with these rules:

What have I missed?

Regarding source-specific switches, there are two ways to specify them: directly in the source via {.push.}/when/{.warning.}/{.pop.} or in the accompanied .nims. What would be better?

It might be worth adding --skipUserCfg too. (I nearly missed the Conf hint that was disabled by my ~/.config/nim/config.nims.)

SirNickolas commented 1 year ago

test.sh adds --warning[ObservableStores]:off --warning[Deprecated]:off as well, but they seem unnecessary.

c-blake commented 1 year ago

Re: a -> NIM_EXTRA - done (but for RELEASE_NOTES.md).

gmake parallelism seems better than nim --parallelBuild in several ways. One is just saturating work better as these test/ programs do not import all that much. Another is indeed being able to target a load average (not that 1minute LA is great in these days where computers are so many 1000s of times faster than 1980). But I don't think I need to warn people way from $J in that GNUmakefile comment. :-) It's just extra magic for power users who maybe have 128 cores or something.

As to old warning toggles - they were necessary at some point and probably still are on some old versions of Nim I run the test suite against. So, it would be more convenient for me to not remove them. I do generally try to keep the code warning free, but I have not always succeeded. Araq sometimes regrets whole families of warnings. :-) That UserCfg bit is definitely a lurking landmine, though.

c-blake commented 1 year ago

Oh, but trying to add --skipUserCfg:off just now reminded me why I didn't do that before - my personal UserCfg lets me run the test suite with tcc/TinyCC about 4.5x faster than with gcc. But, nim core has been less supportive of tcc in recent years. So, I need to set certain things in my ~/.config/nim/nim.cfg to keep it working. So, if there is popular demand for having this in NIM_FLAGS by default I can do it and then just use gmake -j4 NIM_EXTRA=---skipUserCfg:off instead. But really I run test.sh. But I could also see an argument for consistency between the two. But also an argument for the primary author testing 4.5x faster by default. But also an argument for testing against primary deployment of gcc. Anyway, various competing concerns to consider.

c-blake commented 1 year ago

or in the accompanied .nims. What would be better?

Sorry I did not answer this. config.nims is actually quite a bit slower than nim.cfg. But the source code way is fastest of all. I do also usually try to disable warnings in the source code in various places I consider them "false positives" or "unimportant/more distracting than they're worth".

Sometimes the warning seems to come out of some more global/late stage analysis that is not well "lexically scoped" by the push & pop. So, you need to never pop (or else the warning shows up).

EDIT: Anyway, so I am ok with some more warn-y change as long as @CyberTailor is, but it should probably be a new PR now.

c-blake commented 1 year ago

And besides a new PR, you may want to also check the nim cpp backend as I've seen some variation on this kind of thing between c/c++ backends.

SirNickolas commented 1 year ago

But I could also see an argument for consistency between the two.

I believe maintaining consistency between them is important. So we can leave user-config processing as it is, and any bugs, should they appear at some point, will be caught by GitHub CI.

config.nims is actually quite a bit slower than nim.cfg.

Unfortunately, nim.cfg is unable to test Nim version. So, while we can avoid NimScript for file-specific compilation switches, we have to stick to it for config.nims.

Sometimes the warning seems to come out of some more global/late stage analysis that is not well "lexically scoped" by the push & pop. So, you need to never pop (or else the warning shows up).

Those diagnostics in FancyRepeats, FancyRepeats2, and ListDecl are caused by the tests themselves, not cligen’s library code. Upd.: But that’s an interesting observation, thanks.

c-blake commented 1 year ago

Unfortunately, nim.cfg is unable to test Nim version. So, while we can avoid NimScript for file-specific compilation switches, we have to stick to it for config.nims.

This may not be authoritative - it's really just a copy-paste of the top of my nim.cfg file, but it might be helpful.

Ideally, each Nim compiler flag would have an associated defineSymbol or there would be at least a defineSymbol for versions, BUT we can: git log -p compiler/condsyms.nim lib/system.nim & find NimMinor edits and then search forward/backward for defineSymbol instead to get:

E.g., you can say @if nimHasInvariant: instead of (NimMajor,NimMinor,NimPatch)>(1,2,0):.

It's not perfectly precise in terms of date precision, but to fail you need to be using some version of Nim in a small time window which is pretty unlikely in practice. It hasn't yet failed for me, but then I also control the versions of Nim I use. ;-) Seemed worth sharing anyway.

SirNickolas commented 1 year ago

Thank you! Armed with this list, I converted my user config to .cfg.