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

feat: Install noir-profiler and noir-inspector #46

Merged
merged 26 commits into from
Feb 20, 2025
Merged

Conversation

vezenovm
Copy link
Contributor

@vezenovm vezenovm commented Feb 19, 2025

Description

Problem*

Resolves #42

Summary*

This PR simply adds handling for installation of the noir-profiler from source. It uses the new tarball name noir-{architecture}-{platform} that contains all Noir binaries after noir-lang/noir#7443.

Adding an additional binary requires a couple changes to be compatible with older versions of Noir. Most notably we do a check against curl {new binary url} to determine whether we need to use the old binary url name.

When compiling from source we switched to checking whether a binary exists before attempting to move it.

Additional Context

We probably could also rename .nargo to .noir as we now have Noir binaries that are not contained to just nargo. However, I feel we can do this switch later.

PR Checklist*

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

@vezenovm vezenovm requested a review from TomAFrench February 19, 2025 21:52
Comment on lines 82 to 86
- name: Check noir-profiler installation
run: |
if [ -f "${HOME}/.nargo/bin/noir-profiler" ]; then
noir-profiler --version
fi
Copy link
Member

Choose a reason for hiding this comment

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

This test is a little tuatological. We're running noir-profiler --version to check if it's there but only if it's there.

What I've done elsewhere in this workflow is to split the version number into major, minor, patch and we can then assert that after a certain version we expect the profiler to exist.

@TomAFrench
Copy link
Member

LGTM, let's merge this after 1.0.0-beta.3 goes out so we can test it with a noir-profiler binary in the release.

@TomAFrench TomAFrench changed the title feat: Install noir-profiler feat: Install noir-profiler and noir-inspector Feb 20, 2025
@TomAFrench TomAFrench merged commit cf71599 into main Feb 20, 2025
129 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Install the flamegraph tool alongside nargo
2 participants