Skip to content

fix: fetch_proposals resolves relative to source file, not CWD - #153

Open
Kubudak90 wants to merge 1 commit into
ethereum:masterfrom
Kubudak90:fix/fetch-proposals-source-dir
Open

fix: fetch_proposals resolves relative to source file, not CWD#153
Kubudak90 wants to merge 1 commit into
ethereum:masterfrom
Kubudak90:fix/fetch-proposals-source-dir

Conversation

@Kubudak90

Copy link
Copy Markdown

fix: fetch_proposals resolves relative to source file, not CWD

When is given a relative source path, was
constructing relative paths for referenced EIPs. Those paths were
resolved against the process working directory, which fails when the
working directory differs from the source file directory.

This commit makes two changes:

  1. In , normalize paths to absolute
    using before computing the root directory
    for (native targets only).

  2. In , normalize source paths to absolute
    using before passing them to , so the
    JS/WASM binding behaves consistently.

A regression test verifies that requests absolute
paths even when the working directory differs from the source directory.

Closes #12

When  is given a relative source path,  was
constructing relative paths for referenced EIPs. Those paths were
resolved against the process working directory, which fails when the
working directory differs from the source file directory.

This commit makes two changes:

1. In , normalize  paths to absolute
   using  before computing the root directory
   for  (native targets only).

2. In , normalize source paths to absolute
   using  before passing them to , so the
   JS/WASM binding behaves consistently.

A regression test verifies that  requests absolute
paths even when the working directory differs from the source directory.

Closes ethereum#12
Comment thread eipw-lint-js/src/lib.rs
Comment on lines +130 to +136
let p = PathBuf::from(&s);
if p.is_relative() {
let cwd = PathBuf::from(process_cwd());
cwd.join(p)
} else {
p
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I believe join handles absolute paths on its own, so you can omit the conditional and just always join.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Wait, isn't this doing the exact opposite of the PR title? Why would we be joining the CWD anywhere if we're resolving relative to a certain file?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

You're right about join, but I think this whole conditional should go rather than be simplified — and you're right about the second point too. Looking into it properly, this PR is fixing the wrong thing. Let me lay out what I found.

There is no bug on native. With check_file("../eips/input.md") you get root = "../eips", so the sibling lookup is ../eips/eip-20.md — and Fetch resolves that against the same CWD the original path was relative to. Both are anchored to the same base, so they already agree. My eipw-lint/src/lib.rs hunk is a semantic no-op, and the test only asserts is_absolute(), i.e. it tests my change rather than the reported bug. (It also calls std::env::set_current_dir, which is process-global and races with other tests under cargo test.)

The actual bug in #12 is about path semantics, not the CWD. std::path in a wasm32-unknown-unknown build uses POSIX rules — / is the only separator. So when the caller hands in a native Windows path, the whole thing parses as a single component:

let p = Path::new(r"c:\Users\gavin\EIPs\EIPS\eip-5081.md");
p.is_absolute()  // false
p.parent()       // Some("")   <-- Some, not None, so the `.` fallback never fires
p.file_name()    // Some("c:\\Users\\gavin\\EIPs\\EIPS\\eip-5081.md")

In Linter::run that makes source_dir == "" and root == "", so root.join(&basename) + set_extension("md") comes out as bare eip-1559.md. Node's readFile then resolves it against process.cwd() — the VS Code install directory — which reproduces the error in #12 exactly, including the missing EIPS/ component:

ENOENT: no such file or directory, open 'C:\Users\gavin\AppData\Local\Programs\Microsoft VS Code\eip-1559.md'

And that means this hunk actively makes it worse. Since is_absolute() is false for a Windows path under POSIX semantics, my conditional fires on exactly the platform the issue is about, and produces:

C:\Users\gavin\AppData\Local\Programs\Microsoft VS Code/c:\Users\gavin\EIPs\EIPS\eip-5081.md

so it breaks reading the source file, which worked before.

Where the fix belongs. The mismatch is that eipw-lint does POSIX path arithmetic — correctly, it's a wasm build — while the JS binding hands it host-native paths. So normalize at that boundary and leave eipw-lint alone. Node accepts / on Windows for every fs API, so NodeFetch needs no conversion back:

// `node:path` exports `sep` — "\\" on Windows, "/" elsewhere.
#[wasm_bindgen(module = "node:path")]
extern "C" {
    #[wasm_bindgen(js_name = sep, thread_local_v2)]
    static SEP: JsString;
}

/// `eipw-lint` is compiled for wasm32, where `std::path` uses POSIX rules, so a
/// backslash-separated host path parses as a single component and defeats the
/// `parent()`/`join()` arithmetic in `Linter::run`.
fn to_posix(s: &str) -> String {
    if host_sep() == "\\" {
        s.replace('\\', "/")
    } else {
        s.to_owned()
    }
}

This keeps relative paths relative, so the origin in the reports — and the snapshots in eipw-lint-js/tests/main.rs — are unchanged.

If you'd rather eipw-lint not assume POSIX separators at all, the alternative is to move sibling resolution behind Fetch, e.g. fn resolve(&self, source: &Path, basename: &str) -> PathBuf with a POSIX default that NodeFetch overrides using node:path. That's a real API change though, so I'd default to the boundary normalization unless you prefer the trait route.

Happy to rewrite the PR that way: drop both current hunks, keep it to eipw-lint-js plus a unit test for the normalization. Let me know which you'd prefer.

Comment thread eipw-lint/src/lib.rs
Comment on lines +341 to +351
#[cfg(not(target_arch = "wasm32"))]
let source_path = if source_path.is_relative() {
std::env::current_dir()
.map(|cwd| cwd.join(source_path))
.unwrap_or_else(|_| source_path.to_path_buf())
} else {
source_path.to_path_buf()
};

#[cfg(target_arch = "wasm32")]
let source_path = source_path.to_path_buf();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is a bit weird... We shouldn't be operating differently this deep in based on architecture.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Agreed, dropping this hunk. It turns out to be a no-op on native anyway — source_path and the sibling paths derived from it are relative to the same CWD, so they already resolve consistently. The real defect is only in the wasm/JS binding, and it's a path-semantics issue rather than an architecture-conditional one; details in the other thread.

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