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

feat(packages): Add debug output to nix builds #356

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

feat(packages): Add debug output to nix builds #356

wants to merge 2 commits into from

Conversation

kobyhallx
Copy link
Contributor

Description

Adding new package output for nix build command to allow for easy wasm target with debug symbols. Among others this allow to debug cpp code in browser.

Use with nix build git+https://github.com/AztecProtocol/barretenberg#wasm32

Checklist:

  • I have reviewed my diff in github, line by line.
  • Every change is related to the PR description.
  • I have linked this pull request to the issue(s) that it resolves.
  • There are no unexpected formatting changes, superfluous debug logs, or commented-out code.
  • There are no circuit changes, OR specifications in /markdown/specs have been updated.
  • There are no circuit changes, OR a cryptographer has been assigned for review.
  • I've updated any terraform that needs updating (e.g. environment variables) for deployment.
  • The branch has been rebased against the head of its merge target.
  • I'm happy for the PR to be merged at the reviewer's next convenience.
  • New functions, classes, etc. have been documented according to the doxygen comment format. Classes and structs must have @brief describing the intended functionality.
  • If existing code has been modified, such documentation has been added or updated.

@kobyhallx
Copy link
Contributor Author

Test with
nix build "git+https://github.com/AztecProtocol/barretenberg?ref=kh-debug-package#wasm32-debug"

phated
phated suggested changes Apr 14, 2023
View reviewed changes
Copy link
Contributor

@phated phated left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm loving the ability to debug! I had a few questions and comments.

barretenberg-wasm.nix Outdated Show resolved Hide resolved
barretenberg-wasm.nix
] ++ (if debug then [ "-DCMAKE_BUILD_TYPE=Debug" ] else [ "-DCMAKE_BUILD_TYPE=Release" ]);


CXXFLAGS = optionals (debug) [ "-ggdb -O1" ];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't these be compile_options that are set in CMake?

flake.nix
@@ -12,6 +12,8 @@
barretenbergOverlay = final: prev: {
barretenberg = prev.callPackage ./barretenberg.nix { };
barretenberg-wasm = prev.callPackage ./barretenberg-wasm.nix { };
barretenberg-wasm-debug = prev.callPackage ./barretenberg-wasm.nix { debug = true; };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather us use .override { debug = true } in packages that want to consume barretenberg that we want to debug. Is there a reason you wanted to add another package?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This way we can build it remotely and grab just debug artifact easily. Override patter would mean that you need to be changing configs in a workspace?

@ludamad
Copy link
Collaborator

ludamad commented Apr 14, 2023 •
edited

This is awesome! Looks good to my eyes, and in the "needed yesterday" category. Only real feedback for such a hotly requested feature, I would have made a bit more noise that this is being worked on. Looks like another person was working on this, but at least that scope wasn't fully the same.

+1 for getting this into CMake

@kobyhallx @phated
Update barretenberg-wasm.nix
2737d2f 
Adopting all caps DEBUG

Co-authored-by: Blaine Bublitz <blaine.bublitz@gmail.com>
@kevaundray kevaundray changed the title feat(packages): adding debug output feat(packages): Add debug output to nix builds Jun 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phated phated phated requested changes

@ludamad ludamad Awaiting requested review from ludamad

At least 6 approving reviews are required to merge this pull request.

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
@kobyhallx @ludamad @phated