Skip to content
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

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

Merged
merged 74 commits into from May 10, 2019

Conversation

@rjmholt
Copy link
Member

rjmholt commented Apr 8, 2019

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 #1195, but I need it on my machine). When #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

@rjmholt rjmholt requested review from JamesWTruher and bergmeister Apr 8, 2019
@rjmholt 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
Copy link
Collaborator

bergmeister commented Apr 15, 2019

@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.

@bergmeister bergmeister removed their request for review Apr 15, 2019
@rjmholt rjmholt requested review from daxian-dbw and SteveL-MSFT Apr 15, 2019
@rjmholt rjmholt force-pushed the rjmholt:azf-compat branch from f775136 to c9663ce Apr 24, 2019
rjmholt added 9 commits Apr 24, 2019
Copy link
Member

JamesWTruher left a comment

here's the first of my comments

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

This comment has been minimized.

@JamesWTruher

JamesWTruher Apr 24, 2019 Member

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.

This comment has been minimized.

@rjmholt

rjmholt Apr 25, 2019 Author Member

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.

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

This comment has been minimized.

@JamesWTruher

JamesWTruher Apr 24, 2019 Member

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

This comment has been minimized.

@rjmholt

rjmholt Apr 25, 2019 Author Member

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.

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

This comment has been minimized.

@JamesWTruher

JamesWTruher Apr 24, 2019 Member

does dispose order matter here?

This comment has been minimized.

@rjmholt

rjmholt Apr 25, 2019 Author Member

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?

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

This comment has been minimized.

This comment has been minimized.

@rjmholt

rjmholt Apr 25, 2019 Author Member

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.


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

This comment has been minimized.

/// </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)

This comment has been minimized.

@JamesWTruher

JamesWTruher Apr 24, 2019 Member

GetVariablesName?

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

This comment has been minimized.

@JamesWTruher

JamesWTruher Apr 24, 2019 Member

i'm assuming this is where it can throw

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

This comment has been minimized.

@JamesWTruher

JamesWTruher Apr 24, 2019 Member

GetProperties(Public,Instance,DeclaredOnly)?

@@ -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)

This comment has been minimized.

@JamesWTruher

JamesWTruher Apr 24, 2019 Member

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

This comment has been minimized.

@rjmholt

rjmholt Apr 25, 2019 Author Member

Good catch!

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

This comment has been minimized.

@rjmholt

rjmholt Apr 25, 2019 Author Member

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.

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

This comment has been minimized.

@rjmholt

rjmholt Apr 25, 2019 Author Member

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.

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

This comment has been minimized.

@rjmholt

rjmholt Apr 25, 2019 Author Member

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?

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

This comment has been minimized.

@rjmholt

rjmholt Apr 25, 2019 Author Member

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.


private const string THIS_MODULE_NAME = "PSCompatibilityCollector";

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

This comment has been minimized.

@rjmholt

rjmholt Apr 25, 2019 Author Member

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.

// 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")

This comment has been minimized.

@rjmholt

rjmholt Apr 25, 2019 Author Member

Not a bad idea


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

This comment has been minimized.

@rjmholt

rjmholt Apr 25, 2019 Author Member

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


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

This comment has been minimized.

@rjmholt

rjmholt Apr 25, 2019 Author Member

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.

@@ -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)

This comment has been minimized.

@rjmholt

rjmholt Apr 25, 2019 Author Member

Good catch!

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

This comment has been minimized.

@rjmholt

rjmholt Apr 25, 2019 Author Member

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.

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

This comment has been minimized.

@JamesWTruher

JamesWTruher Apr 25, 2019 Member

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

This comment has been minimized.

@rjmholt

rjmholt Apr 25, 2019 Author Member

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

This comment has been minimized.

@rjmholt

rjmholt Apr 26, 2019 Author Member

Happy to go either way on this btw

return;

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

This comment has been minimized.

@JamesWTruher

JamesWTruher Apr 25, 2019 Member

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

This comment has been minimized.

@rjmholt

rjmholt Apr 25, 2019 Author Member

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

[Parameter]
public DotnetData DotNet { get; set; }

protected override void EndProcessing()

This comment has been minimized.

@JamesWTruher

JamesWTruher Apr 25, 2019 Member

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

This comment has been minimized.

@rjmholt

rjmholt Apr 25, 2019 Author Member

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

}

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

This comment has been minimized.

@JamesWTruher

JamesWTruher Apr 25, 2019 Member

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

This comment has been minimized.

@rjmholt

rjmholt Apr 25, 2019 Author Member

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

This comment has been minimized.

@JamesWTruher

JamesWTruher May 10, 2019 Member

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

using System.Collections;
using System.Collections.Generic;

namespace Microsoft.PowerShell.CrossCompatibility

This comment has been minimized.

@JamesWTruher

JamesWTruher Apr 25, 2019 Member

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

This comment has been minimized.

@rjmholt

rjmholt Apr 25, 2019 Author Member

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


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

This comment has been minimized.

@JamesWTruher

JamesWTruher Apr 25, 2019 Member

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.

This comment has been minimized.

@rjmholt

rjmholt Apr 25, 2019 Author Member

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.

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

This comment has been minimized.

@JamesWTruher

JamesWTruher Apr 25, 2019 Member

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

This comment has been minimized.

@rjmholt

rjmholt Apr 25, 2019 Author Member

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

Copy link
Member

JamesWTruher left a comment

good to go

@JamesWTruher JamesWTruher merged commit 064b622 into PowerShell:development May 10, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla All CLA requirements met.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.