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.

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

Improve the hacking guide #7357

Closed
thufschmitt opened this issue Nov 28, 2022 · 6 comments
Closed

Improve the hacking guide #7357

thufschmitt opened this issue Nov 28, 2022 · 6 comments
Labels
contributor-experience Developer experience for Nix contributors

Comments

@thufschmitt
Copy link
Member

The hacking guide exists and is a great thing to have, but has a number of shortcomings. We should work on it to make the first contributor experience smoother.

@thufschmitt thufschmitt added the contributor-experience Developer experience for Nix contributors label Nov 28, 2022
@thufschmitt
Copy link
Member Author

I tried with @yorickvP to blindly follow the hacking guide as a poor man's user study. Below are some notes about that:

"Building Nix" part

  • The first instruction it suggests is to run nix build (or nix-build), which is mostly useless for the rest and will just cause ppl to wait for nothing;
  • Every command is listed twice (with or without Flakes), which is quite annoying;
  • Some commands mention defaultPackage which doesn't exist any more;
  • The dev shell instructions also mentions clang11StdentPackages, which doesn't exist because it's just clang11Stdev;
  • The name of the devShells aren't great;
  • The autoconf script warns (minor);
  • Inside the shell, nix now points to the locally built one, which
    • Isn't documented
    • Is only the case when using nix develop;
  • The defaults when running make aren't great
    • No parallelism (the hacking guide works around this by specifying -j explicitly)
    • Builds with optimizations on by default
      • It's easy to disable, but not documented
      • Probably shouldn't be the default in the dev environment
      • Currently has to be passed at each make invocation, could be a configure flag instead
  • The end of the instructions mentions again the nix develop vs nix-shell stuff, although all the steps are already duplicated for that;
  • Mentions make installcheck although there's already a section on testing below.

"Testing" part

  • The guide mentions --gtest-filter for the unit tests, but doesn't explain how to pass it
    • It's actually a bit painful and non-intuitive:
      • Find the path of the test executable
        • Not sure how without some wild guessing
      • make it
      • Run it manually with the flag
  • If there's no locally built nix, make installcheck will silently use the system one (since it's the next one in $PATH)
  • The Hydra jobs are part of the test suite, but only run on Linux
    • Not sure whether we can fix that, because I'm not sure that the NixOS test runner works on macOS

Other considerations

  • The README links to the hacking guide, but this is the online version for the stable Nix branch, which might (and was in our case) be outdated;
  • The development environment should (maybe optionally) include a working LSP setup
    • clangd is probably our best bet, and comes (nearly) for free with clang
    • It requires a compile_commands.json database to know what to look for
      • Generating it is a bit painful
      • Best thing for now is bear, but it duplicates all the compiler calls
  • We could maybe improve the compilation speed beyond OPTIMIZE=0
    • include-what-you-use could come in handy here
  • A style guide would be tremendously helpful
    • Related to the issue on having an automatic formatter

@nixos-discourse
Copy link

This issue has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/tweag-nix-dev-update-41/23848/1

@yorickvP
Copy link
Contributor

yorickvP commented Dec 9, 2022

I did some work on this, and added inline checkmarks to the above comment.

I would not like to change make defaults (parallelism), since this would be unexpected for anyone running make.

Changing optimization levels should probably go through ./configure, but -O0 would not be recommended for C++ since that changes some semantics quite drastically (and actually has less debug info). The GCC manual recommends -Og as the optimization level of choice for the standard edit-compile-debug cycle.

I've been working on IWYU, but the resulting changes are quite big and somewhat disputable.
They only improve compilation speed by a few %, but IMO increase code readability and make it easier to refactor code, which could lead to compilation speed improvements in the future.


"Building Nix" part

  • The first instruction it suggests is to run nix build (or nix-build), which is mostly useless for the rest and will just cause ppl to wait for nothing;
  • Every command is listed twice (with or without Flakes), which is quite annoying;
  • Some commands mention defaultPackage which doesn't exist any more;
  • The dev shell instructions also mentions clang11StdentPackages, which doesn't exist because it's just clang11Stdenv;
  • The name of the devShells aren't great;
  • The autoconf script warns (minor);
  • Inside the shell, nix now points to the locally built one, which
    • Isn't documented
    • Is only the case when using nix develop;
  • The defaults when running make aren't great
    • No parallelism (the hacking guide works around this by specifying -j explicitly)
    • Builds with optimizations on by default
      • It's easy to disable, but not documented
      • Probably shouldn't be the default in the dev environment
      • Currently has to be passed at each make invocation, could be a configure flag instead
  • The end of the instructions mentions again the nix develop vs nix-shell stuff, although all the steps are already duplicated for that;
  • Mentions make installcheck although there's already a section on testing below.

"Testing" part

  • The guide mentions --gtest-filter for the unit tests, but doesn't explain how to pass it
    • It's actually a bit painful and non-intuitive:
      • Find the path of the test executable
        • Not sure how without some wild guessing
      • make it
      • Run it manually with the flag
  • If there's no locally built nix, make installcheck will silently use the system one (since it's the next one in $PATH)
  • The Hydra jobs are part of the test suite, but only run on Linux
    • Not sure whether we can fix that, because I'm not sure that the NixOS test runner works on macOS

Other considerations

  • The README links to the hacking guide, but this is the online version for the stable Nix branch, which might (and was in our case) be outdated;
  • The development environment should (maybe optionally) include a working LSP setup
    • clangd is probably our best bet, and comes (nearly) for free with clang
    • It requires a compile_commands.json database to know what to look for
      • Generating it is a bit painful
      • Best thing for now is bear, but it duplicates all the compiler calls
  • We could maybe improve the compilation speed beyond OPTIMIZE=0
    • include-what-you-use could come in handy here
  • A style guide would be tremendously helpful
    • Related to the issue on having an automatic formatter

@bew
Copy link
Contributor

bew commented Dec 10, 2022

Only been made aware of this issue, note that I opened a PR ~1year ago improving the hacking docs:
#5422

If you think it can be improved, I'd be happy to continue it!
Or just take parts of it 🤷‍♂️

@stale stale bot added the stale label Jun 18, 2023
@stale stale bot removed the stale label Jul 18, 2023
@kapuranirudh
Copy link

@yorickvP : Hey can I come in and help you with your hacker guide...!! Before I start I would like to take some inputs and links from u...??

@fricklerhandwerk
Copy link
Contributor

Most of this has been addressed in various PRs, closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contributor-experience Developer experience for Nix contributors
Projects
None yet
Development

No branches or pull requests

7 participants
@yorickvP @fricklerhandwerk @thufschmitt @bew @kapuranirudh @nixos-discourse and others