Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upConvert compatibility module to binary module, fix compatibility with Azure environments #1212
Conversation
|
@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. |
|
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.
This comment has been minimized.
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.
This comment has been minimized.
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.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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.
This comment has been minimized.
This comment has been minimized.
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.
...PowerShell.CrossCompatibility/Collection/PlatformInformationCollector.cs
Outdated
Show resolved
Hide resolved
|
|
||
| try | ||
| { | ||
| functionData.ParameterSets = GetParameterSets(function.ParameterSets); |
This comment has been minimized.
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.
This comment has been minimized.
| for (int i = 0; i < outputTypeData.Length; i++) | ||
| { | ||
| outputTypeData[i] = outputType[i].Type != null | ||
| ? TypeNaming.GetFullTypeName(outputType[i].Type) |
This comment has been minimized.
This comment has been minimized.
| private static ReadOnlySet<string> GetPowerShellCommonParameterNames() | ||
| { | ||
| var set = new List<string>(); | ||
| foreach (PropertyInfo property in typeof(CommonParameters).GetProperties()) |
This comment has been minimized.
This comment has been minimized.
| @@ -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.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| /// 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.
This comment has been minimized.
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.
This comment has been minimized.
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.
This comment has been minimized.
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
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.
This comment has been minimized.
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.
This comment has been minimized.
|
|
||
| try | ||
| { | ||
| functionData.OutputType = GetOutputType(function.OutputType); |
This comment has been minimized.
This comment has been minimized.
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.
This comment has been minimized.
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.
This comment has been minimized.
| /// <summary> | ||
| /// The verison of the profile schema this profile object uses. | ||
| /// </summary> | ||
| [DataMember] |
This comment has been minimized.
This comment has been minimized.
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.
This comment has been minimized.
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.
This comment has been minimized.
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.
This comment has been minimized.
| return; | ||
|
|
||
| default: | ||
| throw new ArgumentException($"Unsupported type for {nameof(JsonSource)} parameter. Should be a string, FileInfo or TextReader object."); |
This comment has been minimized.
This comment has been minimized.
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.
This comment has been minimized.
| [Parameter] | ||
| public DotnetData DotNet { get; set; } | ||
|
|
||
| protected override void EndProcessing() |
This comment has been minimized.
This comment has been minimized.
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.
This comment has been minimized.
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.
This comment has been minimized.
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.
This comment has been minimized.
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.
This comment has been minimized.
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.
This comment has been minimized.
JamesWTruher
Apr 25, 2019
Member
I wonder if this has enough utility to be part of PowerShell (or .NET)
This comment has been minimized.
This comment has been minimized.
|
|
||
| namespace Microsoft.PowerShell.CrossCompatibility.Utility | ||
| { | ||
| internal static class PowerShellExtensions |
This comment has been minimized.
This comment has been minimized.
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.
This comment has been minimized.
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.
This comment has been minimized.
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.
This comment has been minimized.
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
|
good to go |
rjmholt commentedApr 8, 2019
•
edited
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
ConstituentProfilesfield to be aReadOnlySet<string>instead of aHashSet<string>Microsoft.PowerShell.CrossCompatibility.Utilitynamespace and into the base namespace,CollectionorRetrievaldepending on what the class does, since I thinkUtilityshould be reserved for internal helpersPR Checklist
.cs,.ps1and.psm1files have the correct copyright headerWIP:to the beginning of the title and remove the prefix when the PR is ready.