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)

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

Extra line.

Will remove

Unnecessary change of line.

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

Closed.

Should this block be surrounded by #if !UNIX?

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

Should this be Test-Path instead of Test_path

Yes, odd that this test passed

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

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.

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.

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

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

yes, will fix

will fix

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

will fix

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.

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:

1. Make sure -Stream not available on Unix platforms
2. Throw error when the inline stream syntax is used for -Path on Unix platforms
3. 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.

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