List Archive

Thread

Thread Index

Message

From: Michał Janiszewski <janisozaur%gmail.com@localhost>
To: Thomas Klausner <tk%giga.or.at@localhost>
Subject: Re: UWP builds
Date: Thu, 30 Nov 2017 23:48:50 +0100

Hi,
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
It already contains your suggestions. I can come up with a sample CI
job proving UWP builds with those changes, should you need that.
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.
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

Made by MHonArc.