jvalue / jayvee

Jayvee is a domain-specific language and runtime for automated processing of data pipelines
https://jvalue.github.io/jayvee/
150 stars 15 forks source link

[BUG] Windows Filepaths #623

Closed georg-schwarz closed 1 month ago

georg-schwarz commented 1 month ago

Steps to reproduce

  1. Install Jayvee v0.6.2 on Windows machine
  2. Run the cars.jv example with jv -d cars.jv

Note: This seems to be a Windows-only issue.

Description

- Actual: Output

Did not load file cars.jv correctly. Could not extract the AST node of the given model-

georg-schwarz commented 1 month ago

The error seems to raised here: https://github.com/jvalue/jayvee/blob/7a02683ac1c4ec63b4fc6754ab3debfccd4a6468/libs/interpreter-lib/src/parsing-util.ts#L47-L53

The initialization happens here: https://github.com/jvalue/jayvee/blob/7a02683ac1c4ec63b4fc6754ab3debfccd4a6468/apps/interpreter/src/run-action.ts#L37-L58 We add cwd.process() to the list of workdirs, and later on retrieve the added document (see first code snippet) based on a relative path.

Current fault hypotheses:

  1. Adding the directory to the workspace does not behave correctly on Windows
  2. Getting the document based on the relative URI does not work on Windows
  3. The interchanging use of relative and absolute URIs causes issues on Windows
georg-schwarz commented 1 month ago

New insights: The document is registered via the following key on Windows: \Users\wagne\Downloads\cars.jv. The lookup, however, uses this path: c:\Users\wagne\Downloads\cars.jv. https://github.com/jvalue/jayvee/blob/7a02683ac1c4ec63b4fc6754ab3debfccd4a6468/libs/interpreter-lib/src/parsing-util.ts#L47-L49

Thx @Waldleufer for helping me to debug this!

georg-schwarz commented 1 month ago

Further insights:

Langium uses the method UriUtils.joinPath when traversing the directory: https://github.com/eclipse-langium/langium/blob/b52cb60f097fd6046f6265e3e72a9cee253df061/packages/langium/src/node/node-file-system-provider.ts#L22-L30

However, this util method uses linux paths https://github.com/eclipse-langium/langium/blob/b52cb60f097fd6046f6265e3e72a9cee253df061/packages/langium/src/utils/uri-utils.ts#L23-L38

So the registered document has path: C:%5CUsers%5Cwagne%5CDownloads/cars.jv - note the / The closest we can get with URI.parse(path.resolve(filePath)).toString() is C:%5CUsers%5Cwagne%5CDownloads%5Ccars.jv - note the missing %5C instead of the /

georg-schwarz commented 1 month ago

I see two fixes, while waiting for Langium to fix this in a future version, both in this area: https://github.com/jvalue/jayvee/blob/7a02683ac1c4ec63b4fc6754ab3debfccd4a6468/libs/interpreter-lib/src/parsing-util.ts#L47-L49

  1. We create the document a second time on Windows machines:
    const fileUri = URI.parse(path.resolve(filePath));
    const document =
    await services.shared.workspace.LangiumDocuments.getOrCreateDocument(
    fileUri,
    );
  2. We replicate the path magic that Langium uses to get the document via the matching (but faulty) path:
    
    const folderPath = path.dirname(filePath);
    const folderUri = URI.parse(path.resolve(folderPath));
    const fileName = path.basename(filePath);
    // Exactly same implementation as in Langium to make sure document lookup works on Windows
    // https://github.com/jvalue/jayvee/issues/623
    const fileUrl = UriUtils.joinPath(folderUri, fileName);

const document = services.shared.workspace.LangiumDocuments.getDocument(fileUrl);



What do you prefer? @rhazn @joluj 
rhazn commented 1 month ago

Second one is more clear and links to the issue, so I would go with that I think.

georg-schwarz commented 1 month ago

Ok, this will have the caveat that once we have a fix in Langium, Windows users will have an issue again (because the replicated implementation changes). But I agree it is better since the problem is not masked.

I created an issue in the langium repo: https://github.com/eclipse-langium/langium/issues/1725