add an option to disable async build

[pietroppeter]

if there is an error during async build (see #53), having an option todo the standard build might be useful. prompted by https://github.com/SciNim/getting-started/issues/49

[pietroppeter]

note that the error should be a bug (if a build fails the whole thing fails when trying to write the log)

(as an aside we could reimplement parallel build with thready, seems easier to reason about: https://github.com/treeform/thready)

[HugoGranstrom]

I can confirm the suspicion you had here, it's the fact that we are using the relative path that it doesn't work. It tries to open getting-started/basics/data_wrangling.log, but the real location is in the book folder: getting-started/book/basics/data_wrangling.log. So it was never about the file not existing, but the folder not existing. So we have to add srcDir in front of entry.path when we open the file and then it should just work.

[pietroppeter]

Ah good to know so already fixing this would allow us to keep async builds! (an option could be added but not urgent)

[HugoGranstrom]

Yes, the only problem I can think of now is how you can't easily read the errors in the CI as it is saved in a .log file (can you even access CI files?). So we should probably print the actual error message as well?

[pietroppeter]

Ah yes, we should print all error logs (possibly at the end async jobs and with option to turn it off)

[HugoGranstrom]

Yes it can be done after all the async stuff has finished running. It should just be a matter of here, reading the file and printing its content.

I can spin up a PR with this change and possibly also #60 while I'm at it.

[HugoGranstrom]

After a bit of investigation, the easiest way to disable async seems to be to await each build before starting the next one (instead of starting all and then awaiting all at the end). But this still means that we have the .log system even without async. I don't know why we are saving them in files when we could just return an object with the error message (if it has length 0, there was no error). So I might rewrite that, which should give us fewer surfaces for errors.

Ah yes, we should print all error logs (possibly at the end async jobs and with option to turn it off)

I don't know why you wouldn't want to print the errors if you get any, though :thinking:

Also, if we are to parse the input parameters, do you have any preferences for which library to use? parseopt feels a bit clunky tbh for anything past reading the first command. bossy seems like a nice and light abstraction on top of parseopt.

[HugoGranstrom]

@beef331 Any reason in particular why you went with log files for the async errors? Just so I don't rewrite it and realizes that log files are superior to returning a Future[string] :sweat_smile:

[beef331]

With many files I figured having a concise log per file made more sense, but I could be wrong.

[HugoGranstrom]

Ok 👍😄 Working locally I agree with you, especially in cases like if you update the Nim version and half of your pages breaks. Scrolling through a terminal isn't the best experience then. So I'll probably keep the log files around then. Now that the log errors are shown in the terminal as well, it is easy to view the errors in the CI as well.

[HugoGranstrom]

@pietroppeter exactly how did you imagine the passing of the option to not build in parallel? By command line: --parallelBuild:false or as an argument to nimibookCli?

[pietroppeter]

I was thinking about an option by command line

[HugoGranstrom]

Do we pass it when building the CLI (nim c --parallelBuild:false nbook.nim) or do we pass it to the build command (nbook build --parallelBuild:false)? The first can be done using a simple when defined(parallelBuild), the other requires paring the input arguments (and possibly also filtering of them to remove parallelBuild so it isn't passed along).

edit: the first option can be done using a booldefine

[pietroppeter]

I think the correct way would be to have a real argument at runtime, but honestly I think for now it is probably not worth it and maybe we should just go with a compile option.

The way I see it, once I have worked on the nimib SSG, nimibook should be refactored to become a special case, possibly not building anymore the nbook command (the dependency should already give the binary). also given that we now have a fix for errors in parallel builds, the feature to have an option to disable has less priority

[HugoGranstrom]

Ok, I do a compile time thing for now then :+1:

The way I see it, once I have worked on the nimib SSG, nimibook should be refactored to become a special case, possibly not building anymore the nbook command (the dependency should already give the binary).

I agree, there is a lot of similarity between them and it should be possible to generalize it far enough that a nimib build system (eventually) will make it into core nimib/its own library.

[pietroppeter]

closed by #61 you can set -d:nimibMaxProcesses=1