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

fix lookup and automatic installation of fortls on Windows #772

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

michaelkonecny
Copy link

@michaelkonecny michaelkonecny commented Dec 10, 2022

Hey there, gnikit,

-- personal message ;) --
first off, let me say what a tremendous job you have done with fortls!
The extension is leaps ahead from what it was two years ago. I only managed to update a couple of months ago after you posted to Discourse and I was amazed.
-- end personal message --

I hopefully made the extension a bit more usable on Windows, where pip installs fortls in the %appdata%\Roaming\Python\Python311\Scripts\ folder, which is typically not in PATH, so the extension wouldn't find fortls after installing it.
So the only way to use it would be to enter the path to the above folder to the config or to install fortls with admin privileges.
In any case, automatic installation by the extension or updates weren't possible.

Now the extension also looks for fortls in this folder, so all of that is gone.

Other changes:

  • The user configured path to fortls must now be absolute.
    This simplified a lot of things and it doesn't make sense to me to
    have multiple versions of fortls on the system, per workspace.
    Please let me know if this is not OK.
  • The fortls.disabled config value now gets stored in the USER settings
    instead of workspace. Similar reasons as above, it seems easier to
    find.
  • A check for python presence is performed before running pip.
  • On Windows, python is used instead of python3 as this is the standard command name on the platform.
  • Added --end-of-line auto argument to prettier to keep me sane.
    (Otherwise prettier would convert everything to LF, which results in hundreds of modified files in git, because git checks out files with CRLF by default on Windows. They're still commited with LF, don't worry.).

pip installs fortls in the %appdata%\Roaming\Python\Python311\Scripts\
folder, which is typically not in PATH, so the extension wouldn't find
fortls after installing it.
Now it also looks for fortls in this folder.

Other changes:
- The user configured path to fortls must now be absolute.
  This simplified a lot of things and it doesn't make sense to me to
  have multiple versions of fortls on the system, per workspace.
  Please let me know if this is not OK.
- The fortls.disabled config value now gets stored in the USER settings
  instead of workspace. Similar reasons as above, it seems easier to
  find.
@michaelkonecny michaelkonecny changed the title look for fortls in the user Scripts folder on Windows fix lookup and automatic installation of fortls on Windows Dec 10, 2022
fixes pre-commit hook failing with the following message:
```
npm run format:
[error] No files matching the pattern were found: "'src/**/*.{ts,json}'".
[error] No files matching the pattern were found: "'test/**/*.ts'".
[error] No files matching the pattern were found: "'syntaxes/**/*.json'".
[error] No files matching the pattern were found: "'snippets/**/*.json'".
[error] No files matching the pattern were found: "'./**/*.{md,json,yaml,yml}'".
```
Copy link
Member

@gnikit gnikit left a comment

Choose a reason for hiding this comment

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

Thanks @michaelkonecny for the interest in contributing but I think this is not as straight forward as it looks. We have to think about it a bit before we spring to action. Also, please try and do one feature change per PR. As it is there is a lot of work to review for what should be a minor fix.

I would we scrape this PR and address one issue at a time.


  • The user configured path to fortls must now be absolute.
    This simplified a lot of things and it doesn't make sense to me to
    have multiple versions of fortls on the system, per workspace.
    Please let me know if this is not OK.

We cannot force people to use absolute paths. A lot of people work with workspace local virtual environments, some companies use weird project configurations that require them to use relative paths or it's more convenient for them to use relative paths.

  • The fortls.disabled config value now gets stored in the USER settings
    instead of workspace. Similar reasons as above, it seems easier to
    find.

I like this idea but there is a reason why I chose not implement it this way. It was for the people that truly wanted an editor for Fortran without any bells and whistles. I suspect that it might be time to re-evaluate this or offer the option to do both.

On another note, I think the proposed solution for spawning fortls will break in some edge cases because of the asynchronicity in the initialiser. The other part is that by moving the this.path into the activation function removes the ability of users being able to change the path to fortls without restarting vsocde. There is a reason why the fortls initialisation is written that way.

In the client.ts there are a lot of changes that are not clear to me why they are necessary and what they are addressing. This seems more like a code refactoring which should not be part of this PR.

  • A check for python presence is performed before running pip.

Not sure if this is worth it. Almost any OS or Fortran development environment comes with Python (Linux, FreeBSD, MacOS, WSL, MinGW, Cygwin, Intel OneAPI). If the user doesn't have Python in their system and reachable, it is a safe bet that they will also have troubles compiling any code. The way that we plan to guide new users through the installation bit is via Walkthroughs, which I have yet to write see #472

  • On Windows, python is used instead of python3 as this is the standard command name on the platform.

This is also incorrect (which I just learned myself), it should be py not python, see https://packaging.python.org/en/latest/tutorials/installing-packages/#installing-from-pypi (Windows option). I have no idea why Windows has these nuances, but I guess we will have to work around it.

Added --end-of-line auto argument to prettier to keep me sane.
(Otherwise prettier would convert everything to LF, which results in hundreds of modified files in git, because git checks out files with CRLF by default on Windows. They're still commited with LF, don't worry.).

I don't think I understand what the issue is. All files should be formatted and committed with LF in all OSs. Does prettier format something that it shouldn't on Windows hence the large diffs? Setting this to auto can lead to mixed line endings which we can't permit. Using WSL is the safest solution IMO. BTW VS Code should be able to account for LF or CRLF on any OS.


The greater question with this PR I guess is how do people configure their Python environments on Windows? How do they reach binaries installed via pip? On Linux and MacOS the user install location for pip is not in the PATH either. There is just the assumption, potentially erroneously, that the user has done some minimal setup for their development environment. It might be worth doing a try-except if binary X is not found in default vscode settings try the "default" pip location first, which might be outside of PATH.

@michaelkonecny
Copy link
Author

Thanks for a quick review!
Sorry about the PR being a bit messy/containing multiple features. It is my first PR containing more than a couple of lines of code. I will try to be more granular next time.


We cannot force people to use absolute paths. A lot of people work with workspace local virtual environments, some companies use weird project configurations that require them to use relative paths or it's more convenient for them to use relative paths.

Understood. I will fix this.

The fortls.disabled config value now gets stored in the USER settings
instead of workspace. Similar reasons as above, it seems easier to
find.

I like this idea but there is a reason why I chose not implement it this way. It was for the people that truly wanted an editor for Fortran without any bells and whistles. I suspect that it might be time to re-evaluate this or offer the option to do both.

I'm not sure you understood what I meant. The extension still allows all settings to be set in both user and the workspace settings.
What I want to change is what happens when you click Disable in the Forlts wasn't found on your system. prompt.
I think it makes more sense to store the disabled state in the user setting than in the workspace setting as more users will install fortls globally (I for one think in most cases the extension should install it for them semi-automatically and they shouldn't worry about it). Do you agree?

On another note, I think the proposed solution for spawning fortls will break in some edge cases because of the asynchronicity in the initialiser.

I'm new to the async await concept (this was my first time doing anything in TypeScript), but I thought I thought about it sufficiently. I will look into it again, though, to be sure.

The other part is that by moving the this.path into the activation function removes the ability of users being able to change the path to fortls without restarting vsocde. There is a reason why the fortls initialisation is written that way.

Understood. Will fix this.

In the client.ts there are a lot of changes that are not clear to me why they are necessary and what they are addressing. This seems more like a code refactoring which should not be part of this PR.

The refactoring made sense for the changes I made. But I guess there's no point going into detail now, before I make the changes you requested...

A check for python presence is performed before running pip.

Not sure if this is worth it. Almost any OS or Fortran development environment comes with Python (Linux, FreeBSD, MacOS, WSL, MinGW, Cygwin, Intel OneAPI). If the user doesn't have Python in their system and reachable, it is a safe bet that they will also have troubles compiling any code. The way that we plan to guide new users through the installation bit is via Walkthroughs, which I have yet to write see #472

I would argue so.
In my opinion, the extension should work as effortlessly as possible.
An example for this being my boss, a mechanical engineer, who has never used python and I know he won't, unless he's absolutely forced to. He's never worked with a package manager, so obviously he doesn't know pip either.
These are already two obstacles for him to get fortls working instead of just saying "just install python and than this".
I know another company here in Brno, where at least half of the Fortran users are similar 50+ year old guys who I'm sure don't regularly use python either.
I want the extension to work for them as well.

Also, expecting Python to be installed alongside a Fortran compiler is not such a safe bet as you say it is, in my opinion. Intel OneAPI does come with their distribution of Python, but nothing forces you to install it. You don't need it to compile Fortran code. I've compiled several open-source libraries (arpack being one of them, off the top of my head) without needing Python.

Also, if we keep the Python check, it could be improved by also checking the python version, as fortls only supports >= 3.7 (however this is probably not critical...).

This is also incorrect (which I just learned myself), it should be py not python, see https://packaging.python.org/en/latest/tutorials/installing-packages/#installing-from-pypi (Windows option). I have no idea why Windows has these nuances, but I guess we will have to work around it.

I'm sure a bad enough reason exists. OK, will fix this to py, will leave python3 on other platforms. Thanks for that.

Added --end-of-line auto argument to prettier to keep me sane.
(Otherwise prettier would convert everything to LF, which results in hundreds of modified files in git, because git checks out files with CRLF by default on Windows. They're still commited with LF, don't worry.).

I don't think I understand what the issue is. All files should be formatted and committed with LF in all OSs. Does prettier format something that it shouldn't on Windows hence the large diffs? Setting this to auto can lead to mixed line endings which we can't permit. Using WSL is the safest solution IMO. BTW VS Code should be able to account for LF or CRLF on any OS.

Don't worry, everything still gets commited with LF.
The way git works on Windows by default is that it checks out everything with CRLF (even if it's stored in the repo with LF) and when you commit, it internally converts all CRLF back to LF.
The problem with prettify was that by doing its thing, it would result files containing just LF.
This then shows up in git as unstaged changes in all of the files.
The --end-of-line auto option should work the way that it looks whether CRLF or LF is used on the first line (this would be CRLF on Windows, LF elsewhere) and then uses this line ending for the rest of the file.
When I commit, git then saves this as LF.

Another option would be to configure git to "checkout LF, convert CRLF to LF, commit LF" (this can be done per repo), but I would strongly recommend we stick to the default behaviour, otherwise things get messy (i.e. when copying stuff between projects with different settings)...

The greater question with this PR I guess is how do people configure their Python environments on Windows? How do they reach binaries installed via pip? On Linux and MacOS the user install location for pip is not in the PATH either. There is just the assumption, potentially erroneously, that the user has done some minimal setup for their development environment. It might be worth doing a try-except if binary X is not found in default vscode settings try the "default" pip location first, which might be outside of PATH.

As I said, I think the ideal scenario is that the extension installs fortls itself using pip install --user fortls and then it is able to find and run it from the user location, whether the user did include it in PATH or not. If a user wants to tinker/adjust the installation themself, they can always do that, but the process should be as effortless as possible. And as the user site of pip can easilly be queried, there's no point in not looking in there.

Looking forward for your thoughts.

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