Skip to content
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.

Sign up for GitHub

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

Jump to bottom

NixOS Manual: Make it easier to debug #26940

Merged
merged 2 commits into from Jun 29, 2017
Merged

NixOS Manual: Make it easier to debug #26940

merged 2 commits into from Jun 29, 2017

Conversation

grahamc
Copy link
Member

@grahamc grahamc commented Jun 28, 2017 •
edited

Motivation for this change

When hacking on the manual, if you make an markup error like an invalid tag you get bogus validation results:

manual.xml:5: element chapter: Relax-NG validity error : Element part has extra content: chapter

except I added that to installation/installing.xml!

Debugging this further, I found jing makes a much better error message. First I perform the xincludes and produce a single monolithic manual file, then run jing, and get:

/tmp/nix-build-nixos-manual-combined.drv-0/./manual-combined.xml:313:10: error: element "chapter" not allowed here; expected element "address", "anchor", ...

With --keep-failed:

$ cat /tmp/nix-build-nixos-manual-combined.drv-0/./manual-combined.xml | head -n 318 | tail -n10
typical sequence of commands for installing NixOS on an empty hard
drive (here <filename>/dev/sda</filename>).  <xref linkend="ex-config"/> shows a corresponding configuration Nix expression.</para>

<example xml:id="ex-install-sequence"><title>Commands for Installing NixOS on <filename>/dev/sda</filename></title>
<chapter><screen>
# fdisk /dev/sda # <lineannotation>(or whatever device you want to install on)</lineannotation>
# mkfs.ext4 -L nixos /dev/sda1
# mkswap -L swap /dev/sda2
# swapon /dev/sda2
# mount /dev/disk/by-label/nixos /mnt

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
  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

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

@grahamc grahamc requested review from aszlig and edolstra June 28, 2017 21:12
0xABAB
0xABAB suggested changes Jun 28, 2017
View reviewed changes
Copy link
Contributor

@0xABAB 0xABAB left a 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
Copy link
Member Author

grahamc commented Jun 28, 2017

For the second time @0xABAB I'm unable to see your comments (same as #26917.) If you can see them somehow, I'm not sure how, but consider viewing the PR in a private browsing window to see what I'm seeing.

@0xABAB
Copy link
Contributor

0xABAB commented Jun 28, 2017

@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 echo -n and instead using printf %s, because of non-portability of echo -n.

@grahamc
Copy link
Member Author

grahamc commented Jun 28, 2017

Fixed the echo -n

@0xABAB
Copy link
Contributor

0xABAB commented Jun 28, 2017

@grahamc You didn't use printf %s "${version}. If version contains e.g. %s%d, undefined behavior will happen.

@grahamc
Copy link
Member Author

grahamc commented Jun 28, 2017

sure, fixed

0xABAB
0xABAB approved these changes Jun 29, 2017
View reviewed changes
Copy link
Contributor

@0xABAB 0xABAB left a 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

@aszlig
Copy link
Member

aszlig commented Jun 29, 2017

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.

@0xABAB
Copy link
Contributor

0xABAB commented Jun 29, 2017

@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.

@grahamc
Copy link
Member Author

grahamc commented Jun 29, 2017

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 :) ❤️

aszlig
aszlig reviewed Jun 29, 2017
View reviewed changes
Copy link
Member

@aszlig aszlig left a 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.

@grahamc
Copy link
Member Author

grahamc commented Jun 29, 2017

Great. Thank you, aszlig! I agree, in that case I'll leave it open a bit more to provide a feedback window, then merge.

@grahamc
Copy link
Member Author

grahamc commented Jun 29, 2017 •
edited

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?

                       DEBUG NOTICE

  Validating the XML, but xmllint's error messages can be a bit
  meaningless. If it fails, you have two options:

   * Jing in a nix shell

     Try building again with --keep-failed and run the file
     through jing like so:

       nix-shell -p jing --run "jing /nix/store/snhbaafcmqpr19pn039rgivamdw29lyr-docbook5-5.0/xml/rng/docbook/docbook.rng /path/to/failed-build-results/manual-combined.xml"


   * Adding Jing to the manual manual build phase

     In nixos/doc/manual/default.nix find the comment:

         # for debug help, add jing here

     Then, add the following code:

         ${pkgs.jing}/bin/jing ${pkgs.docbook5}/xml/rng/docbook/docbook.rng ./manual-combined.xml
         ${pkgs.jing}/bin/jing ${pkgs.docbook5}/xml/rng/docbook/docbook.rng ./man-pages-combined.xml


      Note we can't do this by default, because we can't add
      Jing and the JDK to the default system closure. Do not
      commit this change.

  This will produce much better error messages.

@grahamc grahamc merged commit 8ca805d into NixOS:master Jun 29, 2017
@grahamc grahamc deleted the nixos-manual branch June 29, 2017 23:47
@grahamc
Copy link
Member Author

grahamc commented Jun 29, 2017

(merged without the debug notice commit)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aszlig aszlig aszlig left review comments

@0xABAB 0xABAB 0xABAB approved these changes

@edolstra edolstra Awaiting requested review from edolstra

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@grahamc @0xABAB @aszlig