PowerShell / PSScriptAnalyzer

Download ScriptAnalyzer from PowerShellGallery
https://www.powershellgallery.com/packages/PSScriptAnalyzer/
MIT License
1.87k stars 378 forks source link

Do not PSAvoidUsingDeprecatedManifestField if PowerShellVersion = '2.0' #428

Closed nightroman closed 8 years ago

nightroman commented 8 years ago

This manifest (some fields omitted)

@{
    ...
    ModuleToProcess = 'SplitPipeline.dll'
    PowerShellVersion = '2.0'
}

causes

RuleName                            Severity     FileName   Line  Message
--------                            --------     --------   ----  -------
PSAvoidUsingDeprecatedManifestField Warning      SplitPipel 1     The module manifest member 'ModuleToProcess' has been
s                                                ine.psd1         deprecated. Use the 'RootModule' member instead.

It should not if the target PowerShell version is 2.0, like in the above example.

The field RootModule was introduced in v3 and v2 fails to load such a manifest. Thus, for v2 ModuleToProcess is the only option and it should not cause a warning.

kilasuit commented 8 years ago

Currently PSScriptAnalyzer as of the latest release which was released yesterday has v3+ Support - prior to that it had v5 support only

So I would be inclined to say that building in full v2 support to PSScriptAnalyzer is very unlikely however you can exclude that rule when you run PSScriptAnalyzer by the below

Invoke-ScriptAnalyzer -ExcludeRule PSAvoidUsingDeprecatedManifestFields

Not the "best" solution but I expect the number of people still writing for PS v2 is declining somewhat so the effort to build in v2 support is less beneficial than adding further v3+ features

nightroman commented 8 years ago

@kilasuit, you are talking about something different.

The analyzer does not have to support v2. It has to check the manifest field PowerShellVersion. If it is '2.0' then the module is supposed to be used in v2 (or above but v2 is supported). In this case the analyzer should not complain about ModuleToProcess and require to use RootModule instead.

kilasuit commented 8 years ago

@nightroman - My point was that what you've raised as an issue is that the Rule is complaining about a point that only affects v2

To me this is requesting v2 support and in my opinion this is unlikely to be prioritised by the Script Analyser team.

Though I may add that there is nothing stopping you amending the Rule to include this check and submitting a pull request to add in this - however that then in my opinion opens the door for other v2 type support inclusions and it would be the Script Analyser Team to determine if that it is the direction that they want to spend time on.

nightroman commented 8 years ago

To me this is requesting v2 support and in my opinion this is unlikely to be prioritised by the Script Analyser team.

No, I am not requesting support for v2. A module being checked may be processed by the analyzer in PowerShell v5 or v4 or v3, whatever version the analyzer supports. So the issue does not affect only v2. It affects any current supported version. The thing is that the module in its manifests tells PowerShellVersion = '2.0', i.e. "I am designed for v2 or above". As far as v2 is included, the module must use ModuleToProcess, not RootModule. In this case the analyzer should accept ModuleToProcess without a warning. On the other hand, if the manifest says PowerShellVersion = '3.0' (or 4.0, 5.0) then the warning is valid.

KirkMunro commented 8 years ago

I agree with @nightroman. v2 is still in use, in some environments on servers where the upgrade from v2 is simply deemed not necessary, and in other environments where servers use products that require v2. Having this rule not warn for modules designed for v2 does not mean add support for v2. It means don't raise an invalid warning.

Aside: As long as v2 is still around, I don't like this rule anyway. If a user using v2 tries to load a module that uses RootModule in the manifest, PowerShell gives them an unintuitive error about invalid manifest property. If a user using v2 tries to load a module that requires v3+ that uses ModuleToProcess, they get an error that indicates the module requires v3+. All of my current crop of modules use ModuleToProcess, and none of them work on v2, for this very reason. As a result I get a warning that is completely benign as far as I understand because this attribute rename doesn't actually have any functional change behind it that would otherwise make it beneficial in my modules to switch to using RootModule instead.

kilasuit commented 8 years ago

In reference to the point "I am designed for v2 or above" - You are requesting that PSScriptAnalyzer has a check in this rule (and then other rules too) that has to support functionality that is only found in PowerShell v2

Where as currently PSScriptAnalyzer is built to support the v3 specifics so doesn't currently have any support for the v2 specifics - this issue being one of them.

I'm not against v2 support however as mentioned before this is unlikely to be prioritised by the Script Analyser team and there is nothing stopping you taking the time to amend the rule to make this check - or not running the rule at all.

Hopefully you can see where it is that I'm coming from on this as any of the language changes from v2 to v3 means building a fair amount of v2 specific checks - sometimes complete rules that would only be valid for use with v2 - and I think that although the benefit can be realised because of the amount of machines deployed that have still run v2 as pointed out by @KirkMunro and cant upgrade to v3 (SharePoint 2010 & Exchange 2010 for example) that this would be a bigger discussion which can be raised on the Community call (if you want to attend) to drive the correct route to go with this.

Does that make more sense

KirkMunro commented 8 years ago

Not exactly. You refer to functionality that is only found in PowerShell v2. ModuleToProcess is alive and well and works just fine even in v5. RootModule is only found in PowerShell v3+. So while I am on board for not adding rules for things only in v2, this isn't about functionality that is only in v2 at all.

Further, and more importantly, PowerShell v2 is still supported, and will be for several years yet, so keeping this rule quiet in the scenario described makes perfect sense to me.

nightroman commented 8 years ago

@KirkMunro,

Aside: As long as v2 is still around, I don't like this rule anyway. If a user using v2 tries to load a module that uses RootModule in the manifest, PowerShell gives them an unintuitive error about invalid manifest property.

It's a very good point and a reason to use the obsolete ModuleToProcess. All my modules support v2 so I did not even think of this subtle issue. So I agree that the whole rule is questionable due to the current design of PowerShell.

joeyaiello commented 8 years ago

After some discussion, we're proposing to check for PowerShellVersion = '2.0' up front to decide whether or not to run this rule. If it's there, we'll skip the rule altogether.

Any opposition?

KirkMunro commented 8 years ago

Maybe opposition. If you list PowerShellVersion = '2.0', then you must not use RootModule in the manifest because it won't work. If you skip this rule altogether, then you miss that scenario. And sure, someone might argue about version compatibility with older versions of PowerShell, but if they are still supported and if the checks are trivial (which they are), plus you already have checking for this attribute in place, so I think skipping doesn't sound like the best approach. Wouldn't it be better to rename the rule as something like PSVerifyAttributeCompatibility, and have it do the following:

  1. Raise a warning if you are using a deprecated attribute (although the "deprecation" part of the ModuleToProcess attribute is questionable).
  2. Raise an error if you are using an attribute that is altogether incompatible with the version you specify in PowerShellVersion.

Then internally it just needs to look at the version you have provided, and raise appropriate warnings/errors as required. That would be better for today as well as for future changes.

Unless you have some internal mandate to never rename attributes again and to never add new attributes, I think that would be a better approach for this rule. If you'll never rename attributes again nor add new attributes, then I guess I don't oppose too strongly against skipping it when PowerShellVersion = '2.0'. :)

joeyaiello commented 8 years ago

@KirkMunro: We all agree that that makes a lot of sense, and we'll very likely do what you suggest in the short term for module manifests.

We just want to be clear that this doesn't necessarily mean we'll be using PowerShellVersion to check the scripts for unsupported behavior or features. We may decide to do this in the long-term, but it's a significantly larger body of work that requires some more thought.

KirkMunro commented 8 years ago

Great, I'm glad you're going forward with this approach.

Understood about not doing version checking for script files yet, I wasn't expecting that as that's definitely a much larger chunk of work.

kilasuit commented 8 years ago

This was originally raised in #196 by @KirkMunro

quoctruong commented 8 years ago

This is fixed with https://github.com/PowerShell/PSScriptAnalyzer/pull/446

KirkMunro commented 8 years ago

Saw that, thanks! :)