Remove alternate file stream code from non-Windows by SteveL-MSFT · Pull Request #4567 · PowerShell/PowerShell

Steve Lee (SteveL-MSFT) · 2017-08-14T11:54:01Z

AlternateStreams support relies on pinvoke to win32 apis which don't work

on non-Windows and produces errors about not loading a dll in the case of copy-item

Fix #4542

Ilya (iSazonov)

Leave a comment

Ilya (iSazonov) · 2017-08-14T13:53:27Z

Unblock-File is excluded from Unix at all so we don't need to exclude the code here.

That file is still being compiled on Unix

I think we should exclude them in csproj.

I'm fine with that but then we have effectively conditional compilation in both the source and the csproj files

Should we rename this to UnblockFile.Windows.cs?

In reply to: 133130873 [](ancestors = 133130873)

Ilya (iSazonov) · 2017-08-14T13:54:20Z

Please add a check that ${testPath}:Stream is a file name.

Doesn't Get-Content implicitly cover that?

I guess if the Content cmdlets were doing something funny it could mask an issue, I'll add a check

Ilya (iSazonov) · 2017-08-14T13:54:57Z

Extra line.

Will remove

Aditya Patwardhan (adityapatwardhan) · 2017-08-14T17:02:51Z

Unnecessary change of line.

The change of grouping of the variables makes it a bit more readable to me

Aditya Patwardhan (adityapatwardhan) · 2017-08-14T17:05:41Z

Should this block be surrounded by #if !UNIX?

Some of this code is unnecessary on UNIX so I'll wrap it

Aditya Patwardhan (adityapatwardhan) · 2017-08-14T17:07:52Z

Should this be Test-Path instead of Test_path

Yes, odd that this test passed

Aditya Patwardhan (adityapatwardhan) · 2017-08-15T16:53:43Z

Should we add #if !UNIX to the start of the file like: https://github.com/PowerShell/PowerShell/blob/master/src/System.Management.Automation/utils/tracing/EtwActivity.cs#L1

That seems more discoverable to me when doing code reviews or debugging.

That's what I had originally. Did a search and I don't see other cases of conditional compilation, so I think it's more consistent to use a whole file #ifdef. Ilya (@iSazonov) ok?

I think it's technically better to exclude files in csproj files. But currently we don't use a lot of opportunities of MSBuild and also we are cleaning up FullCLR files and respectively remove "Compile Remove" from csproj files. So it is Ok to use #ifdef .

/cc Dongbo Wang (@daxian-dbw) Could you comment too?

I don't really have a preference although csproj approach seems more discoverable. However in this case, I don't think Unblockfile.cs should be completely excluded from Unix build as it will increase the number of #if unix/endif blocks in places where the code from this file is used, which will impact readability.

Dan Travison (dantraMSFT) · 2017-08-15T23:28:25Z

This #if/#endif block is cumbersome to read. Suggest instead:
if (result != null)
{
#if UNIX
WriteItemObject(...)
#else
original code
}

The WriteItemObject code applies to both UNIX and !UNIX. If I move that bit of code up, I'd need more #ifdefs making it less readable

Actually, if you don't mind duplicating the WriteItemObject line, it can be a pure #if #else block. Your call.

I think the other case I was fine with a bit of duplication, but prefer to leave this one as-is.

Dan Travison (dantraMSFT) · 2017-08-15T23:30:53Z

As above, the code is easier to read if you use a #if UNIX and contain the UNIX lines completely. Its a simple if/else with a single statement for UNIX but looks unwieldy with the current #if/#else placement.

I can move some code around so the UNIX code is in one place and the !UNIX code is also together.

Dan Travison (dantraMSFT) · 2017-08-15T23:32:34Z

                if (isAlternateDataStream)

Should this also be conditioned on !UNIX?

Refers to: src/System.Management.Automation/namespaces/FileSystemProvider.cs:4270 in 7209973. [](commit_id = 720997351e1de1cf26593cfe0e95699d8b3b9eb3, deletion_comment = False)

Dan Travison (dantraMSFT) · 2017-08-15T23:39:03Z

        return Platform.NonWindowsIsSameFileSystemItem(pathOne, pathTwo);

Shouldn't there be a Platform.IsSameFileSystemItem?

Refers to: src/System.Management.Automation/namespaces/FileSystemProvider.cs:8274 in 7209973. [](commit_id = 720997351e1de1cf26593cfe0e95699d8b3b9eb3, deletion_comment = False)

Dan Travison (dantraMSFT) · 2017-08-15T23:51:11Z

On UNIX, _stream will be null causing a ArgumentNullException from new StreamReader at line 874. At best, it will be misleading. Consider throwing NotSupportException on UNIX in the associated constructors

makes sense, will change

New commits added

Steve Lee (SteveL-MSFT) · 2017-08-16T11:45:03Z

Dan Travison (@dantraMSFT) I addressed your feedback. Regarding Platform.IsSameFileSystem we should probably have a single API that is platform agnostic, but that's out of scope of this PR.

Steve Lee (SteveL-MSFT) · 2017-08-17T13:12:57Z

I believe all feedback has been addressed

Paul Higinbotham (PaulHigin)

LGTM

PR has been updated

Dan Travison (dantraMSFT) · 2017-08-17T19:03:51Z

Shouldn't this be in the #if !UNIX block?

yes, will fix

Dongbo Wang (daxian-dbw) · 2017-08-17T18:22:55Z

This variable won't be used on Unix and should be moved to the #if !UNIX block below.

Dongbo Wang (daxian-dbw) · 2017-08-17T19:19:19Z

The alternate streams can be specified using -Stream or an inline stream syntax "file::stream", the check of the inline stream syntax is done in 7 places in FileSystemProvider.cs but some of them are not within #if !UNIX block.

And when running Get-Content -Path file::stream on Linux, it probably should throw an error message about "alternate stream is not supported" instead of working unexpectedly or fail with other errors.

On Unix, a colon is completely allowed in a filename, so we want that to continue working as just a filename. Tests were added to make sure this works.

I'll fix other cases of using the file:stream syntax.

Dongbo Wang (daxian-dbw) · 2017-08-17T19:42:12Z

I thought about it more and now have this question: which the following experience is better -- copy Get-Content -Path <file> -Stream <stream> to Linux powershell and see the error "A parameter cannot be found", or "Alternate stream is not supported on this platform"? I think the latter message is better. To achieve this, maybe we can make the setter of the Stream property throw PlatformNotSupportedException on non-windows.

As for #if UNIX/endif blocks, IMHO, we don't necessarily need to have all codes that only apply to a specific platform guarded by the compile definition. I think it's OK to keep such codes around even for other platforms, and the guideline should be readability and maintainability. I still feel it's better to avoid most of the #if UNIX blocks by controlling the entry points:

Make sure -Stream not available on Unix platforms

Throw error when the inline stream syntax is used for -Path on Unix platforms

Skip the alternate stream discovery codes during remote copy (both PerformCopyFileFromRemoteSession and PerformCopyFileToRemoteSession) on Unix platforms.

After controlling those entry points (and put some comments for sure), it's safe to know that the alternate stream related code paths will not be hit on Unix platforms.

Travis Plunk (TravisEz13)

Please work on correctly resolving merge/rebase issue.

Travis Plunk (TravisEz13) · 2017-08-21T18:42:48Z

something isn't correct in your PR. Already committed changes are in the diff.

Travis Plunk (TravisEz13) · 2017-08-21T21:25:42Z

It looks like the email on your latest commit is not associated with your github account.

…work on non-Windows and produces errors about not loading a dll in the case of copy-item

added new -stream tests

Steve Lee (SteveL-MSFT) · 2017-08-21T21:49:02Z

Travis Plunk (@TravisEz13) fixed

Steve Lee (SteveL-MSFT) · 2017-08-25T21:16:54Z

Travis Plunk (@TravisEz13) Dongbo Wang (@daxian-dbw) feedback addressed

Comment is addressed by new commits.

* AlternateStreams support relies on pinvoke to win32 APIs which don't work on non-Windows and produces errors about not loading a DLL in the case of copy-item * Address PR feedback, remove -Stream parameter for Unix added new -stream tests * if/def out alternate stream syntax checks not applicable to Unix

Steve Lee (SteveL-MSFT) assigned Aditya Patwardhan (adityapatwardhan) Aug 14, 2017

