Set execute permission when extracting files. #1346
Conversation
| @@ -85,5 +87,23 @@ public static void UpdateFileTimeFromEntry(this ZipArchiveEntry entry, string fi | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| public static void UpdateFilePermissionsFromEntry(this ZipArchiveEntry entry, string fileFullPath, ILogger logger) | |||
There was a problem hiding this comment.
Does this need to be public?
There was a problem hiding this comment.
I gave it the same visibility as UpdateFileTimeFromEntry. Should I make it internal or leave it public?
There was a problem hiding this comment.
It's not really my call. @emgarten?
There was a problem hiding this comment.
There are a lot of public APIs like this in the nuget libraries so I think this is fine.
| // The default on .NET Core 2.x has changed to 666. | ||
| // To avoid breaking executable files in existing packages (which don't have the x-bit set) | ||
| // we force the .NET Core 1.x default permissions. | ||
| chmod(fileFullPath, 0x1f6); // 0766 |
There was a problem hiding this comment.
Should all files get this or should it be limited to .dll extensions only? Was this previously being set for everything?
There was a problem hiding this comment.
This was set for all files. Not having the execute bit breaks native executables and scripts.
There was a problem hiding this comment.
Actually .dll .net libraries don't need the 'x' flag at all since the OS doesn't execute them. What is needed is things like apphost which doesn't have an extension and somescript.sh which does have an extension. Honestly I don't think we should be doing extension based checking, it would be very hard to get it right.
Was this previously being set for everything?
In 2.0, the code now lives here: https://github.com/dotnet/coreclr/blob/532c45a27aa2f804ed43448ef1666beeef6c96ae/src/mscorlib/shared/System/IO/FileStream.Unix.cs#L50-L57
|
Looks great to me, thanks for the PR @tmds! I'll run this on the Linux and OSX CIs to verify there are no issues and then I'll merge this into dev for NuGet 4.3.0. |
Fixes NuGet/Home#4424