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(nargo): resolve local dependencies relative to root of depending package #1034

Merged
merged 4 commits into from
Mar 25, 2023

Conversation

ax0
Copy link
Contributor

@ax0 ax0 commented Mar 23, 2023

Related issue(s)

Resolves #

Description

When running nargo in a subdirectory of a package whose Nargo.toml file contains a local dependency expressed as a relative path (e.g. pkg = { path = "../../lib" }), the dependency is parsed relative to the subdirectory, resoluting in an error of the form

Error: cannot find src directory in path ../../lib

Location:
    crates/nargo/src/cli/mod.rs:69:5

This PR sets the current directory to the project root before executing nargo commands.

Summary of changes

  • A call to std::env::set_current_dir after determining the project root.

Dependency additions / changes

Test additions / changes

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.

Additional context

BEGIN_COMMIT_OVERRIDE
fix(nargo): resolve local dependencies relative to root of depending package
END_COMMIT_OVERRIDE

@kevaundray kevaundray requested a review from TomAFrench March 23, 2023 21:05
@TomAFrench
Copy link
Member

TomAFrench commented Mar 23, 2023

Thanks for flagging up this edge case, I'll make sure to add some tests to run commands away from the package root at some point. WRT this PR though, I think of this as just another occurrence of #1013.

My preference for this is that we pass the package root directory (i.e. manifest_path.parent()) into resolver.resolve_manifest so that local paths can be resolved relative to this. We'd avoid magical linkages from code in start_cli fixing issues in the resolver (i.e. we shouldn't rely on current_dir being set correctly) and also support chained local dependencies at the same time for free.

jfecher
jfecher previously approved these changes Mar 24, 2023
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.

LGTM.

@TomAFrench what do you think?

@TomAFrench
Copy link
Member

LGTM however I haven't tested it locally.

crates/nargo/src/resolver.rs Outdated Show resolved Hide resolved
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.

Tested this locally and we're for a dependency chain A->B->C, we're now properly resolving C relative to B rather than A

@TomAFrench TomAFrench added this pull request to the merge queue Mar 25, 2023
Merged via the queue into noir-lang:master with commit 38bf571 Mar 25, 2023
@TomAFrench TomAFrench changed the title fix(nargo): Set current dir to package root before executing commands fix(nargo): resolve local dependencies relative to root of depending package Mar 25, 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.

4 participants