Convert compatibility module to binary module, fix compatibility with Azure environments #1212

rjmholt · 2019-04-08T20:39:17Z

PR Summary

Resolves #1149.

The original compatibility check module was hastily implemented and was quite slow. It also needed CIM/WMI to be available. This meant it didn't work nicely with things like Azure Functions.

I rewrote it as a binary to be faster and to have a simpler, more composable, more semantically definite .NET API.

There are a bunch of breaking changes with the compatibility types here (which are technically exposed by PSSA). There are also huge breaking changes in the PSCompatibilityAnalyzer module (I got rid of most of the commands), but that has never been released and is still below version 1.0, so that seems like less of a concern.

Basically there are no changes that break the PowerShell API of PSScriptAnalyzer unless people have gone hunting for types that are loaded (but aren't used directly by the module).

Also renames the PSCompatibilityAnalyzer module to PSCompatibilityCollector since that's more accurate and less confusing.

List of changes

Renamed PSCompatibilityAnalyzer to PSCompatibilityCollector, since that better describes what the module does. The module is now version 0.2.0.
Got rid of almost all the psm1 module implementation in favour of a binary module. This gets rid of all the command originally in that module in favour of a minimal set of cmdlets with a common noun prefix.
Added explicit JSON configuration to the serialiser. Extra fields in JSON are now explicitly ignored (this is the default, so the behaviour has not changed), and default values are now explicitly omitted from JSON and restored in C# when absent (again, this should not change any behaviours)
Added a JSON schema version, with a default version of 1.0. The current generated schema version is 1.1 (since there are additions but not breaking changes, breaking changes should start at 2.0)
Flattened the Query and Data namespaces. The Query namespace was already mostly like this. It made sense to me to flatten it all from a complexity standpoint.
Change the ConstituentProfiles field to be a ReadOnlySet<string> instead of a HashSet<string>
Change type dictionaries to be case-sensitive for collection and overriding (like PowerShell) for the query API (this is already done in Prevent .NET members with names differing only by case from crashing the compatibility profiler #1195, but I need it on my machine). When Prevent .NET members with names differing only by case from crashing the compatibility profiler #1195 is merged, I'll rebase this on top.
Move things out of the Microsoft.PowerShell.CrossCompatibility.Utility namespace and into the base namespace, Collection or Retrieval depending on what the class does, since I think Utility should be reserved for internal helpers
Omit the pre-existing profiles from the output PSCompatibilityCollector module, since they aren't needed for collection
Add profiles for Azure Functions and Azure Automation

PR Checklist

PR has a meaningful title
- Use the present tense and imperative mood when describing your changes
Summarized changes
Change is not breaking
Make sure all .cs, .ps1 and .psm1 files have the correct copyright header
Make sure you've added a new test if existing tests do not effectively test the code changed and/or updated documentation
This PR is ready to merge and is not Work in Progress.
- If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready.

bergmeister · 2019-04-15T20:46:48Z

@rjmholt I'm overall OK with the changes as they are mainly just in your module. But I think @JamesWTruher would be the better person to review the logic, etc. as he has more experience/knowledge in this area.

JamesWTruher

here's the first of my comments

JamesWTruher · 2019-04-24T17:18:31Z

...ollector/Microsoft.PowerShell.CrossCompatibility/Collection/CompatibilityProfileCollector.cs

+            /// Add path prefixes of modules to exclude.
+            /// </summary>
+            /// <param name="modulePrefixes">Path prefixes of modules to exclude.</param>
+            public Builder ExcludedModulePathPrefixes(IReadOnlyCollection<string> modulePrefixes)


is there a reason why some of these aren't a void return? I see that you don't use the return value where you call it.

Just a convention of the builder pattern I've always used, but looking at the wikipedia page it seems to be less of a thing in C#. I'll change it over to just use simple properties.

JamesWTruher · 2019-04-24T17:43:28Z

...ollector/Microsoft.PowerShell.CrossCompatibility/Collection/CompatibilityProfileCollector.cs

+        /// <returns>An object containing common feature compatibility information.</returns>
+        public CommonPowerShellData GetCommonPowerShellData()
+        {
+            CmdletInfo gcmInfo = _pwsh.AddCommand(PowerShellDataCollector.GcmInfo)


we really need a better way to do this (get a command)

It's possible without the info object (since some commands might be loaded dynamically or are defined in script), but @daxian-dbw told me that providing the info object makes it faster.

JamesWTruher · 2019-04-24T19:08:00Z

...ollector/Microsoft.PowerShell.CrossCompatibility/Collection/CompatibilityProfileCollector.cs

+                    _pwsh.Dispose();
+                    _platformInfoCollector.Dispose();
+                    _pwshDataCollector.Dispose();
+                }


does dispose order matter here?

I never quite know about dispose order. Theoretically, since they are all co-dependent, the outer one should dispose the inner ones, but I tend to dispose from inside -> out. Perhaps it should be the other way round?

JamesWTruher · 2019-04-24T19:12:34Z

...Collector/Microsoft.PowerShell.CrossCompatibility/Collection/PlatformInformationCollector.cs

+                    osData.DistributionId = lsbInfo["DistributionId"];
+                    osData.DistributionVersion = lsbInfo["DistributionVersion"];
+                    osData.DistributionPrettyName = lsbInfo["DistributionPrettyName"];
+                    break;


Looking on macOS now, we might want an extra field for the retail version (i.e. the macOS version rather than the darwin version), but I'll add that later I think. For now, a macOS platform is identified concretely by macOS and the darwin kernel version I think.

...Collector/Microsoft.PowerShell.CrossCompatibility/Collection/PlatformInformationCollector.cs

JamesWTruher · 2019-04-24T20:19:10Z

...ilityCollector/Microsoft.PowerShell.CrossCompatibility/Collection/PowerShellDataCollector.cs

+
+            try
+            {
+                functionData.ParameterSets = GetParameterSets(function.ParameterSets);


JamesWTruher · 2019-04-24T20:19:56Z

...ilityCollector/Microsoft.PowerShell.CrossCompatibility/Collection/PowerShellDataCollector.cs

+        /// </summary>
+        /// <param name="variables">Variables to collect information on.</param>
+        /// <returns>An array of the names of the variables.</returns>
+        public string[] GetVariablesData(IEnumerable<PSVariable> variables)


GetVariablesName?

JamesWTruher · 2019-04-24T20:22:03Z

...ilityCollector/Microsoft.PowerShell.CrossCompatibility/Collection/PowerShellDataCollector.cs

+            for (int i = 0; i < outputTypeData.Length; i++)
+            {
+                outputTypeData[i] = outputType[i].Type != null
+                    ? TypeNaming.GetFullTypeName(outputType[i].Type)


i'm assuming this is where it can throw

JamesWTruher · 2019-04-24T20:25:38Z

...ilityCollector/Microsoft.PowerShell.CrossCompatibility/Collection/PowerShellDataCollector.cs

+        private static ReadOnlySet<string> GetPowerShellCommonParameterNames()
+        {
+            var set = new List<string>();
+            foreach (PropertyInfo property in typeof(CommonParameters).GetProperties())


GetProperties(Public,Instance,DeclaredOnly)?

JamesWTruher · 2019-04-24T21:41:17Z

...mpatibilityCollector/Microsoft.PowerShell.CrossCompatibility/Collection/TypeDataCollector.cs

@@ -106,7 +187,7 @@ public static class TypeDataConversion

            Type[] types = asm.GetTypes();
            JsonDictionary<string, JsonDictionary<string, TypeData>> namespacedTypes = null;
-            if (types.Any())
+            if (types.Length > 0)


in line 193, why not use types from line 188?

Good catch!

rjmholt · 2019-04-25T01:44:32Z

...ollector/Microsoft.PowerShell.CrossCompatibility/Collection/CompatibilityProfileCollector.cs

+            /// Add path prefixes of modules to exclude.
+            /// </summary>
+            /// <param name="modulePrefixes">Path prefixes of modules to exclude.</param>
+            public Builder ExcludedModulePathPrefixes(IReadOnlyCollection<string> modulePrefixes)


Just a convention of the builder pattern I've always used, but looking at the wikipedia page it seems to be less of a thing in C#. I'll change it over to just use simple properties.

rjmholt · 2019-04-25T01:46:48Z

...ollector/Microsoft.PowerShell.CrossCompatibility/Collection/CompatibilityProfileCollector.cs

+        /// <returns>An object containing common feature compatibility information.</returns>
+        public CommonPowerShellData GetCommonPowerShellData()
+        {
+            CmdletInfo gcmInfo = _pwsh.AddCommand(PowerShellDataCollector.GcmInfo)


It's possible without the info object (since some commands might be loaded dynamically or are defined in script), but @daxian-dbw told me that providing the info object makes it faster.

rjmholt · 2019-04-25T01:48:02Z

...ollector/Microsoft.PowerShell.CrossCompatibility/Collection/CompatibilityProfileCollector.cs

+                    _pwsh.Dispose();
+                    _platformInfoCollector.Dispose();
+                    _pwshDataCollector.Dispose();
+                }


I never quite know about dispose order. Theoretically, since they are all co-dependent, the outer one should dispose the inner ones, but I tend to dispose from inside -> out. Perhaps it should be the other way round?

rjmholt · 2019-04-25T01:50:34Z

...Collector/Microsoft.PowerShell.CrossCompatibility/Collection/PlatformInformationCollector.cs

+                    osData.DistributionId = lsbInfo["DistributionId"];
+                    osData.DistributionVersion = lsbInfo["DistributionVersion"];
+                    osData.DistributionPrettyName = lsbInfo["DistributionPrettyName"];
+                    break;


Looking on macOS now, we might want an extra field for the retail version (i.e. the macOS version rather than the darwin version), but I'll add that later I think. For now, a macOS platform is identified concretely by macOS and the darwin kernel version I think.

rjmholt · 2019-04-25T02:12:26Z

...ilityCollector/Microsoft.PowerShell.CrossCompatibility/Collection/PowerShellDataCollector.cs

+
+        private const string THIS_MODULE_NAME = "PSCompatibilityCollector";
+
+        private static readonly Regex s_typeDataRegex = new Regex("Error in TypeData \"([A-Za-z.]+)\"", RegexOptions.Compiled);


Good catch. There's a kind of error PowerShell emits when two modules have conflicting TypeData and the only way to identify it is by parsing the error message.

rjmholt · 2019-04-25T02:29:57Z

...ilityCollector/Microsoft.PowerShell.CrossCompatibility/Collection/PowerShellDataCollector.cs

+            // Get default variables and core aliases out of a fresh runspace
+            using (SMA.PowerShell freshPwsh = SMA.PowerShell.Create(RunspaceMode.NewRunspace))
+            {
+                Collection<PSVariable> defaultVariables = freshPwsh.AddCommand("Get-ChildItem")


Not a bad idea

rjmholt · 2019-04-25T02:57:25Z

...ilityCollector/Microsoft.PowerShell.CrossCompatibility/Collection/PowerShellDataCollector.cs

+
+            try
+            {
+                functionData.OutputType = GetOutputType(function.OutputType);


Actually, I think this occurs at the function.OutputType point, when we fire the getter, because it has to find the type

rjmholt · 2019-04-25T03:00:51Z

...ilityCollector/Microsoft.PowerShell.CrossCompatibility/Collection/PowerShellDataCollector.cs

+
+            try
+            {
+                functionData.OutputType = GetOutputType(function.OutputType);


I'm going to leave this one for now. The real question in my mind was how much data do we want to record about the function that throws when we try to load information about it. But for now I think just the skeleton is ok.

rjmholt · 2019-04-25T03:01:53Z

...mpatibilityCollector/Microsoft.PowerShell.CrossCompatibility/Collection/TypeDataCollector.cs

@@ -106,7 +187,7 @@ public static class TypeDataConversion

            Type[] types = asm.GetTypes();
            JsonDictionary<string, JsonDictionary<string, TypeData>> namespacedTypes = null;
-            if (types.Any())
+            if (types.Length > 0)


Good catch!

rjmholt · 2019-04-25T03:03:35Z

...patibilityCollector/Microsoft.PowerShell.CrossCompatibility/Data/CompatibilityProfileData.cs

+        /// <summary>
+        /// The verison of the profile schema this profile object uses.
+        /// </summary>
+        [DataMember]


I tried a few ways to set the automatic value of this to 1.0, but I couldn't work out how; I can't specify a version in the attribute.

JamesWTruher · 2019-04-25T15:58:11Z

...mpatibilityCollector/Microsoft.PowerShell.CrossCompatibility/Commands/ConvertJsonCommands.cs

+        /// If set, do not include whitespace like indentation or newlines in the output JSON.
+        /// </summary>
+        [Parameter]
+        [Alias("Compress")]


do you think this alias will suggest that it will create an actual compressed (.zip) file?

I actually put this in because ConvertTo-Json calls this Compress, but left it an alias because I think it's a bad name for it

Happy to go either way on this btw

JamesWTruher · 2019-04-25T16:05:40Z

...mpatibilityCollector/Microsoft.PowerShell.CrossCompatibility/Commands/ConvertJsonCommands.cs

+                            return;
+
+                        default:
+                            throw new ArgumentException($"Unsupported type for {nameof(JsonSource)} parameter. Should be a string, FileInfo or TextReader object.");


I think this should probably be a WriteError because it takes a collection of objects so one bad one should not cause the pipeline to abort. Also, if it should be terminating, then you probably want ThrowTerminatingError

Ah ok! Is it possible to throw multiple terminating errors?

JamesWTruher · 2019-04-25T16:07:50Z

...or/Microsoft.PowerShell.CrossCompatibility/Commands/GetPSCompatibilityPlatformDataCommand.cs

+        [Parameter]
+        public DotnetData DotNet { get; set; }
+
+        protected override void EndProcessing()


do you see a scenario where this would be called iteratively? If so, it should probably be in ProcessRecord

I thought about that, but because there are no pipeline parameters and it's probably used for collecting a profile on the local machine, EndProcessing seemed like the right call. This is effectively a function rather than a filter

JamesWTruher · 2019-04-25T16:13:58Z

...llector/Microsoft.PowerShell.CrossCompatibility/Commands/NewPSCompatibilityProfileCommand.cs

+            }
+
+            // If PassThru is set, just pass the object back and we're done
+            if (PassThru)


PassThru is usually used more like /bin/tee where the data is returned and the targets are created.

Yeah I was thinking about that. Maybe I should change this parameter name? Nothing else common really captures the behaviour though

yah, probably, but later before we release and have time to think of a better parameter name

JamesWTruher · 2019-04-25T16:15:44Z

PSCompatibilityCollector/Microsoft.PowerShell.CrossCompatibility/Common/ReadOnlySet.cs

+using System.Collections;
+using System.Collections.Generic;
+
+namespace Microsoft.PowerShell.CrossCompatibility


I wonder if this has enough utility to be part of PowerShell (or .NET)

It really should be somewhere, I've wanted it a few times

JamesWTruher · 2019-04-25T16:19:38Z

...mpatibilityCollector/Microsoft.PowerShell.CrossCompatibility/Utility/PowerShellExtensions.cs

+
+namespace Microsoft.PowerShell.CrossCompatibility.Utility
+{
+    internal static class PowerShellExtensions


I really think this should be in PowerShell (it doesn't much help with down level), but it's a common pattern which we could easily improve for folks.

Agreed. I'm not really sure how you're supposed to reuse a PowerShell instance without this. As in, I don't really understand the default scenario.

JamesWTruher · 2019-04-25T16:21:32Z

...crosoft.PowerShell.CrossCompatibility/Commands/AssertPSCompatibilityProfileIsValidCommand.cs

+    /// <summary>
+    /// Class defining the Assert-PSCompatibilityProfileIsValid cmdlet.
+    /// </summary>
+    [Cmdlet(VerbsLifecycle.Assert, CommandUtilities.MODULE_PREFIX + "ProfileIsValid")]


it's also possible this could be the verb Test and return true or false

Yeah I was weighing up those two. The problem is always that a boolean gives no diagnosis. I don't like that it throws an exception really, but it's better than a third custom solution

JamesWTruher

good to go

rjmholt requested review from JamesWTruher and bergmeister April 8, 2019 21:10

rjmholt changed the title ~~WIP: Convert compatibility module to binary module, fix compatibility with Azure environments~~ Convert compatibility module to binary module, fix compatibility with Azure environments Apr 9, 2019

bergmeister removed their request for review April 15, 2019 20:47

rjmholt requested review from daxian-dbw and SteveL-MSFT April 15, 2019 22:08

rjmholt and others added 23 commits April 24, 2019 14:46

Use fallback CIM API

2f93ca2

Add platform data collection type

df10417

Add C# implementations of powershell data collection

9d3c041

Add more .NET pwsh collection APIs

7159b8b

Add preliminary New-PSCompatibilityProfile cmdlet implementation

3fcb48d

Add new cmdlets, change JSON settings, add schema version

85bd373

Add most validation logic

528e47d

Update collectors, add assertion cmdlet

f7b3628

Fix files

8a75c91

Fix enumeration in json cmdlet

ab660e5

Fix service pack

d7c2169

Fix idiocy

9e2d19b

Fix generic parameter types being null

565dce6

Remove MMI, use registry for windows SKU info

42d4453

Convert scrap errors to warnings

c712760

Fix error -> warning usage

28443d4

Fix up sku querying

28bb895

Exclude compat module from own profiles

0399922

Add copyright

40943fb

Add doc comments

6bdbc20

Add missing docs

dd2035d

Rename PSCompatibilityAnalyzer to PSCompatibilityCollector

2ee3c5e

Change MMI version

b41f0eb

rjmholt and others added 13 commits April 24, 2019 14:48

Fix pester test skip

1413c4b

Add proper versioning to schema

a768d76

Fix up namespaces

66f92a7

Fix file ending newlines

758db97

Update schema version of azure profiles

6d10fb0

Fix namespaces in rules

eaf2ede

Rename Az profiles

a76ac6c

Fix type naming namespace change

138737d

Fix syntax in AzF profile

110a3f5

Add macOS profile

dced589

Prevent permissions problem reading bins for version

b700465

Add simple command tests for AzF profile

7f57e61

Add types tests

c9663ce

rjmholt force-pushed the azf-compat branch from f775136 to c9663ce Compare April 24, 2019 04:49

rjmholt added 9 commits April 24, 2019 15:03

Remove old compatibility module dir

2ce9992

Add rule enable settings

cf97e0b

Fix grouping logic

234fae4

Add breaks to switch

808a7eb

Make negative group name test more diagnostic

e65d011

Refine empty group test

f04009d

Fix other test

1c78a6b

Remove faulty test component

9f6ef22

Fix encoding of test files

69c483c

JamesWTruher reviewed Apr 24, 2019

View reviewed changes

rjmholt commented Apr 25, 2019

View reviewed changes

Address @JamesWTruher's feedback

0bbf54f

JamesWTruher reviewed Apr 25, 2019

View reviewed changes

Fix ConvertFrom-PSCompatibilityJson for loop

db6cd7b

JamesWTruher approved these changes May 10, 2019

View reviewed changes

JamesWTruher merged commit 064b622 into PowerShell:development May 10, 2019


		private const string THIS_MODULE_NAME = "PSCompatibilityCollector";

		private static readonly Regex s_typeDataRegex = new Regex("Error in TypeData \"([A-Za-z.]+)\"", RegexOptions.Compiled);

Convert compatibility module to binary module, fix compatibility with Azure environments #1212

Convert compatibility module to binary module, fix compatibility with Azure environments #1212

Conversation

rjmholt commented Apr 8, 2019 • edited Loading

PR Summary

List of changes

PR Checklist

bergmeister commented Apr 15, 2019

JamesWTruher 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

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

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

rjmholt Apr 25, 2019 • edited Loading

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

JamesWTruher left a comment

Choose a reason for hiding this comment

rjmholt commented Apr 8, 2019 •

edited

Loading

rjmholt Apr 25, 2019 •

edited

Loading