Fix Background Jobs in *nix and Windows #1972

nanalakshmanan · 2016-08-19T22:11:40Z

Resolve issue #452

msftclas · 2016-08-19T22:11:43Z

Hi @nanalakshmanan, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

It looks like you're a Microsoft contributor (Narayanan Lakshmanan). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.

TTYL, MSBOT;

daxian-dbw · 2016-08-19T22:30:40Z

src/System.Management.Automation/engine/hostifaces/PowerShellProcessInstance.cs


+#if CORECLR
+            processArguments = " -s -NoLogo -NoProfile";
+#endif 


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

it's better to not have unused code in CoreCLR build. Can you please make it like the following?

#if CORECLR string processArguments = " -s -NoLogo -NoProfile"; #elif // 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; 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

daxian-dbw · 2016-08-19T22:36:06Z

Fixing background jobs for Unix and Windows

The first commit seems using a different user email when it was committed. Can you please double check?

TravisEz13 · 2016-08-31T02:02:53Z

Still waiting on an answer to this question

ffeldhaus · 2016-09-15T20:57:01Z

As Start-Job is one of the more important Cmdlets, it would be great if we could get this merged as soon as possible. @nanalakshmanan can [comment on the question above?](daxian-dbw commented 27 days ago)

jsoref · 2016-09-20T17:20:02Z

test/powershell/engine/Jobs.Tests.ps1

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


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.

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.

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

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.

mirichmo · 2016-10-13T20:27:39Z

This is dependent on #2472

daxian-dbw · 2016-10-24T17:36:09Z

@nanalakshmanan The PR #2472 has been merged, and now powershell is building against the latest preview .NET Core bits.
Can you please update your changes accordingly?

vors · 2016-10-27T20:24:28Z

@nanalakshmanan can you, please, update the PR and resolve conflicts?

vors · 2016-11-08T01:38:34Z

src/System.Management.Automation/engine/hostifaces/PowerShellProcessInstance.cs

should it be else?

lzybkr · 2016-11-08T19:30:51Z

test/powershell/engine/Jobs.Tests.ps1

This line should be in a BeforeAll block.

Also, I think the job should be removed in a AfterAll block.

lzybkr · 2016-11-08T19:34:42Z

test/powershell/engine/Jobs.Tests.ps1

5 seconds might not be enough time to wait on a heavily loaded system, especially with debug binaries that aren't pre-jit compiled.

What is a good timeout that you suggest?

lzybkr · 2016-11-08T19:36:40Z

test/powershell/engine/Jobs.Tests.ps1

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


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.

daxian-dbw · 2016-11-08T20:17:23Z

@nanalakshmanan Github cannot recognize your account.

This is usually because the email account of your local clone is different from your email address of Github account. Can you please fix it?

msftclas added the cla-not-required label Aug 19, 2016

TravisEz13 added the Review - Needed label Aug 19, 2016

daxian-dbw reviewed Aug 19, 2016
View reviewed changes

daxian-dbw added the WG-Engine core PowerShell engine, interpreter, and runtime label Aug 19, 2016

daxian-dbw assigned lzybkr Aug 19, 2016

TravisEz13 added Review - Waiting on Author and removed Review - Needed labels Aug 22, 2016

daxian-dbw mentioned this pull request Aug 22, 2016

Filling test gaps (issue 2022) - this replaces PR 1834 #2023

Merged

TravisEz13 added Review - Needed and removed Review - Waiting on Author labels Aug 29, 2016

TravisEz13 added Review - Waiting on Author and removed Review - Needed labels Aug 31, 2016

TravisEz13 added Review - Needed and removed Review - Waiting on Author labels Sep 15, 2016

jsoref reviewed Sep 20, 2016

View reviewed changes

vors added Review - Waiting on Author and removed Review - Needed labels Oct 27, 2016

SteveL-MSFT mentioned this pull request Nov 2, 2016

Several Cmdlets from Microsoft.PowerShell.Core are missing #2122

Closed

nanalakshmanan force-pushed the JobFixes branch from ed4e099 to 8b50f92 Compare November 7, 2016 23:01

vors reviewed Nov 8, 2016

View reviewed changes

src/System.Management.Automation/engine/hostifaces/PowerShellProcessInstance.cs Outdated

Copy link

Collaborator

vors Nov 8, 2016

Choose a reason for hiding this comment

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

should it be else?

nanalakshmanan force-pushed the JobFixes branch 2 times, most recently from 13a7580 to 7b7e9e5 Compare November 8, 2016 02:59

vors added Review - Needed and removed Review - Waiting on Author labels Nov 8, 2016

lzybkr suggested changes Nov 8, 2016

View reviewed changes

nanalakshmanan force-pushed the JobFixes branch 2 times, most recently from 1655ae2 to c68e985 Compare November 9, 2016 23:47

Fixing background jobs for Unix and Windows

a515b1b

nanalakshmanan force-pushed the JobFixes branch from 5fcafd5 to a515b1b Compare November 10, 2016 00:05

daxian-dbw approved these changes Nov 10, 2016

View reviewed changes

daxian-dbw merged commit 2077e42 into PowerShell:master Nov 10, 2016

nanalakshmanan deleted the JobFixes branch January 7, 2017 21:26

joeyaiello mentioned this pull request Jan 25, 2017

PowerShell 6.0 Plan #3046

Closed

Fix Background Jobs in *nix and Windows #1972

Fix Background Jobs in *nix and Windows #1972

Uh oh!

Conversation

nanalakshmanan commented Aug 19, 2016

Uh oh!

msftclas commented Aug 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daxian-dbw commented Aug 19, 2016

Uh oh!

TravisEz13 commented Aug 31, 2016

Uh oh!

ffeldhaus commented Sep 15, 2016

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!

mirichmo commented Oct 13, 2016

Uh oh!

daxian-dbw commented Oct 24, 2016

Uh oh!

vors commented Oct 27, 2016

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!

daxian-dbw commented Nov 8, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants