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

[BUG] Windows Filepaths #623

Closed
georg-schwarz opened this issue Oct 22, 2024 · 6 comments · Fixed by #626
Closed

[BUG] Windows Filepaths #623

georg-schwarz opened this issue Oct 22, 2024 · 6 comments · Fixed by #626
Labels
bug Something isn't working

Comments

@georg-schwarz
Copy link
Member

georg-schwarz commented Oct 22, 2024

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

  • Expected: Output
Found 1 pipelines to execute: CarsPipeline
[CarsPipeline] Overview:
        Blocks (6 blocks with 2 pipes):
         -> CarsExtractor (HttpExtractor)
                 -> CarsTextFileInterpreter (TextFileInterpreter)
                         -> CarsCSVInterpreter (CSVInterpreter)
                                 -> NameHeaderWriter (CellWriter)
                                         -> CarsTableInterpreter (TableInterpreter)
                                                 -> CarsLoader (SQLiteLoader)

        [CarsExtractor] Fetching raw data from https://gist.githubusercontent.com/noamross/e5d3e859aa0c794be10b/raw/b999fb4425b54c63cab088c0ce2c0d6ce961a563/cars.csv
        [CarsExtractor] Successfully fetched raw data
        [CarsExtractor] Execution duration: 85 ms.
        [CarsTextFileInterpreter] Decoding file content using encoding "utf-8"
        [CarsTextFileInterpreter] Splitting lines using line break /\r?\n/
        [CarsTextFileInterpreter] Lines were split successfully, the resulting text file has 33 lines
        [CarsTextFileInterpreter] Execution duration: 0 ms.
        [CarsCSVInterpreter] Parsing raw data as CSV using delimiter ","
        [CarsCSVInterpreter] Parsing raw data as CSV-sheet successful
        [CarsCSVInterpreter] Execution duration: 9 ms.
        [NameHeaderWriter] Writing "name" at cell A1
        [NameHeaderWriter] Execution duration: 0 ms.
        [CarsTableInterpreter] Matching header with provided column names
        [CarsTableInterpreter] Validating 32 row(s) according to the column types
        [CarsTableInterpreter] Validation completed, the resulting table has 32 row(s) and 12 column(s)
        [CarsTableInterpreter] Execution duration: 1 ms.
        [CarsLoader] Opening database file ./cars.sqlite
        [CarsLoader] Dropping previous table "Cars" if it exists
        [CarsLoader] Creating table "Cars"
        [CarsLoader] Inserting 32 row(s) into table "Cars"
        [CarsLoader] The data was successfully loaded into the database
        [CarsLoader] Execution duration: 11 ms.
[CarsPipeline] Execution duration: 108 ms.

  • Actual: Output
Did not load file cars.jv correctly.
Could not extract the AST node of the given model-
@georg-schwarz georg-schwarz added the bug Something isn't working label Oct 22, 2024
@georg-schwarz
Copy link
Member Author

georg-schwarz commented Oct 22, 2024

The error seems to raised here:

const document = services.shared.workspace.LangiumDocuments.getDocument(
URI.file(path.resolve(filePath)),
);
if (document === undefined) {
logger.logErr(`Did not load file ${filePath} correctly.`);
return Promise.reject(ExitCode.FAILURE);
}

The initialization happens here:

const currentDir = process.cwd();
const workingDir = currentDir;
const filePathRelativeToCurrentDir = path.relative(currentDir, filePath);
const interpreter = new DefaultJayveeInterpreter({
pipelineMatcher: (pipelineDefinition) =>
pipelineRegExp.test(pipelineDefinition.name),
env: options.env,
debug: options.debug,
debugGranularity: options.debugGranularity,
debugTarget: options.debugTarget,
}).addWorkspace(workingDir);
if (options.parseOnly === true) {
return await runParseOnly(filePathRelativeToCurrentDir, interpreter);
}
const exitCode = await interpreter.interpretFile(
filePathRelativeToCurrentDir,
);
process.exit(exitCode);
}

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
Copy link
Member Author

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.

const document = services.shared.workspace.LangiumDocuments.getDocument(
URI.file(path.resolve(filePath)),
);

Thx @Waldleufer for helping me to debug this!

@georg-schwarz
Copy link
Member Author

georg-schwarz commented Oct 24, 2024

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
Copy link
Member Author

georg-schwarz commented Oct 24, 2024

I see two fixes, while waiting for Langium to fix this in a future version, both in this area:

const document = services.shared.workspace.LangiumDocuments.getDocument(
URI.file(path.resolve(filePath)),
);

  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,
  );
  1. 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
Copy link
Contributor

rhazn commented Oct 24, 2024

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

@georg-schwarz
Copy link
Member Author

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: eclipse-langium/langium#1725

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants