Skip to content

Fix Background Jobs in *nix and Windows #1972

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
daxian-dbw merged 1 commit into from
Nov 10, 2016
+40 −2
Merged

Fix Background Jobs in *nix and Windows #1972

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
15 changes: 13 additions & 2 deletions src/System.Management.Automation/engine/hostifaces/PowerShellProcessInstance.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,13 @@ public sealed class PowerShellProcessInstance : IDisposable
/// </summary>
static PowerShellProcessInstance()
{
#if UNIX
s_PSExePath = Path.Combine(Utils.GetApplicationBase(Utils.DefaultPowerShellShellID),
"powershell");
#else
s_PSExePath = Path.Combine(Utils.GetApplicationBase(Utils.DefaultPowerShellShellID),
"powershell.exe");
#endif
}

/// <summary>
Expand Down Expand Up @@ -71,18 +76,22 @@ public PowerShellProcessInstance(Version powerShellVersion, PSCredential credent
}
}

string processArguments = string.Empty;
#if CORECLR
string processArguments = " -s -NoLogo -NoProfile";
#else
// Adding Version parameter to powershell.exe
// Version parameter needs to go before all other parameters because the native layer looks for Version or
// PSConsoleFile parameters before parsing other parameters.
// The other parameters get parsed in the managed layer.
Version tempVersion = powerShellVersion ?? PSVersionInfo.PSVersion;
processArguments = string.Format(CultureInfo.InvariantCulture,
string processArguments = string.Format(CultureInfo.InvariantCulture,
"-Version {0}", new Version(tempVersion.Major, tempVersion.Minor));

processArguments = string.Format(CultureInfo.InvariantCulture,
"{0} -s -NoLogo -NoProfile", processArguments);

#endif

if (initializationScript != null)
{
string scripBlockAsString = initializationScript.ToString();
Expand All @@ -106,7 +115,9 @@ public PowerShellProcessInstance(Version powerShellVersion, PSCredential credent
RedirectStandardOutput = true,
RedirectStandardError = true,
CreateNoWindow = true,
#if !UNIX
LoadUserProfile = true,
#endif
};

if (credential != null)
Expand Down
4 changes: 4 additions & 0 deletions src/System.Management.Automation/utils/CryptoUtils.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1196,7 +1196,11 @@ internal class PSRemotingCryptoHelperServer : PSRemotingCryptoHelper
/// </summary>
internal PSRemotingCryptoHelperServer()
{
#if UNIX
_rsaCryptoProvider = null;
#else
_rsaCryptoProvider = PSRSACryptoServiceProvider.GetRSACryptoServiceProviderForServer();
#endif
}

#endregion Constructors
Expand Down
23 changes: 23 additions & 0 deletions test/powershell/engine/Jobs.Tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
Describe 'Basic Job Tests' -Tags 'CI' {

BeforeAll {
$job = Start-Job {1}
}

It 'Test job creation' {
$job | should not be $null
}

It 'Test job State' {
Wait-Job $job -Timeout 60
$job.JobStateInfo.State -eq 'Completed' | should be $true
}

It 'Job output test' {
Receive-Job $job -wait | should be 1
}

AfterAll {
Remove-Job $job -Force
}
}
Copy link
Contributor

@jsoref jsoref Sep 20, 2016

Choose a reason for hiding this comment

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

Is there a policy about missing EOL at EOF?

Personally, as someone who uses hg and git (and Linux), missing EOL markers at EOF result in lots of pain which I'd rather avoid.

Copy link
Contributor

@lzybkr lzybkr Nov 8, 2016

Choose a reason for hiding this comment

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

I haven't seen a strong argument for requiring EOL at EOF, and it comes up often enough to be somewhat annoying and distracting.

If it's truly a problem for some tools, I think we should just run a tool over our source from time to time and fix it instead of asking each contributor every time we see it.

Copy link
Contributor

@jsoref jsoref Nov 8, 2016

Choose a reason for hiding this comment

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

I'm not sure where to start.

The length of a file "" and the length of a file "\n" aren't the same.
Version control systems at some point rely on hashing content and ensuring that they're reproducible.

diff and patch work on lines (things that end in "\n"). A patch generated for a properly ended line doesn't apply well to an improperly ended line and vice versa. This makes it hard to graft and merge changes.

Other things like wc (word count tool which also does line count) can be implemented by counting \ns, a file "hello\n" can be defined to have 1 line because it has 1 \n.

Many text editors as a courtesy to other tools (like diff and vcs and ...) will automatically insert the EOL at EOF.

There's a general goal in VCS for commits to have descriptions that describe the entirety of their changes, and for blame to be assignable based on the last user to change a given line.

These factors result in a file like:
"hello.....\n......\nworld" which was created by one person but, where I change "hello" to "Hello" with a message of "capitalizing Hello" also resulting in my commit often changing the file to:
"Hello.....\n......\nworld\n"

And when someone uses a tool to quickly view the blame for lines of the file, they'll think that I was responsible for "world\n".

If you've ever had problems trying to apply changes where a patch was \r\n and a file was \n, or where a file had mixed line endings, the missing EOL at EOF triggers roughly similar failures.

Trying to apply a change that's missing a line ending to a line that has a line ending:

$ cat c; d -c 4; d -c 4|patch c
Hello
world
a
diff --git a/c b/c
--- a/c
+++ b/c
@@ -1,3 +1,3 @@
 Hello
 world
-a
\ No newline at end of file
+b
\ No newline at end of file
patching file c
Hunk #1 FAILED at 1.
1 out of 1 hunk FAILED -- saving rejects to file c.rej

Trying to apply a change with a line ending to a line that is missing a line ending:

 (cat c; echo XXX); d -r 1 -r 2; d -r 1 -r 2|patch c
Hello
world
aXXX
diff --git a/c b/c
--- a/c
+++ b/c
@@ -1,3 +1,3 @@
 Hello
 world
-a
+b
patching file c
Hunk #1 FAILED at 1.
1 out of 1 hunk FAILED -- saving rejects to file c.rej

Copy link
Contributor

@jsoref jsoref Nov 8, 2016

Choose a reason for hiding this comment

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

Converting the repository to not be missing line endings would be appreciated.
But I would still encourage you to also discourage people from adding files that are missing EOL at EOF because unless your script automatically commits the line changes as the user who added the line, you'll be distorting authorship for those lines.