Skip to content

Resolve symlink of the matlab executable when finding install path #22

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

Merged
merged 1 commit into from
Nov 6, 2023
Merged

Conversation

moetayuko
Copy link
Contributor

@moetayuko moetayuko commented Oct 20, 2023

@dklilley dklilley self-requested a review October 25, 2023 17:47
@dklilley dklilley self-assigned this Oct 25, 2023
Copy link
Member

@dklilley dklilley left a comment

Choose a reason for hiding this comment

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

Thanks for looking into this!

@nothans
Copy link
Member

nothans commented Oct 27, 2023

Can you contact me to sign a CLA? Thank you for the PR!

@nothans
Copy link
Member

nothans commented Nov 1, 2023

The CLA has been signed. Please evaluate the PR. Thank you for the contribution!

@dklilley
Copy link
Member

dklilley commented Nov 1, 2023

Hi @moetayuko, I am trying this out but I think this is not behaving as desired. When I try this, I am seeing that line 356 is executed before the fs.realpath callback is executed, resulting in a null return every time.

Is this what you are seeing as well?

@moetayuko
Copy link
Contributor Author

Hi @moetayuko, I am trying this out but I think this is not behaving as desired. When I try this, I am seeing that line 356 is executed before the fs.realpath callback is executed, resulting in a null return every time.

Is this what you are seeing as well?

Nope, it simply works as expected on my side.

  • I didn't manually set matlabInstallPath so that the "else" block on line 338 is entered, then the "try" block.
  • I have a symlink /usr/bin/matlab -> /opt/MATLAB/R2023a/bin/matlab where /usr/bin is in $PATH obviously. Before this PR binPath was assigned /usr/bin, but now it's /opt/MATLAB/R2023a/bin which is expected.

@dklilley
Copy link
Member

dklilley commented Nov 2, 2023

I think this may be working for you somewhat by luck. Since you are using the non-promise version of fs.realpath, I don't believe the await has any effect. If it resolves fast enough, the callback function may be executed before the try is exited, however this is not guaranteed.

@moetayuko
Copy link
Contributor Author

I think this may be working for you somewhat by luck. Since you are using the non-promise version of fs.realpath, I don't believe the await has any effect. If it resolves fast enough, the callback function may be executed before the try is exited, however this is not guaranteed.

Indeed. You reminded me that async shouldn't be used. Now I switched to the promise version so it should be working.

@dklilley
Copy link
Member

dklilley commented Nov 3, 2023

Thanks for making that change!

For me, realpath is resolving to /usr/local/MATLAB/R2023b/bin/matlab, which is expected. However, for this to work, I believe that binPath needs to point to /usr/local/MATLAB/R2023b/bin/.

@moetayuko
Copy link
Contributor Author

Oops, my bad, this revision must be okay...

Copy link
Member

@dklilley dklilley left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this issue!

@dklilley dklilley merged commit 319868d into mathworks:main Nov 6, 2023
diningPhilosopher64 pushed a commit to diningPhilosopher64/MATLAB-language-server that referenced this pull request Jul 2, 2024
…metry

Language Server - Telemetry Logging
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants