Linux: ProcessName: return script name instead of interpreter program #37294
Conversation
|
@stephentoub @wtgodbe @danmosemsft @krwq ptal This matches with behavior of previous .NET Core versions, which are returning the script name in Process.ProcessName. Gnome system monitor uses this value in its 'Process Name' column. |
|
/azp run corefx-outerloop-linux |
|
Any idea what it would take to make macos work? |
|
CI failed:
/azp run corefx-outerloop-linux
Mac user not found between chair and keyboard :) |
|
/azp run |
|
@danmosemsft am I using /azp correctly? |
|
@safern or @ViktorHofer can help with CI |
|
@ViktorHofer will that do outer loop though? that was what @tmds was trying. |
That is an intermitent issue that Github API has and Azure DevOps hits. They're investigating how to fix it or retry when getting the yml file. Now, when hitting this issue we need to retry it manually 😢 |
|
/azp run corefx-outerloop-linux |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
FAilure was dotnet/msbuild#4322 I hit rerun. |
| { | ||
| try | ||
| { | ||
| Assert.Equal(scriptName, process.ProcessName); |
There was a problem hiding this comment.
should this actually return interpreter (whatever the shebang states) and script name in the arguments?
There was a problem hiding this comment.
It's debatable. I think the main use-case for this property is to help identify processes, then the script name is more useful. Also, when starting a process, scripts can be started the same way as native executables, so it may be unexpected something is actually a script and causes the interpreter to show up here.
There was a problem hiding this comment.
note: this has been returning the script name on Linux so far. I'm adding a test here because that regressed in #37144 (xdg-open is a script).
There was a problem hiding this comment.
Honestly I'd never expect this to return anything else than interpreter name - I think we should match what ps aux does (if it does match then I'm fine with this change), otherwise we're just asking developers to implement their own interop layer to do the right thing. I'd rather add something like ScriptName instead.
There was a problem hiding this comment.
ps aux returns the full command line, which includes both the interpreter and the script name.
gnome system monitor (something like Windows process explorer) has a 'Process Name' column which shows the script names.
The ProcessName is the property that helps a user identify a certain process. Having a bunch of bash and python processes is not that helpful (same for Process.GetProcessesByName).
There was a problem hiding this comment.
Is Windows a bad analog? It presumably returns cmd.exe.
There was a problem hiding this comment.
It presumably returns cmd.exe.
I assume so too.
Is Windows a bad analog?
Unix scripts are more closer to executables than Windows batch files. They are executables (+x bit set) and can be started directly from the Process class (no UseShellExecute required). For a user, there is no difference between a script 'executable' and a native executable. I think it will be unexpected and undesired that the interpreter shows up as the ProcessName.
There was a problem hiding this comment.
Unix scripts are more closer to executables than Windows batch files.
Fair enough. I don't feel strongly either way. @stephentoub thoughts?
There was a problem hiding this comment.
What do other languages / frameworks do here from their equivalent types, e.g. Java, node.js, Go, Rust, etc.?
There was a problem hiding this comment.
java's ProcessHandle provides the following:
- arguments: array of strings (from cmdline)
- command: string (from exe)
- commandLine: string (from cmdline)
rust's sysinfo Process provides:
- name: argv[0] (from cmdline)
- cmd: string (from cmdline)
I don't think there is a standard way of getting this info in node.js or go. It is suggested to read stdout from ps ..., which includes cmdline.
imo we shouldn't change ProcessName unless additional properties are added to Process that make it possible to identify a certain script (and even then we may want to keep the current behavior).
|
@tmds will this resolve https://github.com/dotnet/corefx/issues/37082 as well? I don't see that test failing in CI here, which is promising. |
|
I believe it should resolve https://github.com/dotnet/corefx/issues/35783 as well |
Maybe for the failures on Linux, but not on Windows/OSX. |
|
/azp help |
Supported commands
help:
Get descriptions, examples and documentation about supported commands
Example: help "command_name"
list:
List all pipelines for this repository using a comment.
Example: "list"
run:
Run all pipelines or a specific pipeline for this repository using a comment. Use
this command by itself to trigger all related pipelines, or specify a pipeline
to run.
Example: "run" or "run pipeline_name"
where:
Report back the Azure DevOps orgs that are related to this repository and org
Example: "where"
See additional documentation.
|
|
/azp list |
|
Commenter does not have sufficient privileges for PR 37294 in repo dotnet/corefx |
@stephentoub @wtgodbe can you please trigger an outerloop test run? |
|
@ViktorHofer why did |
|
/azp list |
|
CI/CD Pipelines for this repository: |
|
/azp run corefx-outerloop-linux |
|
/azp run corefx-outerloop-osx |
|
Azure Pipelines failed to run 1 pipeline(s). |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/apz run corefx-outerloop-windows |
|
CI failures are unrelated to the PR. |
|
@wtgodbe look ready? |
|
This looks good to me - @krwq @stephentoub were your concerns resolved? |
|
fine with me |
…dotnet/corefx#37294) * Linux: ProcessName: return script name instead of interpreter program Fixes https://github.com/dotnet/corefx/issues/37198 Regression by dotnet/corefx#37144 * Add ProcessNameMatchesScriptName test * ProcessNameMatchesScriptName: don't run on OSX * LongProcessNamesAreSupported: don't run on Alpine * Remove ActiveIssue attributes Commit migrated from dotnet/corefx@1b6f45c
Fixes https://github.com/dotnet/corefx/issues/37198
Regression by #37144