-
-
Notifications
You must be signed in to change notification settings - Fork 14.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
NixOS Manual: Make it easier to debug #26940
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Always good to see improved process, but please fix almost all of the quoting and improve the portability (as indicated in the comment).
@grahamc I think I broke GitHub, because now I cannot see my comments anymore either via the add single line comment feature, which always has worked before or alternatively someone banned me from reviews? Anyway, the comment was about not using |
Fixed the echo -n |
@grahamc You didn't use |
sure, fixed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the quick feedback
Btw. why is it a partability problem? All of these derivations are using bash, no matter on which system. Apart from that, you can't even execute the generic stdenv without bash. |
@aszlig Sure, bash behaves the same, but I don't see portability as black and white, but more as what percentage of code is portable. If the same feature can be implemented in a more portable way without impacting performance too much, then that should be the way it's done. |
If we could grind that particular axe elsewhere, or here after getting this PR merged and focus on the contents of this issue, I'd really love that :) ❤️ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have time to look into the details right now, because I have urgent tasks to attend, but in general I'm certainly not opposed to better error messages.
So from a macro point of view, the only problem that I could see here is that jing increases the amount of build dependencies for the manual. But if it's for better error messages, I think it's a reasonable trade-off.
Great. Thank you, aszlig! I agree, in that case I'll leave it open a bit more to provide a feedback window, then merge. |
Eelco pointed out using jing adds the JDK to the default system closure, so ... that is not good. I added this output instead... I'm not sure. what do y'all think?
|
(merged without the debug notice commit) |
Motivation for this change
When hacking on the manual, if you make an markup error like an invalid tag you get bogus validation results:
except I added that to
installation/installing.xml
!Debugging this further, I found
jing
makes a much better error message. First I perform thexincludes
and produce a single monolithic manual file, then run jing, and get:With
--keep-failed
:Bingo! I added the chapter in the wrong place.
By performing xinclude in one place, this also means the error is only outputted once, and not many times (once per manual type.)
Things done
(nix.useSandbox on NixOS,
or option
build-use-sandbox
innix.conf
on non-NixOS)
nix-shell -p nox --run "nox-review wip"
./result/bin/
)Additionally, I word-diffed the results of this to the current process, and received effectively identical output, for each of the following:
nix-build ./nixos/release.nix -A manual.x86_64-linux -A manualEpub.x86_64-linux -A manpages.x86_64-linux -A options