Skip to content

Refactor stdlib into a separate noir library#443

Merged
kevaundray merged 5 commits intonoir-lang:masterfrom
lambdaclass:master
Nov 8, 2022
Merged

Refactor stdlib into a separate noir library#443
kevaundray merged 5 commits intonoir-lang:masterfrom
lambdaclass:master

Conversation

@ilitteri
Copy link
Contributor

@ilitteri ilitteri commented Nov 5, 2022

Related issue(s)

Resolves #423

Description

Summary of changes

This PR abstracts the driver from the stdlib addition and moves the stdlib as a standalone nargo crate to the package manager level and when nargo is installed it is copied to the native config dir for the driver to look up to when adding it as an implicit dependency when crating a new project.

Dependency additions / changes

dirs is added as a build-dependency in nargo.

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.

Additional context

N/A

ilitteri and others added 2 commits November 5, 2022 10:00
* Remove std_lib package

* Update path_to_stdlib

Given that now the stdlib is a standalone nargo crate

* Copy stdlib nargo crate to the native config dir

* Move stdlib to the package manager level

As a standalone nargo crate

* Add build.rs dependencies

* Return aztec_backend dependency back to its origin commit as it was modified accidentally.

* Abstract driver module from the stdlib addition

Now the stdlib addition is a responsibility for nargo.

Note: It needs to add it before invoking into_compiled_program in order for the resolver to find the stdlib as a dependency of the crate.

* cargo fmt

* Change needless pass value

This argument was passed by value, but not consumed in the function body

* Update file_compiles for tests

* Move tests to the package manager level as well

* Add tests for the build cmd

To ensure that the changes made work as expected
Copy link
Contributor

@kevaundray kevaundray left a comment

Choose a reason for hiding this comment

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

LGTM. Just a few comments on creating tracking issues for some TODOs

Copy link
Contributor

@guipublic guipublic 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 understand the purpose of this PR, and in particular having the std_lib not been a standalone crate anymore does not seem a good move to me.

@ilitteri ilitteri mentioned this pull request Nov 7, 2022
5 tasks
@guipublic
Copy link
Contributor

I don't understand the purpose of this PR, and in particular having the std_lib not been a standalone crate anymore does not seem a good move to me.

We had an internal discussion and what I got from it is that the purpose of the PR is to removing dependency on the rust compiler for the std::lib, so that we do not need additional installation steps for instance.
However we agreed not to move it to a Nargo subdirectory, keep it on top-level but remove it from the compiler directory and have it managed by Nargo like you did.

@ilitteri
Copy link
Contributor Author

ilitteri commented Nov 7, 2022

I don't understand the purpose of this PR, and in particular having the std_lib not been a standalone crate anymore does not seem a good move to me.

We had an internal discussion and what I got from it is that the purpose of the PR is to removing dependency on the rust compiler for the std::lib, so that we do not need additional installation steps for instance. However we agreed not to move it to a Nargo subdirectory, keep it on top-level but remove it from the compiler directory and have it managed by Nargo like you did.

Great! Let me know if more changes are needed, I'll be glad to make them.

@kevaundray kevaundray self-requested a review November 8, 2022 17:16
@kevaundray kevaundray merged commit f223871 into noir-lang:master Nov 8, 2022
jfecher pushed a commit that referenced this pull request Nov 17, 2022
jfecher pushed a commit that referenced this pull request Nov 17, 2022
TomAFrench added a commit to TomAFrench/noir that referenced this pull request Nov 19, 2022
* master:
  Array sort (noir-lang#477)
  Add `--allow-warnings` flag to treat certain errors as warnings (noir-lang#481)
  Improve field comparison error message (noir-lang#499)
  Disable debug output during integration tests (noir-lang#494)
  Aligns build to (noir-lang#443) Refactor stdlib into a separate noir library (noir-lang#496)
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.

Refactor stdlib into a separate noir library

3 participants