Injecting Environment Variables to GDB and GDBSERVER processes #303

asimgunes · 2023-10-04T15:25:15Z

Hi,

I like to propose a new feature for cdt-gdb-adapter which enables injecting environment variables to gdb and gdbserver processes.

The new variables are located as environment and target.environment. The root environment injects environment variables to gdb process, where target.environment injects environment variables to gdbserver process.

Similar to cwd and target.cwd behaviour, gdbserver process also includes environment definition. (Which means gdbserver is injecting both environment and target.environment).

In this implementation, we also support construction of new values with using the old values. This is a common scenario for PATH environment variable in most cases. The following configuration will append a new path to the PATH variable:

PATH: '%PATH%;C:\some\new\path'

or

PATH: '$PATH:/some/new/path'

New value construction is not limited to the PATH variable, the logic could be used in any
variable and the following formats are supported:

%VAR_NAME% format:
TEST_VAR: "%TEST_VAR%;Some other text"

$VAR_NAME format:
TEST_VAR: "$TEST_VAR;Some other text"

${env.VAR_NAME} format:
TEST_VAR: "${env.TEST_VAR};Some other text"

I believe this could be a useful scenario in some cases where developers need to inject some environment variables seamlessly and improve the user experience.

To sum up, I would be happy if you can review this request, and happy to hear any feedback.

Kind regards.
Asim Gunes

asimgunes · 2023-10-04T15:31:19Z

Some additional update notes:

I observe that there is a problem in the Windows integration tests. As far as I see, new tests are working fine but one old test is failing in the report.
I implemented new tests for utility functions, which checks the inject logic and text building formats, however, I couldn't find the proper way to test if the environment variables is injected/passed in to the spawn function. (Since CdtDebugClient is already creating a new process while running the tests, libraries like sinon could not create stub functions to check the parameters.), I do my tests manually and locally, out of scope of this update. I am open for suggestions on this point.

jonahgraham · 2023-10-04T17:47:36Z

For windows tests are broken, see #299

For tests I think we can use getenv C function in a new test class to see what environment ends up in the inferior. See https://www.tutorialspoint.com/c_standard_library/c_function_getenv.htm
Eg you can call setenv on a few env vars and assign them to c variables, insert a breakpoint after those calls and ensure the C vars have expected values.

The other thing you can do is use cdt-gdb-tests/executeCommand to send arbitrary gdb commands to check the effect has worked as expected.

I'll do a review of the code itself soon.

jonahgraham

I tried adding ${env:PATH} to my launch.json and it was expanded as I expected with no special handling. Can you look again at that? I would prefer just to have ${env:XXXX} from VSCode and do no special handling.

jonahgraham · 2023-10-04T19:02:31Z

src/util.ts

+    data: any,
+    ...keyRegexs: RegExp[]
+): string {
+    const _resolveFromSourceHandler =


I expected _ to be unreferenced variables:

cdt-gdb-adapter/.eslintrc.js

Lines 16 to 22 in 33b2f35

'@typescript-eslint/no-unused-vars': [

'warn',

{

argsIgnorePattern: '^_',

varsIgnorePattern: '^_',

caughtErrorsIgnorePattern: '^_',

},

Is this a different style?

In this implementation, setting the variable to null is unsetting the environment variable. I add it to the tests, but I see that I missed in documentation.

My personal view is since _ is also a string I will vote for keep using null to unset the variables and update the documentation, how this sounds to you?

By the way, buildString is only used for merging the key/values into a given string (some kind of string interpolation). The variables handled in createEnvValues function.

I think we are talking cross purposes.

My comment was the minor issue of _resolveFromSourceHandler starting with a _ because of the quoted rules in eslint

This code is no longer exists in the update.

jonahgraham · 2023-10-04T19:19:07Z

src/integration-tests/util.spec.ts

+    const initialENV = {
+        VAR1: 'TEST1',
+        VAR2: 'TEST2',
+        var3: 'TEST3',


I see use of lowercase here, but afaict there is no test for either of these cases:

Linux/osx: case sensitive variables, i.e. var != VAR

Windows: case insensitive variables, i.e. var == VAR

In particular, PATH on Windows is typically written Path, but can be any capitalization of path.

So a valuesToInject of {Path: 'C:\\mytools\\bin;%Path%'} should update if the initialENV has PATH: or Path: properly.

The functinality removed and no longer exists in the update.

jonahgraham · 2023-10-04T19:21:34Z

src/integration-tests/util.spec.ts

+    });
+    it('should injection formats', () => {
+        const valuesToInject = {
+            VARFORMATTEST1: '$VAR1',


I think there should be a test where there is more than one environment variable in the valuesToInject, e.g: valuesToInject = { NEWTEST1: '$VAR1$VAR1', NEWTEST2: '$VAR1VAR2' }

The functinality removed and no longer exists in the update.

jonahgraham · 2023-10-04T19:42:20Z

I tried adding ${env:PATH} to my launch.json and it was expanded as I expected with no special handling.

I experimented with case sensitivity and multiple vars in a single expansion and it all worked as I would expect according to https://code.visualstudio.com/docs/editor/variables-reference#_environment-variables

asimgunes

Thanks for the feedback, let me check on them.

asimgunes · 2023-10-05T08:15:04Z

src/util.ts

+    data: any,
+    ...keyRegexs: RegExp[]
+): string {
+    const _resolveFromSourceHandler =


In this implementation, setting the variable to null is unsetting the environment variable. I add it to the tests, but I see that I missed in documentation.

My personal view is since _ is also a string I will vote for keep using null to unset the variables and update the documentation, how this sounds to you?

By the way, buildString is only used for merging the key/values into a given string (some kind of string interpolation). The variables handled in createEnvValues function.

jonahgraham · 2023-10-06T11:52:19Z

@asimgunes please let me know when you are ready for me to re-review this, with a comment on pressing the "re-request review" button.

Adding tests to cover environment variable controls

asimgunes · 2023-10-06T14:42:15Z

Hi @jonahgraham,

I think this pull request is ready to re-check right now.

To summarize, following changes performed:

Custom variable injection is removed so we leave the VSCode to handle ${env:VARNAME} format and we do not perform any string editing operation.
We cover the Windows environment variables handling scenario with the following rules:
2.1. We looked for case-insensitive environment variables key, if we found any, we update the value of this variable while keeping the existing casing.
2.2. If we do not found the variable than we insert the variable with the provided definition.
2.3. We ignore this check/control if operating system is not Windows.
Update the cdt-gdb-tests/executeCommand command and inject console output as well as the command result. I need to update this implementation due to executing command and getting results. (The previous implementation was neither getting the result nor getting the console output. I added both of them.)
Tests are updated to check environment and target.environment settings are working properly. We get the gdb process environment variables via gdb commands and inferior variables from the application itself.

Note: Both commands tried: show environment <VARNAME> and call (char *) getenv("<VARNAME>") to get the variables and both of them return the result in console output. Thats the reason for cdt-gdb-tests/executeCommand command update.

jonahgraham

This LGTM, some small issues with the tests to look at and it should be ready to go.

jonahgraham · 2023-10-06T15:06:12Z

src/integration-tests/launchWithEnvironment.spec.ts

+            fillDefaults(test, {
+                program: path.join(
+                    testProgramsDir,
+                    platform() === 'win32' ? 'vars_env.exe' : 'vars_env'


It doesn't matter much, but do you know why this code needs the .exe where the rest of the tests don't?

I fixed this issue and remoed the platform() condition.

jonahgraham · 2023-10-06T15:12:42Z

src/integration-tests/launchWithEnvironment.spec.ts

+    });
+
+    it('sets environment variables passed to the process', async function () {
+        if (hardwareBreakpoint) {


Why skip on hardwareBreakpoint?

Getting error in hardwareBreakpoint condition:
"No hardware breakpoint support in the target."
I skipped the test since the target of the test is not related to the hardwareBreakpoint condition.

The reason you were getting those errors is you were using debugAdapter.js when --test-remote was in use. The reason it fails is that hardware breakpoints can only be used in remote (target) debugging. See my commit where I fix this up consistent with how other tests force using the target adapter.

jonahgraham · 2023-10-06T15:13:56Z

src/integration-tests/launchWithEnvironment.spec.ts

+    });
+
+    it('check setting null will delete the variable', async function () {
+        if (platform() === 'win32' || hardwareBreakpoint) {


Why skipped on win32? Can we not delete on win32?

Win32 test environment is injecting another folder into the path so it never became null.

As I mentioned, the problem is related to the test environment, I added an arbitrary environment variable named "ENV_TEST_VAR" to control the deleting variable functionality and updated this code to also check it in Windows.

jonahgraham · 2023-10-06T15:14:13Z

src/integration-tests/launchWithEnvironment.spec.ts

+    });
+
+    it('checks setting environment variables with debugAdapter', async function () {
+        if (hardwareBreakpoint || (platform() === 'win32' && !isRemoteTest)) {


Why this skip condition?

I changed the skip condition for hardwareBreakpoint only. I realise that win32 is failing because of the recent test problem in Windows tests.

src/integration-tests/launchWithEnvironment.spec.ts

jonahgraham · 2023-10-06T15:33:13Z

src/integration-tests/utils.ts

+    if (variable.value === '0x0') {
+        return null;
+    }
+    // IMPORTANT: When value of the string is long, it returns dots `...` at the end,


I don't really like this :-(

Instead I recommend sending a command to gdb to disable this behaviour for the test with set print elements 0 - see https://sourceware.org/gdb/onlinedocs/gdb/Print-Settings.html#index-number-of-array-elements-to-print

Alternatively use printf "%s", name as the gdb command to get the value with the custom command.

The printf option has the advantage that you don't need to do regex to get the string out.

With print I got other complications in NULL checks (throwing error). I used set print addr off option and continue with that. Could you please check the current implementation?

asimgunes · 2023-10-09T13:56:13Z

Hi @jonahgraham,

I made some updates about your comments, Could you please re-check the updates when you are available?

Kind regards.

jonahgraham · 2023-10-10T17:01:20Z

src/integration-tests/launchWithEnvironment.spec.ts

+    });
+
+    it('sets environment variables passed to the process', async function () {
+        if (hardwareBreakpoint) {


The reason you were getting those errors is you were using debugAdapter.js when --test-remote was in use. The reason it fails is that hardware breakpoints can only be used in remote (target) debugging. See my commit where I fix this up consistent with how other tests force using the target adapter.

src/integration-tests/test-programs/vars_env.c

src/integration-tests/launchWithEnvironment.spec.ts

jonahgraham · 2023-10-10T18:02:49Z

src/integration-tests/launchWithEnvironment.spec.ts

+            breakpoints: [
+                {
+                    column: 1,
+                    line: 22,


Lets remove hardcoded line numbers, at least from new tests. Some old tests still use them. See latest commit.

jonahgraham · 2023-10-10T18:30:02Z

@asimgunes I made some changes to the code in line with my review comments. Please have a review of that and let me know if it looks good to you

jonahgraham · 2023-10-11T00:25:53Z

I have now fixed the Windows issue, rebasing this on main will pull in the fix for #299

asimgunes · 2023-10-11T13:00:43Z

Hi @jonahgraham,

Thanks for the updates, they all seem fine and I like the idea of lineTags. I also rebased the current request and all tests seem to be passed.

From my point of view, update seem fine.

jonahgraham · 2023-10-11T16:53:47Z

@asimgunes this is now merged - are there other changes incoming, or should I do a publish to npm and version bump?

Will you provide the corresponding change to cdt-gdb-vscode?

asimgunes · 2023-10-11T19:35:24Z

Hi @jonahgraham,

Thanks for the merge, there is no upcoming change from my side, and sure I can provide an update about related changes to the cdt-gdb-vscode, once you publish the recent cdt-gdb-adapter to npm.

jonahgraham · 2023-10-11T21:55:30Z

@asimgunes - I have published new version - https://www.npmjs.com/package/cdt-gdb-adapter/v/0.0.29 - can you consume that update when you make the cdt-gdb-vscode changes?

jonahgraham · 2023-10-12T21:46:40Z

can you consume that update when you make the cdt-gdb-vscode changes?

I am making the change to cdt-gdb-vscode now to pull in the fix in #306

Adds support for these features in the adapter: - setting cwd for gdb - eclipse-cdt-cloud/cdt-gdb-adapter#306 - better handling of complex variables: - eclipse-cdt-cloud/cdt-gdb-adapter#304 - environment variables - eclipse-cdt-cloud/cdt-gdb-adapter#303 And the new launch.json settings needed to enable the above. Along with a variety of bug fixes in the adapter.

asimgunes added 2 commits October 4, 2023 14:02

Adding support for injecting new environment variables

c1dc7d9

Util tests

4f398b0

jonahgraham requested changes Oct 4, 2023

View reviewed changes

asimgunes commented Oct 5, 2023

View reviewed changes

asimgunes added 5 commits October 5, 2023 16:20

Tests for injection of environment variables

ce05021

Windows specific logic update for environment variables

8d2a442

Test update

e8858b9

Test update

c8168d8

test update

1829c75

Update Tests (#3)

9c7e645

Adding tests to cover environment variable controls

asimgunes requested a review from jonahgraham October 6, 2023 14:42

jonahgraham requested changes Oct 6, 2023

View reviewed changes

Test Updates

28fb943

asimgunes requested a review from jonahgraham October 9, 2023 13:55

jonahgraham added 2 commits October 10, 2023 12:35

Run yarn format

a1bd105

Fix launching when --test-remote is in use

00d563f

jonahgraham requested changes Oct 10, 2023

View reviewed changes

jonahgraham added 3 commits October 10, 2023 13:40

Update src/integration-tests/test-programs/vars_env.c

76db4fa

Update src/integration-tests/launchWithEnvironment.spec.ts

6734daf

Remove hardcoded line number in test

59314e7

jonahgraham reviewed Oct 10, 2023

View reviewed changes

Merge branch 'eclipse-cdt-cloud:main' into proc-environment

45fcc73

asimgunes requested a review from jonahgraham October 11, 2023 13:02

jonahgraham approved these changes Oct 11, 2023

View reviewed changes

jonahgraham merged commit ccfbd07 into eclipse-cdt-cloud:main Oct 11, 2023
3 checks passed

jonahgraham mentioned this pull request Oct 12, 2023

Update to adapter 0.0.30 (cwd, environment and complex vars) eclipse-cdt-cloud/cdt-gdb-vscode#100

Merged

	'@typescript-eslint/no-unused-vars': [
	'warn',
	{
	argsIgnorePattern: '^_',
	varsIgnorePattern: '^_',
	caughtErrorsIgnorePattern: '^_',
	},

Injecting Environment Variables to GDB and GDBSERVER processes #303

Injecting Environment Variables to GDB and GDBSERVER processes #303

Conversation

asimgunes commented Oct 4, 2023

asimgunes commented Oct 4, 2023

jonahgraham commented Oct 4, 2023

jonahgraham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahgraham commented Oct 4, 2023

asimgunes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahgraham commented Oct 6, 2023

asimgunes commented Oct 6, 2023

jonahgraham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asimgunes commented Oct 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahgraham commented Oct 10, 2023

jonahgraham commented Oct 11, 2023

asimgunes commented Oct 11, 2023

jonahgraham commented Oct 11, 2023

asimgunes commented Oct 11, 2023 • edited Loading

jonahgraham commented Oct 11, 2023

jonahgraham commented Oct 12, 2023

asimgunes commented Oct 11, 2023 •

edited

Loading