Merge Request: Better C++ language server and Visual Studio Code support.

Hallo Volker,

@andy has been doing some things too, not sure it there is any overlap.

However, is this one of those things that adds version issues? I installed bear using zypper (opensuse) from some compiler-tools repo.

Falls over immediately:

bear 2.3.13
usage: bear [-h] [--version] [--verbose] [--cdb <file>] [--use-cc <path>]
            [--use-c++ <path>] [--use-only] [--append] [--libear LIBEAR]
            ...

Of course now my patched wmake is broken, since there this now way to define to use bear or not.

I guess you probably wanted something like this:

if [ "$FOAM_WMAKE_FRONTEND" = bear ] \
&& [ -z "$(INTERCEPT_REPORT_COMMAND}" ] \
&& command -v bear >/dev/null
then
    ...
fi

Which would allow you to select/deselect its use.

@andy has been doing some things too, not sure it there is any overlap.

Before opening this Issue, I opened a PR. Andy saw that and took a look at it.

However, is this one of those things that adds version issues? I installed bear using zypper (opensuse) from some compiler-tools repo.

Falls over immediately:

fixed in new mypatch3.patch.

I don't think we need an option to disable it, because it should never fail. with the new patch.

Further questions:

are the compile commands useful for anything beyond building? Probably should dump under build/$WM_OPTIONS/ccls-cache
what happens with different builds? Eg, compile with int32/double and recompile with int64/mixed-precision? It looks like the cache is the same for both.
if we use wclean, does this get properly noticed by your build? Or do we need to remove/refresh the cache manually?

are the compile commands useful for anything beyond building? Probably should dump under build/$WM_OPTIONS/ccls-cache

Are you talking about the ccls-cache folder or the compile_commands.json file or something else?

what happens with different builds? Eg, compile with int32/double and recompile with int64/mixed-precision? It looks like the cache is the same for both.

We are writing into $FOAM_LIBBIN/../compile_commands.json which is e.g openfoam/platforms/linux64GccDPInt32Opt/compile_commands.json. So if you e.g. select clang instead of gcc, $FOAM_LIBBIN will change and so will the path of our compile_commands.json file.

if we use wclean, does this get properly noticed by your build? Or do we need to remove/refresh the cache manually?

wclean has no effect on compile_commands.json or the ccls-cache folder and this is fine. If you run wclean and then wmake again, bear will overwrite the corresponding entry in compile_commands.json.

without ccls or bear, it is possible to setup Go To Definition/Peek etc. for OpenFOAM afaik with few lines in settings.json and/or c_cpp*.json.

or is there any obvious disadvantage that I can't see (or the obvious advantage of the proposal)?

wouldn't be better if we proceed for a vscode extension, instead?

If you want Go To Definition to work reliably (!), then the IDE needs to know how your Program was compiled. Imagine if e.g your source is

#include "file.H"

and there are multiple files in different foldes named file.H. The only way this works is if the IDE knows the -I/path/to/folder arguments passed to the compiler. compilation_command.json is a file that includes this information.

We are using ccls instead of the official vscode C++ plugin from Mircosoft or ClangD because of this issue and this issue.

Because Openfoam uses a shitty build system called make instead of a proper one like Meson, we have to use hacky ways to generate this compilation_command.json file. We are using bear to generate this file.

Because Openfoam uses a shitty build system called make instead of a proper one like..

easy champion..

overall rhetoric of yours seems to be unnecessarily impulsive. we are open minded hardworking people and trying to do the best for the community which we feel part of. we only exchange our ideas here and discuss matters as kindly and transparent as possible.

the existence of the few lines I had mentioned works, to my experience, almost 95 percent - for functions, types etc virtually always works.

therefore I suggest seniors to proceed over a vscode extension rather than maintaining something inside OpenFOAM for some developers using vscode, possibly a small portion of the entire habitat.

yet it will be @mark's call. personally dont mind.

PS: vscode is not an IDE.

easy champion..

overall rhetoric of yours seems to be unnecessarily impulsive. we are open minded hardworking people and trying to do the best for the community which we feel part of. we only exchange our ideas here and discuss matters as kindly and transparent as possible.

Do you think wmake is good, or do you think that my language is inappropriate?

the existence of the few lines I had mentioned works, to my experience, almost 95 percent - for functions, types etc virtually always works.

Even if it would be 95 %, what about the other 5 %? The other 5 % are usually the tricky ones and it would take the user a lot of time to figure out where the called function is. Finding the called line number is a task that programmers have to do very often and that takes a lot of time. Repetitive, time intensive tasks should be automated if possible.

therefore I suggest seniors to proceed over a vscode extension rather than maintaining something inside OpenFOAM

Is anyone currently writing on a OpenFOAM vscode extension?
What should the OpenFOAM vscode extension do? You need to know the compilation commands for Go To Definition to work. This means that either the extension reads the compilation_commands.json file, in this case we still need to generate it or the extension parses the wmake configurations. Generating a compilation_commands.json file is easier and has the advantage of working with other IDE's.

for some developers using vscode, possibly a small portion of the entire habitat.

The goal is to make this portion as high as possible. This is why I want to installation to be as simple as possible and this is why we use bear by default (as long as bear is available). What do you think of adding bear as a dependency to the distribution packages and the "install from source" guide? We could also make other IDE's read the compilation_commands.json file. What IDE's / text editors are you using?

Language is inappropriate

made the issue confidential

changed the description

Why was this issue marked as confidential? It has no security implications.

it was to protect you - nevermind, possibly thought in another perspective - opened it again

made the issue visible to everyone

assigned to @mark

Hi @volker-weissmann - I understand @kuti's call in making it confidential (d.h. sehr direkte Formulierungen könnten schnell ausufern).

It would indeed be interesting to explore alternative build backends for OpenFOAM that are faster. Had a look at ninja a while ago and it seems to be gaining a fair bit of traction. I haven't used it beyond a toy project, so I don't have any performance numbers. One of the problems that we always face it that we have to be stable/compatible across a wide number of systems (cf, use of c++11 vs newer standards until widespread support exists). Replacing make with something else is a similar case.

Another point for your proposed change - how would you reformulate the transformation and make rules into ninja equivalents? Can we do this and preserve the macro preprocessing of Make/options?

I assume that you have tested your adjusted script?

Inline comments for "mypatch4.patch":

if [ -z "${INTERCEPT_REPORT_COMMAND}" ] ; then
    if ! bear --version > /dev/null ; then

What is wrong with command -v bear ?

        echo "WARNING: bear is not installed -> There will be no compile_commands.json output."

Why should people get a warning (on stdout!!!) if their system does not have "bear" (like my system yesterday morning) and they didn't happen to have exported INTERCEPT_REPORT_COMMAND? Assuming you can't rely on passing an additional command-line option to wmake, then exporting a variable to enable makes more sense.

    elif printf '%s\n%s\n' "bear 3.0.0" "$(bear --version)" | sort -V -C ; then

what is the point of the printf and sort for checking the version? I really don't see the purpose. Why does my bear --version produce stderr output, but yours produces stdout? This doesn't look very encouraging.

        bear version >= 3.0.0

This appears to be using >= as a bash arithmetic operation, but this just redirects the stdout to an = file, which we need to remove?

        mkdir -p $FOAM_LIBBIN
        bear --append --output $FOAM_LIBBIN/../compile_commands.json -- wmake $@

why call instead of exec?

        exit $?
    elif printf '%s\n%s\n' "bear 2.0.0" "$(bear --version)" | sort -V -C ; then
        bear version >= 2.0.0
        mkdir -p $FOAM_LIBBIN
        bear --append --output $FOAM_LIBBIN/../compile_commands.json wmake $@

I guess this supposed work for bear >= 2.0.0 ? But with bear 2.3.13 I still only have -o or --cdb as reasonable candidates.

        exit $?
    else
        echo "WARNING: bear version is below 2.0.0 -> There will be no compile_commands.json output."
    fi
fi

What is wrong with command -v bear ?

What do you mean?

This appears to be using >= as a bash arithmetic operation, but this just redirects the stdout to an = file, which we need to remove?

Hups, this should be a comment. Fixed in mypatch5.patch

why call instead of exec?

Why not?

I guess this supposed work for bear >= 2.0.0 ? But with bear 2.3.13 I still only have -o or --cdb as reasonable candidates.

Fixed in mypatch5.patch

what is the point of the printf and sort for checking the version? I really don't see the purpose.

How else would you check the version if not using printf and sort?

Why does my bear --version produce stderr output, but yours produces stdout? This doesn't look very encouraging.

Could not reproduce:

$ ./bear --version 2> /dev/null 
bear 2.3.13

Why should people get a warning (on stdout!!!) if their system does not have "bear" (like my system yesterday morning) and they didn't happen to have exported INTERCEPT_REPORT_COMMAND? Assuming you can't rely on passing an additional command-line option to wmake, then exporting a variable to enable makes more sense.

I could remove this warning if you want. It says that no compile_commands.json file will be produced, which is true. since mypath5.patch this warning gets send to stderr. If you want, I can remove the word "WARNING" but keep the rest of the message.

It would indeed be interesting to explore alternative build backends for OpenFOAM that are faster. If you think that's a good idea, I can prototype the replacement of wmake with meson+ninja and see how fast and good it is.

Hi @volker-weissmann

I will rework the idea as a subcommand hook with more robust testing. Not sure if I'll get it done today.

I will rework the idea as a subcommand hook with more robust testing. Not sure if I'll get it done today.

I have no idea what you are talking about. What is a subcommand hook?

changed the description

mentioned in commit 63fcce3df2f484158810fb4b5fc48340790c509a

Hooks look like in the above mentioned commit. The added code should be robust for most foreseeable circumstances. Not sure about the added value of the vscode generation bits, but at least now have something without an additional dependency. I'm not sure which systems have it installed as /bin/python3 instead of /usr/bin/python3 anyhow.

Hooks look like in the above mentioned commit. The added code should be robust for most foreseeable circumstances.

I can't imagine a situation where my code would wrongfully fail. The only thing I can think of is a faulty bear installation, but then you probably want to notice that your installation is faulty.

Not sure about the added value of the vscode generation bits,

It makes installation easier. This is worth a lot. Ask Apple.

I'm not sure which systems have it installed as /bin/python3 instead of /usr/bin/python3 anyhow.

I replaced it with /usr/bin/env python3 in the newest version

Hi @volker-weissmann

I notice that beyond the change in the python path, nothing else changed in your patch.

Have you tried with flake8? It flags a fair number of warnings.

I notice that beyond the change in the python path, nothing else changed in your patch.

Yes. Is there something else that should change?

Have you tried with flake8? It flags a fair number of warnings.

I just tried and I did some whitespace adjustments in mypath7.patch. The other flake warnings are irrelevant imho.

Please examine what I have pushed to develop.

Is there something else that should change?

Quite simply put, we cannot hijack wmake at the point where you have done so. If I use the following commands, I expect them to be processed directly, without extraneous warnings or errors from bear.

Examples,

wmake -help
wmake -build-info
wmake -show-compile-cxx
...

Imagine the following command:

CC="$(wmake -show-c)" CFLAGS="$(wmake -show-cflags)" ./configure --static

why should bear be involved?

Since you want a once-only entry hook, why not do so directly? If you see the settings emitted from my bin/tools/vscode-settings, you will notice this:

"C_Cpp.default.compileCommands":
"/path/OpenFOAM-com/etc/openfoam wmake -with-bear -s -j"

To see how (if) it works, please test it from the command-line in a clean (non-OpenFOAM) environment.

See this entry about the shell session. It might also be worthwhile to update your Wiki for this information as well.

changed the description

mentioned in commit a50047bb

changed the description

Hi @volker-weissmann

I have pushed changes to the develop branch that should address the integration, albeit not in the form that you proposed. The 'bin/tools/vscode-settings` script is a simple shell script, which ensures portability. Since it only emits content, but does not write anything it can safely be called from anywhere.

If you wish to continue with your python script however, it should *not be installed the bin directory since it automatically creates a .vscode directory in the current directory wherever the user happens to be (too dangerous).

Attached is modified version of the python script for your reference. vscode-settings.py

The 'bin/tools/vscode-settings` script is a simple shell script, which ensures portability.

Because there are so many machines where openfoam and vs code works but python3 does not. This totally justifies rewriting something in a language with a syntax as nice as shell./s. vscode itself has a dependency on python2. I wouldn't have rewrote it in shell, but if you are masochistic enough to do that its fine for me.

If you wish to continue with your python script however, it should *not be installed the bin directory since it automatically creates a .vscode directory in the current directory wherever the user happens to be (too dangerous).

Why exactly is creating a .vscode directory dangerous? This is literally what it is supposed to do and what the wiki says.

Quite simply put, we cannot hijack wmake at the point where you have done so. If I use the following commands, I expect them to be processed directly, without extraneous warnings or errors from bear.

If you don't like the warnings we could just remove the two echo "WARNING..." lines.

Examples, wmake -help wmake -build-info wmake -show-compile-cxx ... Imagine the following command: CC="
(wmake -show-c)" CFLAGS="
(wmake -show-cflags)" ./configure --static why should bear be involved?

My code has the advantage of being much shorter and simpler. To answer your question about "why should bear be involved?": To figure out whether this command compiles something or not. With your solution the wmake scripts figure that out, with my solution, bear figures that out.

Since you want a once-only entry hook, why not do so directly? If you see the settings emitted from my bin/tools/vscode-settings, you will notice this: "C_Cpp.default.compileCommands": "/path/OpenFOAM-com/etc/openfoam wmake -with-bear -s -j"

If we do this, it won't work if the user compiles openfoam using a command line outside of vscode.

Please examine what I have pushed to develop.

Ok: To start I would like to point out a wisdom I learned when learning Rust: If you have a great feature that is not enabled by default, you might as well not have it. Rust is successful because security checks are enabled by default and not optional like in C++. Setting up an IDE to use bear with Openfoam needs to be as simple as possible, if we want people to use it.

I looked at your bin/tools/vscode-settings script and I had an idea. What if ./Allwmake would, by default (!), write the vscode settings into $WM_PROJECT_DIR/IDEs/openfoam_default_recommendation.code-workspace ? That way the user would never be bothered with bin/tools/vscode-settings or bin/setupVScodeFOAM . Also, that way some people that do not know that there is such a thing as bear and ccls will see the openfoam_default_recommendation.code-workspace file and find out that this feature exists. Note: If we do this, we should add a comment to openfoam_default_recommendation.code-workspace that links to my wiki page.

I also looked at your wmake/scripts/AllwmakeParseArguments wmake/scripts/wmake-with-bear wmake/wmake . The obvious disadvantage is that its way more complex and way more lines of code than my solution, but if you want to write and maintain such a thing, this is ok for me. What I don't like is that the -with-bear is not enabled by default, if bear is installed. That way, the user needs to install bear and remember to pass this flag whenever he compiles something instead of just installing bear. If the user accidentally forgets this flag when compiling a part of openfoam, "Go to definition" will not work on some functions and it will be a nightmare to debug that (trust me, I tried). It is one more thing that the user needs to do.

Imagine if we would implement the two features that I suggested and we added bear as a dependency to the Linux packages: All the user would need to do is

Install openfoam
Install vscode
Install ccls
Open the openfoam_default_recommendation.code-workspace

to get accurate "Go to Definition"'s. As I said, the goal should be to make the installation as simple as possible and these 4 steps above are the simplest possible way I can think of.

Locally handled options (eg, -log) must preceeded trapped options such as -with-bear. This is non-intuitive, but not easily fixed.

OT: Stuff like this is why I don't like shell/bash and prefer python/xonsh.

Why exactly is creating a .vscode directory dangerous? This is literally what it is supposed to do and what the wiki says.

Imagine this one:

cd some/simulation/path
setupVScodeFOAM --help

You now have created a hidden directory somewhere. If this is supposed to be a one-time command, it can be moderately more difficult to locate.

To answer your question about "why should bear be involved?": To figure out whether this command compiles something or not. With your solution the wmake scripts figure that out, with my solution, bear > figures that out.

Reformulated. If I use wmake -show-compile-cxx than there is no reason to involve any other tools. If any of these tools break, this command breaks, even although it does not use that tool. If wmake -check-dir DIR fails because of your added code, the test fails. If wmake -show-cxx gets any additional stdout rubbish in it, scripts depending on it will fail.

If any of the changes cause failure on any machines this is much, much more inconvenient for many people. Have you tested your solution with an RPM and debian builds on fresh machines? (min CentOS7, ubuntu 18.04; max Fedora 33, leap 15.2, tumbleweed, ubuntu 20.10). Does it harmonize with the spack and EasyBuild installations?

Take a look at issues #1757 (closed) #1817 (closed) for an idea of unexpected failures, for things that were actually properly tested, but not in a particular combination.

Merge Request: Better C++ language server and Visual Studio Code support.

Child items

Activity

Admin message

Merge Request: Better C++ language server and Visual Studio Code support.

Child items

Linked items

Activity