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

Generation of VS Code debug configuration #302

Merged
merged 3 commits into from
Aug 9, 2023

Conversation

awillecke
Copy link
Contributor

@awillecke awillecke commented Jul 26, 2023

The process of adding Artery to VS Code for debugging can be quite error-prone (see #215). This PR adds the generation of a generic launch configuration for VS Code to the build process. It is only generated when building Artery in debug mode and will back up existing launch configurations.

One further consideration would be to build this config only on demand, so it does not clutter the working directory. What do you think?

@Enough7
Copy link
Contributor

Enough7 commented Jul 28, 2023

One further consideration would be to build this config only on demand, so it does not clutter the working directory. What do you think?

I would just commit examples/templates of these .vscode files, eg. suffixed with .example like launch.json.example.
So its possible to copy and paste them without the suffix.
Given that we add the folder .vscode to .gitignore Git's working tree does not get cluttered.
In another team we usually handled .env-Files that way.

In contrast to your proposal this way is more simple and stupid, which is beneficial regarding maintenance, robustness and effectiveness.
The CMake Build configuration is in my opinion large enough.
This makes it hard for newcomer developers to stay on top of things.
What if someone breaks this by accident.
Project developers will never notice this, because they got they .vscode files already.
New developers get confused and in the end they are frustrated because there is no .vscode config.

Comment on lines +33 to +35
// OMNet++ interface can either be Qtenv or Cmdenv
// "-uQtenv",
"-uCmdenv"
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi,
I was wondering, is not QTenv the default graphical environment and CMDenv is deprecated afaik.
I notice this on the website as well.
So should not it be the other way around, QTenv commented in and CMDenv commented out?

Copy link
Owner

Choose a reason for hiding this comment

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

I don't think that Cmdenv is deprecated but Tkenv. Of course, Cmdenv is not a graphical environment at all but it is very useful for automated/headless simulation runs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then I mixed that up. Thank you for the clarification.


# collect libraries for opp_run
set(opp_run_libraries "")
_get_opp_run_dependencies(${args_TARGET} opp_run_dependencies)
Copy link
Contributor

Choose a reason for hiding this comment

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

These are the OmnetC++ dependencies am I right?
Do they change frequently?

Copy link
Contributor Author

@awillecke awillecke Jul 31, 2023

Choose a reason for hiding this comment

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

These are the libraries of Artery (e.g. libartery_core.so, libtraci.so, ...). These might change when building Artery with other flags (e.g. libartery_envmod.so when WITH_ENVMOD is ON) or when you extend Artery with custom functionalities. Basically, they could change with every (re)generation of the build system.

Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if one includes all the libs all the time in the launch.json? Will this work?

Comment on lines 17 to 21
# backup existing launch.json
if(EXISTS ${args_FILE})
string(TIMESTAMP t)
file(COPY_FILE ${args_FILE} ${args_FILE}.${t})
endif()
Copy link
Contributor

Choose a reason for hiding this comment

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

If this does overwrite the file, then custom values get lost, am I right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If a launch.json is found, it is moved to a new file and suffixed with the current time. This way, you would still have access to custom values, however, you would need to transfer them to newly written launch configuration again manually.

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 this is the biggest drawback especially for people with additional custom Configuration in their launch.json.

@@ -0,0 +1,40 @@
function(generate_vscode)
Copy link
Contributor

Choose a reason for hiding this comment

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

A doc-comment explaining the purpose of this function would be beneficial and appreciated 😃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes!

@Enough7
Copy link
Contributor

Enough7 commented Jul 28, 2023

I think what this needs is some kind of indirection.
Maybe it is possible to write the dependencies and so on to an environment file and include it in the .vscode/launch.json 🤔

@riebl
Copy link
Owner

riebl commented Jul 30, 2023

I welcome any effort to make our lives easier. Just as @Enough7 I am sceptical if overwriting the launch.json is the way to go, though.
AFAIK, VSCode support for customization regarding launch.json is quite limited. I am not aware of any "include" mechanism, for example. An alternative approach might be an "Artery launcher executable" (replacing the plain opp_run) which could evaluate various environment variables and alike. Just brainstorming here.

@awillecke
Copy link
Contributor Author

Thanks for your feedback and comments.

I agree that overwriting the launch.json is not very nice (although the existing one is backed up) and might lead to confusion.

I think what this needs is some kind of indirection. Maybe it is possible to write the dependencies and so on to an environment file and include it in the .vscode/launch.json thinking

An environment file is indeed quite nice and very useful to have. I could imagine a file, containing the OMNETPP_ROOT, the NED folders and all necessary libraries. This file could be used to supplement the launch.json for the specific environment.

However, I encountered several issues after experimenting with it:

  1. Variables specified in the envFile cannot be used to change the programm path, pointing to opp_run_dbg. So is necessary to either depend on the OMNeT++ being in the PATH or setting the path to the OMNETPP_HOME as specified for the building process (which is similar to the handling of run_artery.sh).
  2. AFAIK, an individual key-value pair is required in args for each library to be loaded. It seems to be quite difficult to generate these from a single environment variable string using Code only. Furthermore, the number of libraries depends on the CMake build configuration, making it more complicated.

An alternative approach might be an "Artery launcher executable" (replacing the plain opp_run) which could evaluate various environment variables and alike. Just brainstorming here.

I think this approach should work to circumvent the current limitations of VS Code regarding configuration. However, I personally do not like having another runner, if it can be avoided. But if you are more in favor of this approach, I would also be fine going this route :)

One further approach could be to utilize CMakes JSON support to modify the launch.json that may exist. CMake could find the previously generated launch configuration and, if required, modify the build depended JSON keys (program, NED folders and libraries) accordingly.

@Enough7
Copy link
Contributor

Enough7 commented Jul 31, 2023

I am not aware of any "include" mechanism, for example.

Maybe opp_run_dbg has flag which allows to read the libs from a file or maybe there is a command which can be used to generate a tmp-launch.json?
Probably someone before us had this problem too because this is a quite common problem if you see it in a tech-agnostic way.
I guess, there might be a solution to it somewhere.

Edit: Brainstorming

Either deploys a new debug config or updates the previous one without
removing other configs.
@awillecke
Copy link
Contributor Author

Thanks for your thoughts on this.
Unfortunately, all libraries need to be specified individually using the -l flag.

  • In my opinion, an extension would be too extensive. However, it would open up new opportunities for further features.
  • The issue with substitution is that only CMake knows exactly how many libraries and, thus, key-value attributes there need to be given as arguments to gdb
  • AFAIK, only one launch.json is possible, which holds multiple configurations.

Based on your feedback, I adjusted the current approach to the following:

  1. When building as Debug, generate a default configuration to build/launch.json.default from the template
  2. Check if .vscode/launch.json exists
  3. If present, add/overwrite just the default configuration

src/artery/CMakeLists.txt Outdated Show resolved Hide resolved
cmake/AddVSCode.cmake Outdated Show resolved Hide resolved
@riebl riebl merged commit 4885a0a into riebl:master Aug 9, 2023
AndK17 pushed a commit to CAVISE/artery that referenced this pull request Jan 4, 2025
Generation of VS Code debug configuration
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.

3 participants