implement env vars in settings (#2785) #9116

christapley · 2021-02-11T15:32:37Z

Summary of the Pull Request

use StringMap as a container for environment key/value pairs
recursively resolves ${env:NAME} vars in values in the StringMap or in the process' environment
implement some tests

PR Checklist

Closes (Feature Request) Terminal Environment setting (like in VScode) #2785
CLA signed. If not, go over here and sign the CLA
Tests added/passed
Documentation updated. If checked, please file a pull request on our docs repo and link it here: #250
Schema updated.
I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Wrote and ran the unit tests, tested manually to see environment variables were available.

christapley · 2021-02-11T15:35:58Z

src/cascadia/TerminalSettingsModel/Profile.cpp

+            std::wstring value(static_cast<size_t>(size), L'\0');
+            if (GetEnvironmentVariableW(key.c_str(), value.data(), size) > 0)
+            {
+                // Documentation (https://docs.microsoft.com/en-us/windows/win32/api/processenv/nf-processenv-getenvironmentvariablew)


Surely, I've missed something here?

github-actions · 2021-02-11T15:36:20Z

Misspellings found, please review:

ASDSDJ
HOMEDRIVE
HOMEPATH
MIDLE
Vauhiuhas

To accept these changes, run the following commands from this repository on this branch

pushd $(git rev-parse --show-toplevel)
perl -e '
my @expect_files=qw('".github/actions/spelling/expect/alphabet.txt
.github/actions/spelling/expect/expect.txt
.github/actions/spelling/expect/web.txt"');
@ARGV=@expect_files;
my @stale=qw('"aspnet boostorg dahall fde fea fmtlib isocpp mintty NVDA pinam QOL remoting unte vcrt what3words xamarin "');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
  if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
  next if /^(?:$re)(?:(?:\r|\n)*$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spelling/expect/16d00a68fe0e59342593e1e2a150376c05c5aa10.txt";
use File::Path qw(make_path);
make_path ".github/actions/spelling/expect";
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"ASDSDJ HOMEDRIVE HOMEPATH MIDLE Remoting Vauhiuhas "');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a) cmp lc($b)} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;'
popd

✏️ Contributor please read this

By default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.

⚠️ The command is written for posix shells. You can copy the contents of each perl command excluding the outer ' marks and dropping any '"/"' quotation mark pairs into a file and then run perl file.pl from the root of the repository to run the code. Alternatively, you can manually insert the items...

If the listed items are:

... misspelled, then please correct them instead of using the command.
... names, please add them to .github/actions/spelling/dictionary/names.txt.
... APIs, you can add them to a file in .github/actions/spelling/dictionary/.
... just things you're using, please add them to an appropriate file in .github/actions/spelling/expect/.
... tokens you only need in one place and shouldn't generally be used, you can add an item in an appropriate file in .github/actions/spelling/patterns/.

See the README.md in each directory for more information.

🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉

🗜️ If you see a bunch of garbage and it relates to a binary-ish string, please add a file path to the .github/actions/spelling/excludes.txt file instead of just accepting the garbage.

File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.

^ refers to the file's path from the root of the repository, so ^README\.md$ would exclude README.md (on whichever branch you're using).

github-actions · 2021-02-11T15:50:34Z

Misspellings found, please review:

HOMEPATH

To accept these changes, run the following commands from this repository on this branch

pushd $(git rev-parse --show-toplevel)
perl -e '
my @expect_files=qw('".github/actions/spelling/expect/alphabet.txt
.github/actions/spelling/expect/expect.txt
.github/actions/spelling/expect/web.txt"');
@ARGV=@expect_files;
my @stale=qw('"aspnet boostorg dahall fde fea fmtlib isocpp mintty NVDA pinam QOL remoting unte vcrt what3words xamarin "');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
  if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
  next if /^(?:$re)(?:(?:\r|\n)*$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spelling/expect/16d00a68fe0e59342593e1e2a150376c05c5aa10.txt";
use File::Path qw(make_path);
make_path ".github/actions/spelling/expect";
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"HOMEPATH Remoting "');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a) cmp lc($b)} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;'
popd

✏️ Contributor please read this

By default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.

⚠️ The command is written for posix shells. You can copy the contents of each perl command excluding the outer ' marks and dropping any '"/"' quotation mark pairs into a file and then run perl file.pl from the root of the repository to run the code. Alternatively, you can manually insert the items...

If the listed items are:

... misspelled, then please correct them instead of using the command.
... names, please add them to .github/actions/spelling/dictionary/names.txt.
... APIs, you can add them to a file in .github/actions/spelling/dictionary/.
... just things you're using, please add them to an appropriate file in .github/actions/spelling/expect/.
... tokens you only need in one place and shouldn't generally be used, you can add an item in an appropriate file in .github/actions/spelling/patterns/.

See the README.md in each directory for more information.

🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉

🗜️ If you see a bunch of garbage and it relates to a binary-ish string, please add a file path to the .github/actions/spelling/excludes.txt file instead of just accepting the garbage.

File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.

^ refers to the file's path from the root of the repository, so ^README\.md$ would exclude README.md (on whichever branch you're using).

christapley · 2021-02-13T14:17:15Z

Any ideas why the CI is failing with:

##[error]C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\MSBuild\Microsoft\DesktopBridge\Microsoft.DesktopBridge.Common.targets(175,5): Error MSB3202: The project file "..\..\host\dll\Host.DLL.vcxproj" was not found.

The solution is building locally fine and I can't find a reference to 'Host.DLL.vcxproj'?

skyline75489 · 2021-02-14T01:02:49Z

@christapley You may need to merge main

christapley · 2021-02-14T12:50:58Z

Ok, thanks @skyline75489. This will require a force push for the changes in this pr, so apologies in advance for that.

* use StringMap as a container * recursively resolve ${env:NAME} vars in values in the StringMap or in the process' environment * implement some tests

christapley · 2021-02-22T14:01:47Z

I don't think this is quite ready yet.

zadjii-msft

Hey, sorry I didn't reply faster. I was in the middle of reviewing the code, but didn't manage to finish. If you are planning on finishing this up, here's the feedback I had so far. It's mostly nits, but might be helpful anyways ☺️

zadjii-msft · 2021-02-19T15:52:02Z

src/cascadia/TerminalSettingsModel/Profile.h

@@ -21,6 +21,9 @@ Author(s):
 #include "../inc/cppwinrt_utils.h"
 #include "JsonUtils.h"
 #include <DefaultSettings.h>
+#include <winrt/impl/Windows.Foundation.Collections.2.h>


You shouldn't need to include this header directly here - we should be able to get winrt::Windows::Foundation::Collections::StringMap from the Windows.Foundation.Collections.h in pch.h

zadjii-msft · 2021-02-19T15:53:58Z

src/cascadia/TerminalSettingsModel/Profile.h

@@ -21,6 +21,9 @@ Author(s):
 #include "../inc/cppwinrt_utils.h"
 #include "JsonUtils.h"
 #include <DefaultSettings.h>
+#include <winrt/impl/Windows.Foundation.Collections.2.h>
+
+using namespace winrt::Windows::Foundation::Collections;


this is gonna sound like a nit, but it's usually best practice to avoid using namespace ... statements in headers. It results in everyone who includes that header automatically using that namespace, which can be probelmatic. I know it's annoying that you need to have the whole namespaced name in the property declaration, but ¯\_(ツ)_/¯

zadjii-msft · 2021-02-19T17:20:35Z

src/cascadia/TerminalApp/TerminalSettings.h

+#include <winrt/impl/Windows.Foundation.Collections.2.h>
+
+using namespace winrt::Windows::Foundation::Collections;


Same deal in this header

zadjii-msft · 2021-02-19T17:29:41Z

src/cascadia/TerminalSettingsModel/Profile.cpp

+namespace
+{


nit: I don't think we need an extra namespace here. Declaring the function as static will keep it internal to this file, and that should be good enough

zadjii-msft · 2021-02-19T17:31:30Z

src/cascadia/TerminalSettingsModel/Profile.cpp

+        for (auto it = view.First(); it.HasCurrent(); it.MoveNext())
+        {
+            const std::wstring key((*it).Key());
+            resolvedEnvMap.Insert(key, ResolveEnvironmentVariableValue(key, rawEnvMap, {}));


Since this last parameter is always {}, and we're never using it after the function is called, should we just remove it from the function signature and declare it internal to the function above?

It's used to detect self or circular references when resolving the environment variables so it needs to be updated and passed in as a parameter (not reference). I can default it in the function declaration so it is not needed to be specified here if that helps?

christapley · 2021-02-23T12:35:04Z

Thanks for the feedback, I will apply these to the next pr when the feature is more complete.

christapley commented Feb 11, 2021

View reviewed changes

christapley marked this pull request as ready for review February 11, 2021 15:41

zadjii-msft self-assigned this Feb 11, 2021

implement env vars in settings (microsoft#2785)

113d873

* use StringMap as a container * recursively resolve ${env:NAME} vars in values in the StringMap or in the process' environment * implement some tests

christapley force-pushed the feature/2785-profile-env-vars-with-string-map branch from 0439f5b to 113d873 Compare February 14, 2021 13:02

christapley closed this Feb 22, 2021

zadjii-msft reviewed Feb 22, 2021

View reviewed changes

zadjii-msft assigned DHowett Feb 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement env vars in settings (#2785) #9116

implement env vars in settings (#2785) #9116

christapley commented Feb 11, 2021 •

edited

Loading

christapley Feb 11, 2021

github-actions bot commented Feb 11, 2021

github-actions bot commented Feb 11, 2021

christapley commented Feb 13, 2021

skyline75489 commented Feb 14, 2021

christapley commented Feb 14, 2021

christapley commented Feb 22, 2021

zadjii-msft left a comment

zadjii-msft Feb 19, 2021

zadjii-msft Feb 19, 2021

zadjii-msft Feb 19, 2021

zadjii-msft Feb 19, 2021

zadjii-msft Feb 19, 2021

christapley Feb 23, 2021

christapley commented Feb 23, 2021

		#include <winrt/impl/Windows.Foundation.Collections.2.h>

		using namespace winrt::Windows::Foundation::Collections;

implement env vars in settings (#2785) #9116

implement env vars in settings (#2785) #9116

Conversation

christapley commented Feb 11, 2021 • edited Loading

Summary of the Pull Request

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

christapley Feb 11, 2021

Choose a reason for hiding this comment

github-actions bot commented Feb 11, 2021

Misspellings found, please review:

github-actions bot commented Feb 11, 2021

Misspellings found, please review:

christapley commented Feb 13, 2021

skyline75489 commented Feb 14, 2021

christapley commented Feb 14, 2021

christapley commented Feb 22, 2021

zadjii-msft left a comment

Choose a reason for hiding this comment

zadjii-msft Feb 19, 2021

Choose a reason for hiding this comment

zadjii-msft Feb 19, 2021

Choose a reason for hiding this comment

zadjii-msft Feb 19, 2021

Choose a reason for hiding this comment

zadjii-msft Feb 19, 2021

Choose a reason for hiding this comment

zadjii-msft Feb 19, 2021

Choose a reason for hiding this comment

christapley Feb 23, 2021

Choose a reason for hiding this comment

christapley commented Feb 23, 2021

christapley commented Feb 11, 2021 •

edited

Loading