Make sure that SettingFile arg is parsed before we load the settings #7449

TravisEz13 · 2018-08-04T22:30:14Z

PR Summary

Make sure that SettingFile arg is parsed before we load the settings

Add ability to parse some setting before console host is created
Trigger the Parse before the console host is created
disable snapin loading from loading setting before this point
re-enable mac logging tests.

Also, fix the following Travis-ci issues:

make sure the Linux and macOS caches are separate to reduce the size
update the macOS image to the closest image as used by the official build system (equivalent to VSTS hosted mac builds)

PR Checklist

fixes PowerShell#7432

… build environment.

… old Ids

…rameter

iSazonov · 2018-08-05T11:07:02Z

src/Microsoft.PowerShell.ConsoleHost/host/msh/CommandLineParameterParser.cs

+            {
+                if (consoleSessionSetting.EnableConsoleSessionConfiguration == true)
+                {
+                    if (!string.IsNullOrEmpty(consoleSessionSetting.ConsoleSessionConfigurationName))


We could combine three if in one or use ? operator in just single return.

iSazonov · 2018-08-05T11:12:47Z

src/Microsoft.PowerShell.ConsoleHost/host/msh/CommandLineParameterParser.cs

+                if (MatchSwitch(switchKey, "settingsfile", "settings"))
+                {
+                    // parse setting file arg and don't write error as there is no host yet.
+                    if (!ParseSettingFileHelper(args, ++i, null))


Maybe use TryParseSettingFileHelper?

iSazonov · 2018-08-05T11:16:04Z

src/Microsoft.PowerShell.ConsoleHost/host/msh/CommandLineParameterParser.cs

+            {
+                if (switchKey.Length >= smallestUnambiguousMatch.Length)
+                {
+                    return true;


We could exclude if-s and use single return.

sorry, I don't follow what you are saying.

Well, looking at the code, I made a guess.

I meant

return (match.Trim().ToLowerInvariant().IndexOf(switchKey, StringComparison.Ordinal) == 0) && (switchKey.Length >= smallestUnambiguousMatch.Length)

iSazonov · 2018-08-05T11:20:29Z

src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHost.cs

            {
+                // We need to read the settings file before we create the console host
+                string[] tempArgs = new string[args.GetLength(0)];
+                args.CopyTo(tempArgs, 0);


Could you please clarify why we need clone args?

The existing code cloned the args. I don't want to break something by not cloning.

Reviewing the code. I don't think it has to be. I especially don't think it has to be twice. compromise, I'll remove the second clone, you file an issue to remove the first one.

Done #7457.

iSazonov · 2018-08-05T11:27:56Z

src/Microsoft.PowerShell.ConsoleHost/host/msh/CommandLineParameterParser.cs

        }

+#region Internal properties
        internal bool AbortStartup


Should #region be in position with the line?

TravisEz13 · 2018-08-05T20:06:13Z

@iSazonov I believe I have addressed all your comments one way or another.

iSazonov

Leave a comment

iSazonov · 2018-08-06T03:13:40Z

src/Microsoft.PowerShell.ConsoleHost/host/msh/CommandLineParameterParser.cs

+        /// The command line parameters to be processed.
+        /// </param>
+        /// <param name="argIndex">
+        /// The index in args to the argument following '-SettingFile'.


The parameter name is generic but comment says that it is used only for '-SettingFile' - should the parameter name reflects the fact?

iSazonov · 2018-08-06T03:20:14Z

src/Microsoft.PowerShell.ConsoleHost/host/msh/CommandLineParameterParser.cs

+            var consoleSessionSetting = Utils.GetPolicySetting<ConsoleSessionConfiguration>(Utils.CurrentUserThenSystemWideConfig);
+
+            return (consoleSessionSetting?.EnableConsoleSessionConfiguration == true && !string.IsNullOrEmpty(consoleSessionSetting?.ConsoleSessionConfigurationName)) ? 
+                    consoleSessionSetting.ConsoleSessionConfigurationName : string.Empty;


Maybe use more short name like settings to get more short code strings.

this is an existing variable name. Please file an issues

iSazonov · 2018-08-06T03:22:38Z

src/Microsoft.PowerShell.ConsoleHost/host/msh/CommandLineParameterParser.cs

+            bool noexitSeen = false;
+            for (int i = 0; i < args.Length; ++i)
+            {
+                var switchKeyResults = GetSwitchKey(args, ref i, null, ref noexitSeen);


It is not clear that we get from the method. I'd replace var with explicit type.
Can we use:

(string switchKey, bool shouldBreak) = GetSwitchKey(args, ref i, null, ref noexitSeen);

Named argument for null will be good.

iSazonov · 2018-08-06T03:30:06Z

src/Microsoft.PowerShell.ConsoleHost/host/msh/CommandLineParameterParser.cs

+
+                string switchKey = switchKeyResults.SwitchKey;
+
+                if (MatchSwitch(switchKey, "settingsfile", "settings"))


Named arguments will be good for constants. Below I see some such patterns.

iSazonov · 2018-08-06T04:14:20Z

src/Microsoft.PowerShell.ConsoleHost/host/msh/CommandLineParameterParser.cs

+            path = path.Replace(StringLiterals.AlternatePathSeparator,
+                                StringLiterals.DefaultPathSeparator);
+
+            return Path.GetFullPath(path);


The method is used once and then we call File.Exist() where Path.GetFullPath() is called again.
https://source.dot.net/#System.IO.FileSystem/System/IO/File.cs,119
We could simplify the TryParseSettingFileHelper() method - remove try-catch there (line 429), move the line 574 to TryParseSettingFileHelper() before File.Exist()

existing code, please file an issue, the method was just changed from an instance to static

Done #7469.

iSazonov · 2018-08-06T04:18:17Z

test/powershell/Host/Logging.Tests.ps1

+        $testPid = & $powershell -NoLogo -NoProfile -SettingsFile $configFile -Command '$PID'

-        Export-PSOsLog -After $after -Verbose | Set-Content -Path $contentFile
+        Export-PSOsLog -after $after -LogPid $testPid -Verbose | Set-Content -Path $contentFile


Please revert -after to -After.

iSazonov · 2018-08-06T04:20:19Z

src/Microsoft.PowerShell.ConsoleHost/host/msh/CommandLineParameterParser.cs

+            {
+                configFile = NormalizeFilePath(args[argIndex]);
+            }
+            catch (Exception ex)


I believe we could remove the try-catch. See my comment for NormalizeFilePath() method.

dantraMSFT

None of my comments are blocking but recommend you reconsider adding release logic for the new Dbg.Assert calls.

dantraMSFT · 2018-08-06T17:49:26Z

test/tools/Modules/PSSysLog/PSSysLog.psm1


+
 class PSLogItem
 {


I suggest updating the comment block at the top of the file to call out the change in format.
Other than that, this file LGTM

dantraMSFT · 2018-08-06T17:50:59Z

.travis.yml

+      env: TRAVIS_CI_CACHE_NAME=linux
    - os: osx
-      osx_image: xcode8.1
+      osx_image: xcode9.4


I assume this means we won't be running tests against the prior OS version, right?

I found no differences in the log format, but yes correct

dantraMSFT · 2018-08-06T17:58:15Z

src/Microsoft.PowerShell.ConsoleHost/host/msh/CommandLineParameterParser.cs

+        /// </param>
+        internal static void EarlyParse(string[] args)
+        {
+            Dbg.Assert(!_dirtyEarlyParse, "This instance has already been used. Create a new instance.");


The assert message is misleading. The method can only be called once since _dirtyEarlyParse is a static field.
Additionally, back this up with a release test or simply return if true. We don't run debug tests enough to catch this problem.

removed, I verified this can be run multiple times

dantraMSFT · 2018-08-06T17:59:52Z

src/Microsoft.PowerShell.ConsoleHost/host/msh/CommandLineParameterParser.cs

+        /// </param>
+        private static void EarlyParseHelper(string[] args)
+        {
+            Dbg.Assert(args != null, "Argument 'args' to ParseHelper should never be null");


Back this up with a check in release mode. At a minimum, silently ignore and return.

dantraMSFT · 2018-08-06T18:03:33Z

src/Microsoft.PowerShell.ConsoleHost/host/msh/CommandLineParameterParser.cs

+        /// The index in args to the argument to process.
+        /// </param>
+        /// <param name="parser">
+        /// Used to allow the helper to write errors to the console.  If not supplied, Files will not be parsed.


The comment about writing errors doesn't appear to apply.

Updated comment (I have not pushed at this point.)

dantraMSFT · 2018-08-06T19:05:08Z

src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHost.cs

+                string[] tempArgs = new string[args.GetLength(0)];
+                args.CopyTo(tempArgs, 0);
+
+                CommandLineParameterParser.EarlyParse(tempArgs);


I've created issue #7460 to move this call to UnmanagedPSEntry.Start prior to PSEtwLog.LogConsoleStartup().

dantraMSFT · 2018-08-06T19:25:20Z

test/powershell/Host/Logging.Tests.ps1

-        Export-PSOsLog -After $after -Verbose | Set-Content -Path $contentFile
-        $items = Get-PSOsLog -Path $contentFile -Id $logId -After $after -TotalCount 3 -Verbose
+        # Made tests more reliable
+        Start-Sleep -Milliseconds 500


I've created issue #7462 to address the Start-Sleep workaround as well as the possible false-positive in filtering test below.

dantraMSFT · 2018-08-06T19:32:53Z

src/Microsoft.PowerShell.ConsoleHost/host/msh/CommandLineParameterParser.cs

I'm pretty sure tuple field names are case sensitive.

TravisEz13 added 4 commits August 4, 2018 15:10

Add ability to parse -settingsfile before a console host is created

dbd1878

fixes PowerShell#7432

Parse the settings file before the console host is actually created

10943a5

fixes PowerShell#7432

do not set snap in logging information on Unix

3391e14

fixes PowerShell#7432

re-enable tests

1e673d3

TravisEz13 requested review from adityapatwardhan, anmenaga, dantraMSFT and daxian-dbw as code owners August 4, 2018 22:30

update to the latest mac image which most closely matches our release…

5979c49

… build environment.

TravisEz13 force-pushed the fix_settings_issue branch from aca4308 to 5979c49 Compare August 4, 2018 22:37

TravisEz13 added 8 commits August 4, 2018 16:52

change OsLogIds to a class, update ids, and add function to switch to…

4086976

… old Ids

add comments about fields

7d04ee2

change OsLogIds to a class, update ids, and add function to switch to…

e708a43

… old Ids

update export-psoslog so that it can also filter on PID

05eed04

move test to feature as it was not 100% reliable in my testing

1e9d32d

update tests to filter logs on pid

2226218

update test to use count as I found it more reliable

b2d36a8

add workaround for logs not showing up reliably

96e0b52

TravisEz13 force-pushed the fix_settings_issue branch 2 times, most recently from f98c185 to ebf9239 Compare August 5, 2018 03:01

Address CodeFactor issues

5c48dbc

TravisEz13 force-pushed the fix_settings_issue branch from ebf9239 to 5c48dbc Compare August 5, 2018 03:25

Update GetSwitchKey to return a NamedTuple instead of using an out pa…

777f5eb

…rameter

iSazonov reviewed Aug 5, 2018

View reviewed changes

TravisEz13 added 4 commits August 5, 2018 11:19

indent regions

e9fd826

simplify logic in MasterSwitch metod

4f1f4f0

Rename ParseSettingFileHelper method TryParseSettingFileHelper

8ca2220

simply GetConfigurationNameFromGroupPolicy using ?: and .? operators

ed941fa

TravisEz13 force-pushed the fix_settings_issue branch from f48196f to ed941fa Compare August 5, 2018 19:09

remove second clone of args

371ed6f

make sure linux and mac have seperate caches

92a8b9b

iSazonov reviewed Aug 6, 2018

View reviewed changes

dantraMSFT mentioned this pull request Aug 6, 2018

Move CommandLineParameterParser.EarlyParse to UnmanagedPSEntry.Start to ensure early settingsfile parsing #7460

Closed

dantraMSFT approved these changes Aug 6, 2018

View reviewed changes

dantraMSFT reviewed Aug 6, 2018

View reviewed changes

make switchKeyResults an explicit type

b8dcdfe

TravisEz13 force-pushed the fix_settings_issue branch from 2906b02 to b8dcdfe Compare August 6, 2018 19:57

TravisEz13 added 3 commits August 6, 2018 12:59

update comment about what parser is used for in GetSwitchKey

6799d24

Address PR Comments

d2a33ca

Address PR Comments

00c2a10

TravisEz13 force-pushed the fix_settings_issue branch from f1aec6f to 00c2a10 Compare August 6, 2018 23:08

Address PR comment

108ae7f

TravisEz13 merged commit 1523c21 into PowerShell:master Aug 7, 2018

TravisEz13 deleted the fix_settings_issue branch August 7, 2018 22:01

dantraMSFT mentioned this pull request Aug 9, 2018

Event logging fails on macOS 10.13 #7432

Closed

iSazonov mentioned this pull request Aug 16, 2018

Early parsing command line parameters #6374

Closed

iSazonov mentioned this pull request Oct 1, 2018

Remove clone args #7910

Merged

11 tasks

iSazonov mentioned this pull request Apr 25, 2019

Dash as a parameter crashes PWSH #9362

Closed


		string switchKey = switchKeyResults.SwitchKey;

		if (MatchSwitch(switchKey, "settingsfile", "settings"))

Make sure that SettingFile arg is parsed before we load the settings #7449

Make sure that SettingFile arg is parsed before we load the settings #7449

Uh oh!

Conversation

TravisEz13 commented Aug 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TravisEz13 commented Aug 5, 2018

Uh oh!

iSazonov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dantraMSFT left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

TravisEz13 commented Aug 4, 2018 •

edited

Loading