-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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.
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
Copy execute permissions for apphost for non-Windows #1338
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 'if' should not be necessary, although keeping it will prevent the (unnecessary) file copy for Windows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My suggestion would be to keep the code consistent - there is no overhead scenario in this operation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @steveharter - I concur with you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. Thanks.
Rebase and retarget PR to release/2.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above
@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. |
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 denied
error 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 myConsoleApp
Risk
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.