-
Notifications
You must be signed in to change notification settings - Fork 247
Add support for environment variable substitution in launch.json #1117
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
base: master
Are you sure you want to change the base?
Conversation
I am a bit confused about the envFile and what other debuggers do with it. It is generally meant for environment variables for the debugger or the target and not for replacements of other stuff in launch.json. The launch.json is just a VSCode thing. I the package.json, the documentation is inadequate for what envFile/environmet does and the precedence rules. See https://code.visualstudio.com/docs/python/environments#_environment-variables Using the same names (envFile/environment) for a totally different purpose can raise support issues and is confusing. Even to me because I cross boundaries between embedded and non-embedded environments. I haven't done a deep dive on what other do but reading from their docs the meaning does not seem to be the same. How does work with The whole reason people were asking for this is so that their build can populate the envFile. The way this is done, how can it work? Did you test it? What does cpptools do? |
Your lint is failing. Please run |
|
||
// Now lets replace anything in launch.json that needs replaced. | ||
const environment = config.environment || {}; // Ensure we can iterate over config.environment even if it's empty | ||
const replaceEnvVariables = (obj: any) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This algorithm is O(n*m) isnt int? We can do this in O(n+m). Imagine a 100 env. vars and a 100 properties in the launch config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been thinking about how to make this more efficient and wasn't sure how to get to O(n+m). Do you have an idea you can point me at?
My new strategy is to check each key in launch.json for ${env:*} noting them down as I go. Then in a separate loop check if they're in the envFile and replace them then. Should reduce the number of comparisons I'm doing a lot but I don't know if it actually changes the bigO.
So, who handles the actual environment variables? Maybe I missed it but are you merging the process.env with what we find in the launch config? I would start with (a copy of) process.env as the initial environment. I am assuming VSCode uses process.env as well. And have clear/documented precedence rules. |
What do you know of case sensitivity/preservation nature of env vars on Windows VSCode? What Windows does may not matter. What matters is what does VSCode do on Windows. We have to emulate whatever that behavior is. |
Wasn't able to find documentation that outright said whether or not VSCode was intended to obey case sensitivity for env variables but I did check and at least for the ${workspaceFolder} substitution it did so I went ahead and made sure that my search considered case before replacing. |
I'll get more and clearer docs made up. I'll also get something prepared for the wiki too so I can go into details without cluttering the package.json too much.
The documentation for other implementations of these are super lacking and it seems like they all handle it differently. For example: https://code.visualstudio.com/docs/python/environments#_environment-variables https://code.visualstudio.com/docs/cpp/launch-json-reference#_environment https://code.visualstudio.com/docs/reference/variables-reference
I played around with placing the whole routine in resolveDebugConfigurationWithSubstitutedVariables instead of resolveDebugConfiguration but that runs into supercession issues with the actual system variables. Essentially, some time between resolveDebugConfiguration and resolveDebugConfigurationWithSubstitutedVariables VSCode is doing it's own substitution which would replace any instance of ${env:*} that it doesn't find with an empty string making the searches and swaps done in my function always fail. What this guy microsoft/vscode-cpptools#5060 (comment) is saying fits what I'm seeing. In my implementation preLaunch tasks should have no effect on the envFile or environment since by the time they run they should have already been parsed and swapped out.
My use case is entirely in swapping out aspects of the launch configurations. Moreso using the envFile to populate aspects of my builds than the the build populating the envFile.
I tested a bunch on windows, I'm planning to do some runs on a linux box this weekend to make sure it acts like I expect it to.
As far as I can tell it doesn't interact with cpptools at all. What drove me to do this in the first place is that when I have cortex-debug selected as the type for a launch config it won't allow me to use the "environment" or "envFile" keywords at all in the launch configs. I think it warrants some digging though so I'll find some time to build a blinky project that uses the cpptools "environment" keyword and see if maybe I'm missing something and if it integrates well with the changes I'm asking for. |
That is not what I meant. Of course cpptools will not be involved if the debug type is I know it is easy to simply do what is needed for "my" use case. But, we have to look at the general picture. We are kind of doing this backwards. We are deriving specifications/requirements while developing. Even when I want to do something, I submit a proposal (using an issue here) and solicit feedback before writing any code.
Welcome to my world. Most of the time, I have to reverse engineer stuff like this. I would not use I don't want to end up with two sets of rules and two processors. We need to deal with ALL env vars. No half and half. And, thanks for taking the initiative. I knew it would not be as simple and if I could achieve the true goal of WHY people were asking for it. |
Relating to this issue: #717
I've added code which checks for optional properties "envFile" and "environment" in launch.json at the beginning of resolveDebugConfiguration. I tested on Windows 10 only as I don't have access to a Linux machine rn.
My primary reason for doing this was to make automating my build process easier. My build system organizes different builds under ./build/{software version}/{gcc output} with my makefile handling everything involved there. I've grown really fond of building/flashing/debugging all with one button click in vs code, but to do that I've had to keep the file paths in my launch.json in sync with the software version defined in my makefile, but that's rather inconvenient.
With this I can create a .env file in my .vscode folder and define any values that are unique to my setup and/or the current version in it, then .gitignore the .env file so that my coworkers can maintain their own unique environment. This shouldn't have any effect on any projects which don't use the "envFile" or "environment" properties as it only makes changes to the launch config if those values are present.
Formatting for the key/value pairs:
Example launch configuration:
Example .env file (with fake key obv):