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.

Sign up for GitHub

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

Jump to bottom

fix: fix go to definition for copybook in shared drive (UNC) #2395

Merged
merged 1 commit into from
Jul 25, 2024
Merged

fix: fix go to definition for copybook in shared drive (UNC) #2395

merged 1 commit into from
Jul 25, 2024

Conversation

ap891843
Copy link
Contributor

@ap891843 ap891843 commented Jul 23, 2024 •
edited
Loading

fix go to definition for copybook in shared drive (UNC).
Vs Code is unable to resolve below UNC path
file:////<hostname>/Users/FILE ,
but can resolve file://<hostname>/Users/FILE

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Configure copybooks on an accessible shared drive. Include these copybooks in a Cobol source. After the Cobol source is analyzed , try go to definition for copybooks in the shared drive. Vs code should be able to locates copybooks

Checklist:

  • Each of my commits contains one meaningful change
  • I have performed rebase of my branch on top of the development
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have checked my code and corrected any misspellings

@ap891843 ap891843 force-pushed the fix/unc_path_cpybk_go_to_def branch 2 times, most recently from 2e61f09 to 6308ceb Compare July 23, 2024 10:37
@ap891843 ap891843 marked this pull request as ready for review July 23, 2024 11:18
@slavek-kucera
Copy link
Contributor

Is there an impact on processor groups (either library or program spec being UNC)?
Are dialects impacted?
Is it feasible to create a test that would run in the CI e.g. open a folder \\.\<whatever>?

ap891843
ap891843 commented Jul 24, 2024
View reviewed changes
clients/cobol-lsp-vscode-extension/src/test/suite/lsp.spec.test.ts Outdated
await helper.showDocument("TESTCPY1.cbl");
const progUri = await helper.getUri("TESTCPY1.cbl");
let editor = await helper.showDocument("TESTCPY1.cbl");
const progUri = editor.document.uri;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

VsCode API has a different behavior for below 2 cases
vscode.languages.getDiagnostics(vscode.Uri.file("c:/Users/TESTCPY1.cbl")) <-- SHOWS DIAGNOSTICS
vscode.languages.getDiagnostics(vscode.Uri.file("//./c:/Users/TESTCPY1.cbl")) <-- NO DIAGNOSTICS

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the test is not opening the UNC path.

path.join(__dirname, "../../../../tests/test_files/project")

would need to be prefixed in the launchArgs with \\.\

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried something in the same line here

I see paths being passed as file:///./c%3A/Users/che-che4z-lsp-for-cobol/tests/test_files/project/TESTCPY1.cbl, when opening the document

ap891843
ap891843 commented Jul 24, 2024
View reviewed changes
clients/cobol-lsp-vscode-extension/src/test/runTest.ts Outdated
if (process.platform === "win32") {
console.log("----- RUNNING TEST USING UNC PATH ------");
options.extensionTestsEnv = { UNC_PREFIX: "\\." };
await runTests(options);
Copy link
Contributor Author

@ap891843 ap891843 Jul 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Is it ok to run the UNC tests like this? or handle it using custom mocha test runner or something in that line

Copy link
Contributor

Choose a reason for hiding this comment

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

In general, I think this would be fine. But the UNC path needs to be passed to vscode e.g.

launchArgs[0] = "\\\\.\\" + launchArgs[0];

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! let me try that

Copy link
Contributor Author

@ap891843 ap891843 Jul 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Found below issue while adding UNC tests -

  • Java doesn't mind path like \\<hostname>\cobolls-test , but FAILS for path like \\.\c:\cobolls-test

Caused by: java.nio.file.InvalidPathException: Illegal character [:] in path at index 5: \\.\c:\cobolls-test
Also, there are a few bugs around UNC path JDK-8218618, which results in a failure at random places. For e.g. this bug resulted me in an error for UUID.randomUUID(), as it internally relies on some nio packages.

So,
code \\.\c:\Users\testUsr\cobolls-test <-- doesn't work
code \\HOSTNAME\Users\testUsr\cobolls-test <-- works

But I guess, idea was to add support for copybook resolution in the shared drive. So, I plan to add a test for copybook resolution using UNC path. Do you think, its ok? please suggest

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible that you are running unpatched version of java? The issue, you linked, mentions two v11 versions that are supposed to be fixed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was even able to read an NTFS alternate stream using UNC path e.g. \\.\c:\Source\test.txt:XYZ

@ap891843
Revert "test: Add UNC integration tests"
3c99227 
This reverts commit c1a3cb65a55d99b8e66b1169b3bfa910fb6b39d0.

test: Add UNC integration tests

Signed-off-by: ap891843 <aman.prashant@broadcom.com>

fix: fix go to definition for copybook in shared drive (UNC)

fix go to definition for copybook in shared drive (UNC).
VsCode is unable to resolve below unc path
 file:////<hostname>/Users/FILE ,
 but can resolve  file://<hostname>/Users/FILE

Signed-off-by: ap891843 <aman.prashant@broadcom.com>
@ap891843 ap891843 force-pushed the fix/unc_path_cpybk_go_to_def branch from daadcea to 3c99227 Compare July 25, 2024 11:00
@ap891843 ap891843 merged commit 38a0ffd into eclipse-che4z:development Jul 25, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@slavek-kucera slavek-kucera slavek-kucera approved these changes

@VitGottwald VitGottwald Awaiting requested review from VitGottwald

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ap891843 @slavek-kucera