-
Notifications
You must be signed in to change notification settings - Fork 2k
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
VSCode integration results in an inconsistent environment #2299
Comments
This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in a week if no further activity occurs. Thank you for your contribution(s). |
This issue has been automatically closed due to it not having any activity since it was marked as stale. Thank you for your contribution(s). |
@selimb sorry I just saw this after the bot closed it. There is a lot here to take in. Great explanation! I think I understand the issue.
Without actually testing it I believe your conclusion is correct and you are correct the libs are not documented anywhere other than in the libs themselves. Re-opening. |
@selimb Did my response make any sense? I am not sure there is much we can do to resolve the issue any better than you have already done. To be honest it took me a little while to wrap my head around what you came up with as a workaround. Pretty slick solution! The Visual Studio Code integration was a user bolt-on enhancement and is limited to very basic Cmder init.bat since we cannot pass args to init.bat via The only thing I could suggest that might be a possibility as another workaround is to set these globally on Windows:
No custom Also don't use |
@daxgames Thanks for replying!
Oh, that's neat. Are those variables documented somewhere? I'll give that a try. I'm guessing that doesn't allow you to specify other options such as
Ah! Wasn't aware of that. Would it be worth a quick mention in the docs? Not necessarily a full workaround, but at least knowing that it's deliberately a simplistic solution might have (personally) saved me some time. |
You could also set |
I'll try to get back to you tomorrow. |
Another option might be to set the env variable in the settings.json:
|
@daxgames Using I guess what would fully solve the problem is if the arguments to
Aaah. Not 100% what I'm after then (unless I use |
We can fix the nix_tools and other variables override. |
Note that I'm OK with my workaround. If you feel it doesn't add much value to users, feel free to close the issue. |
@selimb I think it would be better to make it more useable. I will take care of it and let you know. |
@selimb Ceckout https://ci.appveyor.com/project/MartiUK/cmder/builds/33372802/artifacts
|
@daxgames Awesome! Thanks. I'll try it out today. |
@daxgames I grabbed the new build and reverted to the vanilla setup:
I set these variables globally:
And everything works as I would've liked/expected, except for the fact that the VSCode terminal is no longer "clink-enabled": whereas it should be: That's why I needed to have:
in my To be honest I have no idea how clink works, so perhaps there's another way of accomplishing the same thing. |
Probably need to remove the "CMDER_CONFIGURED" skipping to user config. |
Another possibility would be to move the skip below the clink enablement. This does that. |
Both options solve the problem (unsurprisingly). I'm wondering though: why should |
The original intent was so user config changes could be tested simply by re-running |
@daxgames Apologies for the delay. Just gave the new build a shot today. Using the same setup as in #2299 (comment), it all works great. For reference:
|
I just noticed something though: it seems that
|
@selimb I will need to look. |
@daxgames Bit confused re: the emails I've been getting XD Again, thanks for your time on this. |
@selimb the fix as coded fixes you but others. See: #2311 (comment) So I need to try and figure out how to fix both. |
With this settings.json:
Note: The Should get you what you want without causing #2311 (comment) |
@selimb Thinking of making one more change that will change the usage of Currently:
Future:
@selimb Question: Is I am asking you because:
|
Is
Because that's exactly what I found to be the absolute minimum code necessary to bootstrap a new vscode terminal when vscode was launched from cmder. |
@selimb yes you are correct. |
Then the "future" implementation of
will not result in the same environment (and possibly be broken?) as:
Hope this makes sense. |
Why is this:
even necessary? I believe the bug reported in #2311 (comment) isn't quite a "separate" bug -- I forgot to test aliases, apologies for that. What I mean is:
The aliases indeed (for some reason that I ignore, more on this below) don't persist through either task splits or vscode terminals, but the "profile" (which I assume is environment variables) definitely do persist through both task splits and vscode terminals. Reloading the aliases does not require rerunning the whole user config. It merely requires an |
@selimb you do not have to set I guess I need to understand your use cases and desired outcomes.
|
@daxgames Nice. I'll try this out thoroughly today. |
Sorry for taking so long. This made me realize that the user config gets called when splitting the window. I ended up going with something similar to what I had initially, although I no longer have to rely on the internals and can simply set Thanks for your patience on this. Noticed a few things that need fixing in the vscode integration wiki page:
|
@selimb so the pr is good to go? |
@selimb wiki fixed. |
You too.
That's why
So you are not using the new I still think this is a better way to work for the average Cmder VSCode user than what we had before. Definitely more flexible? It would be great if you could post your final setup so others could benefit in case the out of the box experience does not fit. |
@daxgames Actually I got mixed up with my setup. Turns out the |
That indeed works for PATH, but not for other variables.
Here goes. Let's start with a few definitions:
Given that I:
My requirements:
|
Indeed, it's definitely an improvement! I thought it violated requirement 2., but I guess it wouldn't if I used the env. vars instead of the cmdline options. What's currently stopping me, however, is requirement 3 for cmder child terminals. The reason is: EDIT: I don't think I ever understood why the |
I don't understand this statement.
That's easy to fix. Try this, edit
|
With the latest build, there's 2 ways to ensure root cmder and root vscode terminals use the same init.bat options:
The former violates "I want to define init.bat options exactly once", and the latter works, but env. vars are harder to keep in sync (unless it's a single one). What I mean by that is: I have a Now, let's say I use On the other hand, if I only had to set So, basically, would you be open to reviewing a PR that adds support for this
That would work indeed.
Ah. Personally: I don't spend a significant portion of my time tinkering with my user config, and really wouldn't mind having to create a new tab (instead of a split) in that specific (rare) case, especially if it's (in my opinion) harmful otherwise. |
PRs are always welcome and will be reviewed. I don't understand how CMDER_CONFIGURED=1 behavior could be harmful. I have never had an issue with it. Do you have an example? |
In the example workflow I posted, it would result prepend |
I am trying to understand so I tested your sample workflow by doing the following:
See screenshot below: The only difference is selected in the top right duplicated terminal and was not done by Cmder. I assume Conemu did this when duplicating the terminal vertically. |
Interesting. What does your user_profile look like? How could it not override pytest_addopts? |
I did not do that part. I simply did what was in number 3 of your workflow. If I define those settings, as below, in the
However if I do this all is good:
I just did a new commit and using it I get the below if I set |
@daxgames Sorry for taking so long to get back to this. Went on vacation. Right, if you're careful enough in I was asking whether For reference, here's my updated setup using https://ci.appveyor.com/project/MartiUK/cmder/builds/34261352/artifacts Global environment variables (that I set in a bootstrapping script):
:: wrapper around cmder's init.bat so we can use the same options from VSCode and ConEmu Tasks
@echo off
call "%CMDER_ROOT%\vendor\init.bat" /c "%DOTFILES_ROOT%\cmder" /nix_tools 0 /f || exit /b
:: Copied from cmder's vscode_init, but adjusted to call our init.bat
@echo off
IF [%1] == [] (
REM -- manually opened console (Ctrl + Shift + `) --
CALL "%~dp0init.bat"
) ELSE (
REM -- task --
CALL cmd %*
exit
)
:: set CMDER_CONFIGURED=2 to skip cmder+user config (except essential bootstrapping like aliases and clink)
:: if we've already been init'd
set CMDER_CONFIGURED=2
set "PYTEST_ADDOPTS=-v --tb=short"
set "PATH=%HOME%\.local\bin;%PATH%" vscode user {
"terminal.integrated.shell.windows": "C:\\Windows\\System32\\cmd.exe",
"terminal.integrated.shellArgs.windows": [
"/k",
"%DOTFILES_ROOT%\\cmder\\vscode_init.cmd"
]
} conemu task:
This satisfies all requirements defined in #2299 (comment). It's actually not much different than what I described in the original comment: the only difference is I no longer need this hack in my custom
@daxgames Finally, I'd be happy to close this issue (once #2339 gets merged). Note that, unfortunately, I'm still not using
|
1.3.16 Released |
First of all, a million thanks for this project. <3
Purpose of the issue
Version Information
Cmder: 1.3.14.982
ConEmu 191012
OsVer: 10.0.19559.x64
Background
My ConEmu task is:
and I launch cmder.exe with this shortcut:
where
DOTFILES_ROOT
is my local dotfiles Git repo.In the wiki, it's recommended to add the following to vscode's
settings.json
:At the time of this writing,
vscode_init.cmd
merely callsinit.bat
without any arguments.Problem 1
If I don't launch Visual Studio Code from cmder (e.g. Windows Explorer context menu, Start Menu, Desktop shortcut, etc.), the options to
init.bat
are not the same as in my ConEmu task. Additionally, this means my%DOTFILES_ROOT%\cmder
user config is not picked up.Workaround 1
I worked around this by adding a wrapper
init.bat
and duplicatingvscode_init.cmd
:modifying the ConEmu task to:
and the vscode
settings.json
to:This ensures parity between
init.bat
options in the ConEmu task and in the vscodeshellArgs
.This actually leads to a second (probably less important) problem:
Problem 2
If I launch vscode from Cmder (
code .
, or similar), then PATH is effectively "re-enhanced", resulting in a PATH like:where
[cmder init.bat]
are the modifications made to PATH byinit.bat
and my user configuration, and[global]
is the global (system?) value ofPATH
.I believe by removing the
/f
option toinit.bat
, cmder's PATH enhancements will not be duplicated, but my user configuration still will (unless I were to uselib_path
as well?).In most cases, I agree that duplicated values in PATH is merely unsightly. However, it can be a problem if modifications to PATH (let's call them
[session]
) are made after launching Cmder but before launching vscode. This results in something like:All of our projects at work require setting up PATH (and other envs) before launching vscode, i.e. the workflow is something like:
cd [project_dir]
scripts\edit.bat
(this prepends[session]
to PATH and launches vscode)scripts\test.bat
(this prepends[session]
to PATH and runs the tests)So why is it problematic? Simply because it means the environment I get in a vscode terminal isn't the same as the environment I run the tests in, and in fact isn't even the same as the environment that VSCode tasks (such as running the tests and debugging) run in. I haven't come across any problems with this yet, but it bothers me.
Workaround 2
My workaround for the above was to tweak my
init.bat
to useCMDER_CONFIGURED
(which isn't used anywhere AFAICT?):Conclusion
Re: problem 1: Am I missing something?
Problem 2 can probably be avoided by not using
/f
and usinglib_path enhance_path
in myuser_profile.cmd
? Would it be worth mentioning that somewhere (sincere apologies if it already is).The text was updated successfully, but these errors were encountered: