Skip to content
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

Merged
merged 1 commit into from
Jun 13, 2017
Merged

Copy execute permissions for apphost for non-Windows #1338

merged 1 commit into from
Jun 13, 2017

Conversation

steveharter
Copy link
Member

@steveharter steveharter commented Jun 13, 2017

@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, a Permission 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.

@@ -59,7 +60,24 @@ protected override void ExecuteCore()
{
Directory.CreateDirectory(destinationDirectory);
}
File.WriteAllBytes(ModifiedAppHostPath, array);

if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good. Thanks.

@nguerrera
Copy link
Contributor

Rebase and retarget PR to release/2.0

Copy link
Contributor

@nguerrera nguerrera left a comment

Choose a reason for hiding this comment

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

See above

@steveharter
Copy link
Member Author

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?

@nguerrera
Copy link
Contributor

@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 steveharter changed the base branch from master to release/2.0.0 June 13, 2017 18:24
@steveharter steveharter changed the title [NoMerge] Copy execute permissions for apphost for non-Windows Copy execute permissions for apphost for non-Windows Jun 13, 2017
@nguerrera
Copy link
Contributor

This needs to be rebased to just your changes over 2.0. As is, it is merging changes for 2.1 from master.

@steveharter
Copy link
Member Author

steveharter commented Jun 13, 2017 via email

@nguerrera
Copy link
Contributor

nguerrera commented Jun 13, 2017

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.

@nguerrera
Copy link
Contributor

Also, is the perf bug for future filed?

@steveharter
Copy link
Member Author

@nguerrera just created perf issue #1339

@steveharter
Copy link
Member Author

@nguerrera should I also send out a shiproom mail?

@nguerrera
Copy link
Contributor

@steveharter No, this is sufficient for a change affecting only 2.0 SDK.

@nguerrera
Copy link
Contributor

@MattGertz for approval

@gkhanna79
Copy link
Member

CC @leecow as well to keep this in his purview as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants