Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -1188,6 +1188,12 @@ public virtual ScriptBlock ScriptBlock

private ScriptBlock _scriptBlock;

/// <summary>
/// Gets or sets the initial location of the script block.
/// This is used to set the location prior to executing the script block.
/// </summary>
public virtual string WorkingDirectory { get; set; }

/// <summary>
/// The file containing the script that the user has specified in the
/// cmdlet. This will be converted to a powershell before
Expand Down Expand Up @@ -1843,6 +1849,13 @@ internal Pipeline CreatePipeline(RemoteRunspace remoteRunspace)

pipeline.Commands.Clear();

if (WorkingDirectory != null)
{
var command = new Command("Set-Location");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably the module, like:

var command = new Command("Microsoft.PowerShell.Management\\Set-Location");

command.Parameters.Add("LiteralPath", WorkingDirectory);
pipeline.Commands.Add(command);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something about this doesn't feel right. Starting with the test:

Start-Job -ScriptBlock { Join-Path $pwd "" } -WorkingDirectory $tempPath 

I think this code does the equivalent of:

Set-Location $tempPath | Join-Path $pwd ""

Instead of:

Set-Location $tempPath
Join-Path $pwd ""

I think you need to call PSCommand.AddStatement on this command you're adding.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly what you are saying is that the object Pipeline represents only the powershell pipeline and if I want to create a sequence of commands I would need to add a statement to the powershell instance instead of adding a command to the pipeline.

This means that the correct approach is to have something like this:

_powershellV3 = System.Management.Automation.PowerShell.Create();
if (WorkingDirectory != null)
{
    _powershellV3.AddStatement().
             AddCommand("Microsoft.PowerShell.Management\\Set-Location").
             AddParameter("LiteralPath", WorkingDirectory);
}

I would assume that this statement should be added for both when the server is ps2 and ps5+.

(P.S. thanks for the comment it helped me understand the code way better)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think what @lzybkr is trying to draw your attention to is what happens if WorkingDirectory != null on line 1852 of PSRemotingCmdlet.cs is true. In that case, you add the Set-Location command to the empty set of pipeline commands (the collection that was previously cleared on line 1850), but then logic continues beyond your if-statement and commands are added as additional commands in that same pipeline that Set-Location is in on lines 1859-1862. If you insert pipeline.Commands.AddStatement(); before line 1857, you'll properly terminate the statement that invokes Set-Location, allowing the commands that are being invoked to be property added as a separate commands following that statement.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PowerShell api was originally designed for a single pipeline, but you could do more with that api if you used AddScript.

Some secure endpoints disallow script (no language mode), so the PowerShell instance must be expressible as commands. At some point in the past, this also meant a "Pipeline" was exactly that, it was a single pipeline.

I think in V3 we added support for multiple distinct statements via the AddStatement api. You can think of AddCommand as adding a | between every statement unless AddStatement was called, then we'd add a ; instead. I also think that means the correct code is more like:

    _powershellV3.
             AddCommand("Microsoft.PowerShell.Management\\Set-Location").
             AddParameter("LiteralPath", WorkingDirectory).
             AddStatement();

I do see that the test passed though, and that surprised me. I'd do some more testing to verify my assumptions above.

I don't recall specifics about going between the PowerShell api and the Pipeline api, and to be honest, I find the original code here (before the PR) a little confusing, so I'm reluctant to say that AddStatement is definitely needed.

I should also mention the need for compatibility testing if AddStatement is needed. I recall a bug where the conversion from a script block to a PowerShell instance could not use AddStatement because of a bug in the server side code. If I'm remembering this correctly, the bug was fixed in V5, so it would probably be worth testing across different server versions.

}

foreach (Command command in powershellToUse.Commands.Commands)
{
pipeline.Commands.Add(command);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -480,6 +480,24 @@ public virtual ScriptBlock InitializationScript

private ScriptBlock _initScript;

/// <summary>
/// Gets or sets a working directory of the process.
/// </summary>
[Parameter]
[ValidateNotNullOrEmpty]
public override string WorkingDirectory
{
get
{
return base.WorkingDirectory;
}

set
{
base.WorkingDirectory = value;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be simplified to:

get => base.WorkingDirectory;
set => base.WorkingDirectory = value;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No strong opinion here but: I am tempted to leave this as it is and the reason for that is the rest of the file that I am touching follows this pattern for the get\sets source

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally speaking from what I've seen, when most contributors add new code, they do so using the current practices rather than match existing syntax. This keeps us forward moving. Eventually PRs come in to advance old syntax to catch up to newer syntax.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I will fix it in the next iteration

}

/// <summary>
/// Launches the background job as a 32-bit process. This can be used on
/// 64-bit systems to launch a 32-bit wow process for the background job.
Expand Down Expand Up @@ -589,6 +607,18 @@ protected override void BeginProcessing()
ThrowTerminatingError(errorRecord);
}

if (WorkingDirectory != null && !Directory.Exists(WorkingDirectory))
{
string message = StringUtil.Format(RemotingErrorIdStrings.StartJobWorkingDirectoryNotFound, WorkingDirectory);
var errorRecord = new ErrorRecord(
new DirectoryNotFoundException(message),
"DirectoryNotFoundException",
ErrorCategory.InvalidOperation,
targetObject: null);

ThrowTerminatingError(errorRecord);
}

CommandDiscovery.AutoloadModulesWithJobSourceAdapters(this.Context, this.CommandOrigin);

if (ParameterSetName == DefinitionNameParameterSet)
Expand Down Expand Up @@ -623,7 +653,7 @@ protected override void CreateHelpersForSpecifiedComputerNames()
ErrorCategory.PermissionDenied,
Context.LanguageMode));
}

NewProcessConnectionInfo connectionInfo = new NewProcessConnectionInfo(this.Credential);
connectionInfo.InitializationScript = _initScript;
connectionInfo.AuthenticationMechanism = this.Authentication;
Expand All @@ -633,14 +663,14 @@ protected override void CreateHelpersForSpecifiedComputerNames()
Utils.GetTypeTableFromExecutionContextTLS());

remoteRunspace.Events.ReceivedEvents.PSEventReceived += OnRunspacePSEventReceived;

Pipeline pipeline = CreatePipeline(remoteRunspace);

IThrottleOperation operation =
new ExecutionCmdletHelperComputerName(remoteRunspace, pipeline);

Operations.Add(operation);
}

/// <summary>
/// The expression will be executed in the remote computer if a
/// remote runspace parameter or computer name is specified. If
Expand Down Expand Up @@ -724,7 +754,7 @@ protected override void ProcessRecord()
if (_firstProcessRecord)
{
_firstProcessRecord = false;

PSRemotingJob job = new PSRemotingJob(ResolvedComputerNames, Operations,
ScriptBlock.ToString(), ThrottleLimit, _name);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1301,6 +1301,9 @@ All WinRM sessions connected to PowerShell session configurations, such as Micro
<value>Cannot find a scheduled job with type {0} and name {1}.</value>
<comment>{0} is the job definition type and {1} is the job definition name.</comment>
</data>
<data name="StartJobWorkingDirectoryNotFound" xml:space="preserve">
<value>Cannot find the directory {0}.</value>
</data>
<data name="CannotFindSessionForConnect" xml:space="preserve">
<value>Cannot connect to session {0}. The session no longer exists on computer {1}.</value>
<comment>{0} is the session name that cannot be found.
Expand Down
8 changes: 8 additions & 0 deletions test/powershell/engine/Job/Jobs.Tests.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,14 @@ Describe 'Basic Job Tests' -Tags 'CI' {
$ProgressMsg[0].StatusDescription | Should -BeExactly 2
}

It 'Can use the user specified working directory parameter' {
$tempPath = [System.IO.Path]::GetTempPath()
# In the script block we use join path to normalize the path string
$job = Start-Job -ScriptBlock { Join-Path $pwd "" } -WorkingDirectory $tempPath | Wait-Job
$jobOutput = Receive-Job $job
$jobOutput | Should -BeExactly $tempPath
}

It "Create job with native command" {
try {
$nativeJob = Start-job { pwsh -c 1+1 }
Expand Down