Steve Lee (SteveL-MSFT) requested a review from Andrew (anmenaga) August 14, 2017 11:54

Microsoft Contribution License Agreements (msftclas) added the cla-not-required label Aug 14, 2017

Ilya (iSazonov) reviewed Aug 14, 2017

View reviewed changes

Aditya Patwardhan (adityapatwardhan) requested changes Aug 14, 2017

View reviewed changes

Steve Lee (SteveL-MSFT) force-pushed the copy-item-streams branch 2 times, most recently from 0191663 to 8c69d06 Compare August 15, 2017 07:41

Ilya (iSazonov) previously approved these changes Aug 15, 2017

View reviewed changes

Aditya Patwardhan (adityapatwardhan) previously requested changes Aug 15, 2017

View reviewed changes

Steve Lee (SteveL-MSFT) force-pushed the copy-item-streams branch from 8c69d06 to 7209973 Compare August 15, 2017 21:44

Steve Lee (SteveL-MSFT) requested review from Bruce Payette (BrucePay), James Truher (JamesWTruher), Paul Higinbotham (PaulHigin), Travis Plunk (TravisEz13), Dan Travison (dantraMSFT), Dongbo Wang (daxian-dbw), Jason Shirk (lzybkr) and sergei (vors) as code owners August 15, 2017 21:44

Travis Plunk (TravisEz13) removed request for Paul Higinbotham (PaulHigin), Travis Plunk (TravisEz13), Andrew (anmenaga) and sergei (vors) August 15, 2017 23:08

Dan Travison (dantraMSFT) reviewed Aug 15, 2017

View reviewed changes

Steve Lee (SteveL-MSFT) requested a review from Andrew (anmenaga) as a code owner August 16, 2017 09:45

Steve Lee (SteveL-MSFT) force-pushed the copy-item-streams branch 4 times, most recently from 22cc78b to f0260d7 Compare August 17, 2017 12:41

Paul Higinbotham (PaulHigin) approved these changes Aug 17, 2017

View reviewed changes

Dan Travison (dantraMSFT) reviewed Aug 17, 2017

View reviewed changes

Dongbo Wang (daxian-dbw) reviewed Aug 17, 2017

View reviewed changes

Steve Lee (SteveL-MSFT) assigned Dongbo Wang (daxian-dbw) and unassigned Aditya Patwardhan (adityapatwardhan) Aug 21, 2017

Travis Plunk (TravisEz13) previously requested changes Aug 21, 2017

View reviewed changes

Steve Lee (SteveL-MSFT) and others added 5 commits August 21, 2017 14:48

AlternateStreams support relies on pinvoke to win32 apis which don't …

154a1ee

…work on non-Windows and produces errors about not loading a dll in the case of copy-item

address PR feedback

77cc9d9

address PR feedback

6ae0bf9

address PR feedback, remove -Stream parameter for Unix

aed2f24

added new -stream tests

ifdef out alternate stream syntax checks not applicable to unix

7701b7a

Steve Lee (SteveL-MSFT) force-pushed the copy-item-streams branch from 8e6aa4e to 7701b7a Compare August 21, 2017 21:48

Dongbo Wang (daxian-dbw) approved these changes Aug 28, 2017

View reviewed changes

Dongbo Wang (daxian-dbw) merged commit deb8c44 into PowerShell:master Aug 28, 2017

Steve Lee (SteveL-MSFT) deleted the copy-item-streams branch August 29, 2017 00:01

Ilya (iSazonov) mentioned this pull request Oct 7, 2017

-STREAM cmdlets will never work on Linux so we need to remove them or have a clear error message #709

Closed

Uh oh!

Conversation

Steve Lee (SteveL-MSFT) commented Aug 14, 2017

Uh oh!

Ilya (iSazonov) left a comment

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!

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!

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!

Dan Travison (dantraMSFT) commented Aug 15, 2017

Uh oh!

Dan Travison (dantraMSFT) commented Aug 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Steve Lee (SteveL-MSFT) commented Aug 16, 2017

Uh oh!

Steve Lee (SteveL-MSFT) commented Aug 17, 2017

Uh oh!

Paul Higinbotham (PaulHigin) left a comment

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!