Re: UWP builds

[Michał Janiszewski <janisozaur%gmail.com@localhost>]

On Mon, Dec 4, 2017 at 11:18 AM, Thomas Klausner <tk%giga.or.at@localhost> 
wrote:
> On Thu, Nov 30, 2017 at 11:48:50PM +0100, Michał Janiszewski wrote:
>> I have just noticed I missent this email and it never reached the mailing 
>> list.
>> It only strengthens my point about mail reviews vs pull requests and
>> true to my words I have submitted a pull request:
>> https://github.com/nih-at/libzip/pull/16
>
> Thanks. I've added review comments.

Are you sure you have posted the review? I see no comments yet.

>> It already contains your suggestions. I can come up with a sample CI
>> job proving UWP builds with those changes, should you need that.
>
> If you could get appveyor working on github, that'd be really
> appreciated. (Or if there is other automation that can be
> automatically run from github.)

Sure, I'll work on getting vcpkg proof. I'll let you know when I will
have done it.

>> Please consider merging it and if you do, can I ask for creating
>> another patch release? That way we could drop some of the patches
>> needed in vcpkg.
>
> Once we have handled the remaining issues we can surely make a patch
> release.

Sounds great, thanks.

> Cheers,
>  Thomas
>
>> Cheers,
>> Michał.
>>
>> On Tue, Nov 28, 2017 at 5:36 PM, Michał Janiszewski
>> <janisozaur%gmail.com@localhost> wrote:
>> > On Tue, Nov 28, 2017 at 5:19 PM, Thomas Klausner <tk%giga.or.at@localhost> 
>> > wrote:
>> >>
>> >> On Fri, Nov 24, 2017 at 12:30:02PM +0100, Michał Janiszewski wrote:
>> >> > Please consider applying this patch which allows building for UWP.
>> >>
>> >> Is this the complete patch, i.e. I can forget about the one that moved
>> >> around zipint.h etc?
>> >
>> > Yes, the previous patch was prepared for libzip 1.2.0, the
>> > then-current vcpkg version.
>> > Since then I have updated vcpkg to 1.3.2 and based my work on that.
>> >
>> >> > For your convenience, the patch is also available at
>> >> > https://hastebin.com/diwavuqubo.diff
>> >> > This only addresses building the library itself and won't support
>> >> > encryption, as seeding entropy is turned off, until someone who knows 
>> >> > the
>> >> > platform better can implement it.
>> >>
>> >> Hm, that would be nice to have before the merge.
>> >
>> > While I agree, I will leave this exercise to someone who will know
>> > where to seed the entropy from.
>> >
>> >> > +if(CMAKE_SYSTEM_NAME MATCHES WindowsPhone OR CMAKE_SYSTEM_NAME
>> >> > MATCHES WindowsStore)
>> >> > +  ADD_DEFINITIONS(-DWINRT)
>> >>
>> >> What does WINRT stand for?
>> >
>> > I believe it stands for Windows RunTime, the limited API that used to
>> > form the base of "Windows on ARM" when that previously tried to be a
>> > thing.
>> > This acronym is still in use across some products and is once again
>> > being used for "Windows on ARM (v2)"
>> >
>> >> > diff --git a/lib/zip_source_win32a.c b/lib/zip_source_win32a.c
>> >> > index e343a70..625784f 100644
>> >> > --- a/lib/zip_source_win32a.c
>> >> > +++ b/lib/zip_source_win32a.c
>> >> > @@ -31,7 +31,7 @@ OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
>> >> > SOFTWARE, EVEN
>> >> >  IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>> >> >  */
>> >> >
>> >> > -
>> >> > +#ifndef WINRT
>> >> >  #include <stdio.h>
>> >> >
>> >> >  #include "zipint.h"
>> >> > @@ -122,3 +122,5 @@ _win32_remove_a(const void *fname)
>> >> >      DeleteFileA((const char *)fname);
>> >> >      return 0;
>> >> >  }
>> >> > +
>> >> > +#endif //WINRT
>> >>
>> >> Is there no ANSI interface on UWP?
>> >>
>> >> Instead of commenting out the whole file, it would be better to
>> >> disable building it in lib/CMakeLists.txt.
>> >
>> > Ok, I can try amending the patch to exclude this file.
>> >
>> >> > diff --git a/lib/zip_source_win32handle.c b/lib/zip_source_win32handle.c
>> >> > index 7fe003d..f7ac12e 100644
>> >> > --- a/lib/zip_source_win32handle.c
>> >> > +++ b/lib/zip_source_win32handle.c
>> >> > @@ -420,12 +420,15 @@
>> >> > _win32_create_temp_file(_zip_source_win32_read_file_t *ctx)
>> >> >      int i;
>> >> >      HANDLE th = INVALID_HANDLE_VALUE;
>> >> >      void *temp = NULL;
>> >> > -    SECURITY_INFORMATION si;
>> >> > -    SECURITY_ATTRIBUTES sa;
>> >> >      PSECURITY_DESCRIPTOR psd = NULL;
>> >> >      PSECURITY_ATTRIBUTES psa = NULL;
>> >> > +
>> >> > +
>> >> > +#ifndef WINRT
>> >> >      DWORD len;
>> >> >      BOOL success;
>> >> > +    SECURITY_ATTRIBUTES sa;
>> >> > +    SECURITY_INFORMATION si;
>> >> >
>> >> >      /*
>> >> >      Read the DACL from the original file, so we can copy it to the 
>> >> > temp file.
>> >> > @@ -452,6 +455,10 @@ 
>> >> > _win32_create_temp_file(_zip_source_win32_read_file_t *ctx)
>> >> >      }
>> >> >
>> >> >      value = GetTickCount();
>> >> > +#else
>> >> > +    value = (zip_uint32_t)GetTickCount64();
>> >> > +#endif
>> >> > +
>> >> >      for (i = 0; i < 1024 && th == INVALID_HANDLE_VALUE; i++) {
>> >> >       th = ctx->ops->op_create_temp(ctx, &temp, value + i, psa);
>> >> >       if (th == INVALID_HANDLE_VALUE && GetLastError() != 
>> >> > ERROR_FILE_EXISTS)
>> >>
>> >> This code is just a workaround anyway... I can imagine that there is a
>> >> better interface for temporary files on UWP -- do you know one?
>> >
>> > Sorry, I don't know. Files are generally frowned upon, based on how
>> > convoluted the UWP API is.
>> > All the UWP examples try forcing some odd and proprietary C++
>> > extension on users.
>> >
>> >> > diff --git a/lib/zip_source_win32w.c b/lib/zip_source_win32w.c
>> >> > index d8b8f86..092c044 100644
>> >> > --- a/lib/zip_source_win32w.c
>> >> > +++ b/lib/zip_source_win32w.c
>> >> > @@ -83,7 +83,19 @@ _win32_strdup_w(const void *str)
>> >> >  static HANDLE
>> >> >  _win32_open_w(_zip_source_win32_read_file_t *ctx)
>> >> >  {
>> >> > +#ifdef WINRT
>> >> > +    CREATEFILE2_EXTENDED_PARAMETERS extParams = { 0 };
>> >> > +    extParams.dwFileAttributes = FILE_ATTRIBUTE_NORMAL;
>> >> > +    extParams.dwFileFlags = FILE_FLAG_RANDOM_ACCESS;
>> >> > +    extParams.dwSecurityQosFlags = SECURITY_ANONYMOUS;
>> >> > +    extParams.dwSize = sizeof(extParams);
>> >> > +    extParams.hTemplateFile = NULL;
>> >> > +    extParams.lpSecurityAttributes = NULL;
>> >> > +
>> >> > +    return CreateFile2(ctx->fname, GENERIC_READ, FILE_SHARE_READ |
>> >> > FILE_SHARE_WRITE, OPEN_EXISTING, &extParams);
>> >> > +#else
>> >> >      return CreateFileW(ctx->fname, GENERIC_READ, FILE_SHARE_READ |
>> >> > FILE_SHARE_WRITE, NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
>> >> > +#endif
>> >> >  }
>> >> >
>> >> >
>> >> > @@ -103,7 +115,19 @@
>> >> > _win32_create_temp_w(_zip_source_win32_read_file_t *ctx, void **temp,
>> >> > zip_uint32
>> >> >       return INVALID_HANDLE_VALUE;
>> >> >      }
>> >> >
>> >> > +#ifdef WINRT
>> >> > +    CREATEFILE2_EXTENDED_PARAMETERS extParams = { 0 };
>> >> > +    extParams.dwFileAttributes = FILE_ATTRIBUTE_NORMAL |
>> >> > FILE_ATTRIBUTE_TEMPORARY;
>> >> > +    extParams.dwFileFlags = FILE_FLAG_RANDOM_ACCESS;
>> >> > +    extParams.dwSecurityQosFlags = SECURITY_ANONYMOUS;
>> >> > +    extParams.dwSize = sizeof(extParams);
>> >> > +    extParams.hTemplateFile = NULL;
>> >> > +    extParams.lpSecurityAttributes = NULL;
>> >> > +
>> >> > +    return CreateFile2((const wchar_t *)*temp, GENERIC_READ |
>> >> > GENERIC_WRITE, FILE_SHARE_READ, CREATE_NEW, &extParams);
>> >> > +#else
>> >> >      return CreateFileW((const wchar_t *)*temp, GENERIC_READ |
>> >> > GENERIC_WRITE, FILE_SHARE_READ, sa, CREATE_NEW, FILE_ATTRIBUTE_NORMAL
>> >> > | FILE_ATTRIBUTE_TEMPORARY, NULL);
>> >> > +#endif
>> >> >  }
>> >>
>> >> The file is so short I wonder if a separate zip_source_win32uwp.c (or
>> >> so) instead would be better.
>> >
>> > I'll see what I can do about it.
>> >
>> >> > diff --git a/lib/zipwin32.h b/lib/zipwin32.h
>> >> > index 60f8d0c..701a34c 100644
>> >> > --- a/lib/zipwin32.h
>> >> > +++ b/lib/zipwin32.h
>> >> > @@ -35,7 +35,10 @@
>> >> >  */
>> >> >
>> >> >  /* 0x0501 => Windows XP; needs to be at least this value because of
>> >> > GetFileSizeEx */
>> >> > +#ifndef WINRT
>> >> >  #define _WIN32_WINNT 0x0501
>> >> > +#endif
>> >> > +
>> >> >  #include <windows.h>
>> >> >
>> >> >  /* context for Win32 source */
>> >>
>> >> Why is that needed?
>> >
>> > I can only imagine it clashes with some UWP defines.
>> >
>> >> Cheers,
>> >>  Thomas
>> >
>> > One final note - would you accept this change to be submitted as a
>> > pull request to https://github.com/nih-at/libzip ? The mail review
>> > process is very high maintenance for me. You can still download raw
>> > patches from github that you can later apply in your repo.
>> >
>> > Cheers,
>> > Michał.
>> >
>> >
>> > --
>> > Michal Janiszewski
>>
>>
>>
>> --
>> Michal Janiszewski
>>



-- 
Michal Janiszewski