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

fix(crates): do not process relative dependencies twice #1856

Merged
merged 9 commits into from
Jul 4, 2023

Conversation

Maddiaa0
Copy link
Member

@Maddiaa0 Maddiaa0 commented Jul 4, 2023

Description

Currently in the aztec3 repo we are unable to import helper dependencies that rely on the aztec3-noir standard library that we have created.

The module structure is something like this:

- root
- library
- helper

in our case both helper depends on library and helper, where helper also depends on library. This leads to noir compiling the library files, then recompiling them when processing the helper.

This leads to instances like below, where the same type has two different idents that mismatch.
image

This pr fixes this issue by recognising when a dependant local crate has already been compiled, and not compiling it again

Problem*

Resolves

Summary*

Documentation

  • This PR requires documentation updates when merged.

    • I will submit a noir-lang/docs PR.
    • I will request for and support Dev Rel's help in documenting this PR.

Additional Context

PR Checklist*

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

crates/fm/src/lib.rs Outdated Show resolved Hide resolved
@Maddiaa0 Maddiaa0 marked this pull request as ready for review July 4, 2023 13:25
crates/fm/src/lib.rs Outdated Show resolved Hide resolved
crates/fm/src/lib.rs Show resolved Hide resolved
@Maddiaa0 Maddiaa0 requested a review from jfecher July 4, 2023 14:21
@jfecher
Copy link
Contributor

jfecher commented Jul 4, 2023

Looks like graph::tests::it_works2 is failing

Copy link
Contributor

@jfecher jfecher left a comment

Choose a reason for hiding this comment

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

👍

@jfecher jfecher enabled auto-merge July 4, 2023 15:26
@jfecher jfecher added this pull request to the merge queue Jul 4, 2023
Merged via the queue into master with commit b2e71bb Jul 4, 2023
5 checks passed
@jfecher jfecher deleted the md/fix-circ-deps branch July 4, 2023 16:03
@phated
Copy link
Contributor

phated commented Jul 5, 2023

I believe this broke wasm because (last time I checked) the canonicalize function is not supported by wasi. I had replaced this in circom with an alternative impl.

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.

5 participants