Copy execute permissions for apphost for non-Windows #1338
Conversation
| @@ -59,7 +60,24 @@ protected override void ExecuteCore() | |||
| { | |||
| Directory.CreateDirectory(destinationDirectory); | |||
| } | |||
| File.WriteAllBytes(ModifiedAppHostPath, array); | |||
|
|
|||
| if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) | |||
There was a problem hiding this comment.
Why do we need this "if" check. The "else" section would work for Windows as well and keep the implementation consistent.
There was a problem hiding this comment.
The 'if' should not be necessary, although keeping it will prevent the (unnecessary) file copy for Windows.
There was a problem hiding this comment.
My suggestion would be to keep the code consistent - there is no overhead scenario in this operation.
There was a problem hiding this comment.
I would keep the code consistent between platforms too. I don't know what you mean by "no overhead", though. We're copying the whole file only to replace its contents completely. We can fix this by finding the index of the marker, seeking to the right position and overwriting it.
On a related note, I pointed out in the original code review that reading the whole file in to a managed byte array was wasteful, but that was never addressed.
There was a problem hiding this comment.
OK I will remove the 'if', and log a future issue for the performance aspect so we only change the marker and not the whole file. The apphost file is ~77K so fairly small, but still we should do the right thing here.
There was a problem hiding this comment.
Thanks @steveharter - I concur with you.
|
Rebase and retarget PR to release/2.0 |
@nguerrera I was assuming we want to go into master first, then release/2.0. If not, are we planning on doing an RI later? |
We will RI 2.0 -> master (work is in progress to make that automatic and often). We should avoid dual checkins to master and release where possible. |
steveharter
changed the title
|
This needs to be rebased to just your changes over 2.0. As is, it is merging changes for 2.1 from master. |
|
Please update the PR description with the template as seen in #1326 (comment) Once we have that, we can tag Matt for approval and merge if approved. |
|
Also, is the perf bug for future filed? |
|
@nguerrera just created perf issue #1339 |
|
@nguerrera should I also send out a shiproom mail? |
|
@steveharter No, this is sufficient for a change affecting only 2.0 SDK. |
|
@MattGertz for approval |
|
CC @leecow as well to keep this in his purview as well. |
@dotnet/dotnet-cli
@MattGertz for approval
Customer scenario
The executable generated from
dotnet publish -r{RID}doesn't have the "x" (execute) permission for non-Windows and thus when attempting to execute, aPermission deniederror is returned from the OS. This means the standalone + RID scenario is broken for non-Windows.The fix is to do a File.Copy from the source apphost to the destination (modified apphost) so the execute permission will be copied with that. Then the file is updated to have the proper contents.
Bugs this fixes:
#1331
Workarounds, if any
The developer would need to do a manual chmod, such as
chmod +x myConsoleAppRisk
Low.
Performance impact
Minor; extra copy of ~77K apphost file. Created issue #1339 to address in future.
Is this a regression from a previous update?
No
Root cause analysis:
The underlying issue has been masked for a long time by a NuGet issue which didn't restore executable files with the "x" permission.
How was the bug found?
Going through a core developer scenario on non-Windows.