Add new functionality and restrictions to #requires #122

rjmholt · 2018-03-23T06:56:02Z

No description provided.

markekraus · 2018-03-23T08:40:56Z

One thing I would like clarified by the RFC is how it will interact with Linux shbang

#!/usr/bin/env pwsh
#requires -OS Linux

# Some linux specific code

I would like to ensure that is not broken.

rjmholt · 2018-03-23T18:20:29Z

@markekraus Yes I had that specifically in mind. I will make the language more explicit.

lzybkr · 2018-03-23T19:30:27Z

1-Draft/RFCNNNN-#Requires-Additions.md

+  after will cause a parse-time error. This would be a **breaking
+  change**, albeit one that the documentation already claims to be 
+  in force.
+* Using `#requires` in the interactive console will cause


I'm not sure this provides any benefit, and I'm not aware of a straightforward way to implement it.

Today the engine knows nothing about interactive input - that's the domain of a host, but the host for the most part doesn't care what the input looks like.

One thought would be to only allow #requires in a file, but that won't work in some important scenarios, e.g. Invoke-Command -File foo.ps1 sends the contents of the file to the remote session, but the remote session doesn't know about a file.

People also use Get-Content or Invoke-WebRequest piped to Invoke-Expression - again losing the fact that the input was really in a file.

@lzybkr You're referring to preventing #requires in an interactive session here right? I'm not particularly invested in this feature, but saw PowerShell issue #3803 and implemented this PR which still throws an error. Thought I would include it in the RFC for the purposes of discussion.

I see - in that context, interactive == add to history. From the code, it's not obvious that it doesn't break the Invoke-Command scenario I mentioned, nor is it obvious under what conditions one might invoke something adding to history where the command isn't actually interactive.

Ah right! Admittedly part of me thinks that piping into Invoke-Expression is something we should discourage, but I suppose we shouldn't break it, and moreover that #requires is probably more useful doing what it promises to do there. But good that I now understand the full context of the interactiveCommand; I'll roll back a couple of those changes.

Seems there is some scenarios where "interactive" is important (ex.: don't load some modules). We already added -i to be compatible with Bash. I'd want to get that the engine knows that the session is interactive.
Perhaps @mklement0 have more thoughts.

@iSazonov - an interactive session is different than a command entered interactively.

Set-ExecutionPolicy is the only place in the engine that I know of that infers if the command is entered interactively and alters it's behavior accordingly.

lzybkr · 2018-03-23T19:35:46Z

1-Draft/RFCNNNN-#Requires-Additions.md

+  any executable PowerShell code). Placing `#requires` anywhere
+  after will cause a parse-time error. This would be a **breaking
+  change**, albeit one that the documentation already claims to be 
+  in force.


I don't have a strong opinion on introducing a breaking change, but I suppose some might see a benefit in putting #requires elsewhere, e.g. at the bottom of the file, perhaps to somewhat hide what might seem like an unimportant implementation detail.

Instead of a breaking change, the documentation could be made clearer. The directive is not scoped - it applies to the outermost ScriptBlock in which the directive appears.

Not normative, just some history: Originally #requires had to be the first non-space non-comment line in the file. This is because we were asked to consider the case were we got the language sufficiently wrong that we had to introduce incompatible changes. In that scenario, #requires -version X might cause a different parser to be used for the file. (This feature, along with numbers in the extensions (.ps1), and snapins were all part of the pre v1 versioning reviews.) #requires was also done in a preprocessing phase, rather than as part of the main parse. I think @lzybkr changed this in the V3 parser rewrite.

SteveL-MSFT · 2018-03-23T19:41:23Z

1-Draft/RFCNNNN-#Requires-Additions.md

+    be specified as required. See [this PowerShell issue](https://github.com/PowerShell/PowerShell/issues/3751).
+  * `-Assembly <Assembly-name>`, where a .NET assembly can
+    be specified as required. See [this PowerShell issue](https://github.com/PowerShell/PowerShell/issues/5022).
+  * `-MaxVersion <V>[.<v>]`, where a maximum PowerShell


I don't know if we need a -MaxVersion. The original issue was trying to differentiate Windows PowerShell and PowerShell Core. Since these changes wouldn't go down to Windows PowerShell, it seems to be of limited use to use -MaxVersion to differentiate between the two.

I think it does provide some benefit. Even though a version of PSCore is only supported for 18 months? scripts will be written in that time for that version. If we implement breaking changes in 6.3.0, a script written during 6.1.0's lifetime could put a max version of 6.1.0 in it to ensure that anyone running an older version of the script on a newer 6.3.0 version of pwsh, they will fail fast rather than risk potential broken behavior.

This goes to the larger discussion of how to deal with breaking changes, but I think this feature actually makes it possible to do them without major version madness.

@markekraus for your scenario, it seems that an -ExactVersion would be more useful if you have business critical scripts that you want time to verify before moving to a newer version of PSCore6

-ExactVersion would. for some instances. I think if we are committed to X.Y.Z versions where Z changes never include a breaking change, something that somehow limiting to a minor (Y) version would work and probably be what is desirable for most cases. Basically way to do (version >= 6.X.0 && version < 6.(X+1).0).

If we say about business critical scripts I'd want to see error events in logs if the directive catch wrong version.

If we'll want the -MaxVersion and -ExactVersion I'd re-use -Version with prefixes like <,>, ... - -Version <=6.1.0

@iSazonov That would definitely make it more flexible... but... something like -ExactVersion 6.0.2 and -MinorVersion 6.1 would cover probably most use cases (assuming we don't suddenly start making breaking changes in the PATCH level iterations)

SteveL-MSFT · 2018-03-23T19:41:51Z

1-Draft/RFCNNNN-#Requires-Additions.md

+   console.
+3. `#requires` can take an `-OS` parameter, with possible
+   arguments being `Windows`, `Linux` and `MacOS` (and
+   possibly some syntax for `or`-ing them). This check


Probably -OS Linux,macOS

Maybe use generic -OS Unix for all supported Unixes?
And should we consider OS version? -OS Unix(Ubuntu 16.04)

Using Unix as generic term for Linux , Unix, and macOS is 1) not accurate and 2) really makes some people angry. 🙁 I would advise against anything Microsoft does making that mistake as it then makes MS look either ignorant or arrogant depending on what issue a person takes with that usage.

@markekraus: I think @iSazonov is suggesting Unix as an additional identifier, to serve as an abstraction over macOS and Linux for scripts that run on both. Given that both platforms have a Unix heritage and many commonalities (especially in contrast with Windows), it is a useful abstraction. It is the case variant UNIX that is trademarked; I think that the term Unix to loosely refer to all platforms with a Unix heritage is well-established - see https://en.wikipedia.org/wiki/Unix

@mklement0 Yes, and that usage is contentious to the point of being inflammatory. That's is why I'm recommending against it. I understood @iSazonov intent.

I'll also counter that it is not practical. My exposure to CoreFX is that macOS is it's own beast. That's not even considering the radically different ecosystem from Linux. The gap is wide enough that I see very few cases where Unix would be useful to mean not-windows.

With that and the inflammatory nature a Microsoft product of using Unix to lump them together further coupled with "there is nothing wrong with -OS Linux,macOS" I'd say we shouldn't use it.

Though another category might be helpful: Windows, Linux, macOS, IoT

@markekraus: Whether the abstraction is useful should be discussed separately from its name.

To me, the abstraction is useful (speaking as a shell user on both platforms) - it all depends on the use case (we're not talking system-level programs here); perhaps others can weigh in.

If the name Unix is really that contentious (which I'm personally not aware of), perhaps UnixLike will do.

Current .Net Core team support goal is "we should run on all platforms". So maybe we should use more generic parameter -OS and sub-parameters for platform, version ...

"UNIX" is a trademark. "UnixLike" doesn't address the aversion to "Unix", but maybe "LikeUnix" could be shortened to… "Linux".

exchange12rocks · 2018-03-23T22:42:42Z

I cannot agree that 'requires' statements should be allowed in scripts only - I personally find it very useful in functions

iSazonov · 2018-03-24T16:36:26Z

I think we could keep scripts as simple as possible and not enhance this directive.
All of the inhancements described here are more useful for modules and we could put them in a module manifest.
If a user wants strict (version) control for a business critical script, then he will have to checkout the script as a module.

joeyaiello · 2019-02-04T21:28:02Z

Reviewed this one with @JamesWTruher and @SteveL-MSFT: we feel like #requires should stay fairly high-level, and that using should be used for assemblies and modules.

To that end, the only ones we'd like to add is -OS and -MaxVersion. The former should map directly to the $IsOS variables with possible values of Windows, macOS, or Linux. We don't want to get into the weeds with specific OS versions (these can still be done at runtime). With -MaxVersion we should use basic semver strings: 6 would be inclusive of all 6.x.x versions, 6.2 would be inclusive of everything 6.2.x and below, etc.

The only other open question is whether we enable loose validation of #requires going forward so that we can continue to add more values without breaking older versions.

Still need quorum to close on this, though.

iSazonov · 2019-02-05T04:29:00Z

I agree with the conclusion and I don't like to enhance #requires directive in future.

brianary · 2019-02-05T06:26:36Z

The using syntax is fine, but the performance should be significantly improved (see PowerShell #5022).

joeyaiello · 2019-02-13T23:59:59Z

@brianary agreed, but that same problem would exist if we added that behavior here.

@HemantMahawar suggested we alias -Version to -MinimumPSVersion for clarity, and then have -MaximumVersion called -MaximumPSVersion ("maximum" for consistency with Import-Module).

We'll wait until we have post-snow quorum to wrap this up with a vote. :)

brianary · 2019-02-14T21:15:59Z

It could, it just depends on how it's implemented. If the existing syntax can't improve the performance due to backward compatibility issues, this would provide an alternative. Maybe I need to open a separate issue to address the performance.

…

On Wed, Feb 13, 2019, 16:00 Joey Aiello ***@***.*** wrote: @brianary <https://github.com/brianary> agreed, but that same problem would exist if we added that behavior here. @HemantMahawar <https://github.com/HemantMahawar> suggested we alias -Version to -MinimumPSVersion for clarity, and then have -MaximumVersion called -MaximumPSVersion ("maximum" for consistency with Import-Module). We'll wait until we have post-snow quorum to wrap this up with a vote. :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#122 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AH8mJ49By6iMQaw_MARWyNLyvQnBaTRgks5vNKcBgaJpZM4S4QSQ> .

SteveL-MSFT · 2019-02-25T21:41:13Z

@PowerShell/powershell-committee reviewed this and is fine with current proposal to alias for clarity and adding -OS and -MaximumPSVersion

rjmholt · 2019-02-26T18:24:54Z

I've edited the RFC to adapt to the @PowerShell/powershell-committee's decisions. Please let me know if further edits are required

iSazonov · 2019-02-27T05:26:19Z

1-Draft/RFCNNNN-#Requires-Additions.md

+  * `-MinimumPSVersion` as an alias of `-MinimumVersion`.
+
+* **Withdrawn**, in favor of `using` statements.
+  ~~`-Assembly <Assembly-name>`, where a .NET assembly can


Should we remove this?

Just kept it for context. Happy to remove if that's better. Or add a Withdrawn section to contain these things

joeyaiello · 2019-03-04T20:54:58Z

Per @SteveL-MSFT 's request, can you move all the withdrawn stuff to an "Alternate Considerations" section near the bottom, along with explanation of what it is that was rejected/withdrawn?

rjmholt · 2019-04-15T18:24:38Z

@joeyaiello @SteveL-MSFT @PowerShell/powershell-committee This has now been updated to comply with the request above

rjmholt added 2 commits March 22, 2018 23:43

Add #requires RFC draft

4eb9ab0

Correct language area

4c7c592

rjmholt added 2 commits March 23, 2018 11:28

More info on hashbangs

0941a56

Add PR info for interactive #requires

070e0db

lzybkr reviewed Mar 23, 2018

View reviewed changes

SteveL-MSFT reviewed Mar 23, 2018

View reviewed changes

joeyaiello added the Review - Committee label Feb 4, 2019

Edit RFC inline with committee ruling

e833578

iSazonov reviewed Feb 27, 2019

View reviewed changes

SteveL-MSFT added Review - Waiting on Author and removed Review - Committee labels Mar 18, 2019

Move withdrwan proposals to the bottom

3e974d4

joeyaiello added 2 commits April 15, 2019 13:24

Accept and merge RFC0035 on #requires additions

37a2dcd

Fix small typo in RFC0035

ae5a103

joeyaiello merged commit 0336332 into PowerShell:master Apr 15, 2019

ThomasNieto mentioned this pull request Jul 30, 2019

Discussion: New properties to add to module manifest? PowerShell/PowerShell#10251

Closed

iSazonov mentioned this pull request Nov 18, 2019

'#Requires -Assembly' does not have any effect PowerShell/PowerShell#11098

Closed

Add new functionality and restrictions to #requires #122

Add new functionality and restrictions to #requires #122

Uh oh!

Conversation

rjmholt commented Mar 23, 2018

Uh oh!

markekraus commented Mar 23, 2018

Uh oh!

rjmholt commented Mar 23, 2018

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!

markekraus Mar 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

iSazonov Mar 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

mklement0 Mar 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

exchange12rocks commented Mar 23, 2018

Uh oh!

iSazonov commented Mar 24, 2018

Uh oh!

joeyaiello commented Feb 4, 2019

Uh oh!

iSazonov commented Feb 5, 2019

Uh oh!

brianary commented Feb 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joeyaiello commented Feb 13, 2019

Uh oh!

brianary commented Feb 14, 2019 via email

Uh oh!

SteveL-MSFT commented Feb 25, 2019

Uh oh!

rjmholt commented Feb 26, 2019

markekraus Mar 23, 2018 •

edited

Loading

iSazonov Mar 24, 2018 •

edited

Loading

mklement0 Mar 24, 2018 •

edited

Loading

brianary commented Feb 5, 2019 •

edited

Loading