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: Create FileManager with a root and normalize filenames against it #1881

Merged
merged 3 commits into from
Jul 19, 2023

Conversation

phated
Copy link
Contributor

@phated phated commented Jul 6, 2023

Description

Problem*

Resolves #1869
Resolves #1599 (this can still be cleaned up a bit by taking a reference & adding a lifetime to context)

Summary*

This changes the FileManager so that it requires a root when constructed. It then resolves the root against filenames passed to add_file and uses a custom normalize_path function to resolve . and .. in the resulting file paths. This avoids the canonicalize function which goes out to the filesystem and is unimplemented!() in WASM.

Since the FileManager must be constructed with a root, I've also removed the Default deriver and constructed Context manually in each location we need one.

I've tried to think through all of the ramifications of this change, but I probably missed something that @TomAFrench will catch. Nevertheless, I think this is a step in the right direction because we aren't reliant on the current_dir of the running process.

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.

@phated phated force-pushed the phated/filemanager-root branch 2 times, most recently from 0620d5c to 781f93b Compare July 7, 2023 19:24
Copy link
Member

@Maddiaa0 Maddiaa0 left a comment

Choose a reason for hiding this comment

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

I really appreciate you doing this @phated, this looks good to me

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.

I'm getting test failures if I swap out normalize_path for std::fs:canonicalize when using my alternative impl which only normalizes files which should exist. This seems odd so will going to take a little bit of an extra look before approving.

crates/fm/src/lib.rs Outdated Show resolved Hide resolved
crates/noirc_driver/src/main.rs Outdated Show resolved Hide resolved
crates/noirc_driver/src/main.rs Outdated Show resolved Hide resolved
crates/fm/src/lib.rs Show resolved Hide resolved
crates/fm/src/file_reader.rs Outdated Show resolved Hide resolved
crates/fm/src/file_reader.rs Outdated Show resolved Hide resolved
@TomAFrench TomAFrench changed the title fix(fm): Create FileManager with a root and normalize filenames against it fix: Create FileManager with a root and normalize filenames against it Jul 18, 2023
@TomAFrench
Copy link
Member

This looks good to me once all the conflicts are fixed. Took a look at addressing this myself but I'm not sure what the deal is with lsp_hacky and don't want to use an incorrect path in there and break things.

@phated phated force-pushed the phated/filemanager-root branch from 075cbb2 to 870c45d Compare July 18, 2023 20:12
@phated phated requested a review from TomAFrench July 18, 2023 20:16
Copy link
Contributor

@kobyhallx kobyhallx left a comment

Choose a reason for hiding this comment

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

LGTM

@kobyhallx kobyhallx added this pull request to the merge queue Jul 19, 2023
Merged via the queue into master with commit 50c1648 Jul 19, 2023
@kobyhallx kobyhallx deleted the phated/filemanager-root branch July 19, 2023 16:45
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.

Path canonicalize doesn't have an implementation in wasm FileManager should be controlled by nargo
5 participants