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(compile): Noir std lib embedded #973

Merged
merged 4 commits into from
Mar 10, 2023
Merged

feat(compile): Noir std lib embedded #973

merged 4 commits into from
Mar 10, 2023

Conversation

kobyhallx
Copy link
Contributor

Related issue(s)

Relates to #951

Description

It's beneficial to embed std lib into distributed binary:

  1. Source resolution does not concern itself with differences between execution environments (eg. native vs browser)
  2. Source resolution does not concern itself with differences between systems and default file locations
  3. Source resolution does not concern itself with differences between systems and file permissions
  4. Distribution of artifacts becomes more straightforward with less maintenance needed for installation scripts

Summary of changes

  1. noir_stdlib/src contents get embedded as StdLibAssets in crates/fm/src/file_reader.rs
  2. StdLibAssets is asked for source upfront during read_file_to_string routine
  3. crates/nargo/src/cli/mod.rs understands that StdLibAssets are embedded with std/ prefix and adds such search path for create_non_local_crate routine.

Dependency additions / changes

Adds rust-embed = "6.6.0"

Test additions / changes

N/A

Checklist

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt with default settings.
  • I have linked this PR to the issue(s) that it resolves.
  • I have reviewed the changes on GitHub, line by line.
  • I have ensured all changes are covered in the description.

Documentation needs

  • This PR requires documentation updates when merged:

Std lib no longer needs to be present on a host file system, and libraries do not need to provide the source in alternative ways, ie. source resolution in browser

Additional context

n/a

@kobyhallx kobyhallx requested a review from phated March 10, 2023 14:22
@kobyhallx kobyhallx requested a review from guipublic March 10, 2023 14:22
@kobyhallx kobyhallx enabled auto-merge March 10, 2023 14:24
crates/wasm/src/lib.rs Outdated Show resolved Hide resolved
@TomAFrench
Copy link
Member

TomAFrench commented Mar 10, 2023

Now that we'll always have the stdlib available, should we not embed the addition of the stdlib onto the end of Driver::resolve_root_config? Ah misread Resolver as Driver, in any case we should be able to abstract away the addition of the stdlib into the dependency graph.

@kobyhallx
Copy link
Contributor Author

kobyhallx commented Mar 10, 2023

... in any case we should be able to abstract away the addition of the stdlib into the dependency graph.

agreed, should be refactored to more common path at later stage

@kobyhallx kobyhallx requested a review from TomAFrench March 10, 2023 16:09
Copy link
Member

@TomAFrench TomAFrench left a comment

Choose a reason for hiding this comment

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

Looks good. I've run a quick test locally just to confirm that we're actually pulling the stdlib from an embedded version and all looks good.

@kobyhallx kobyhallx added this pull request to the merge queue Mar 10, 2023
TomAFrench added a commit to noir-lang/build-nargo that referenced this pull request Mar 10, 2023
Merged via the queue into master with commit 13b9069 Mar 10, 2023
@kobyhallx kobyhallx deleted the kh-embed-stdlib branch March 10, 2023 17:16
kobyhallx pushed a commit to noir-lang/build-nargo that referenced this pull request Mar 10, 2023
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.

2 participants