Re: UWP builds

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

Hi,
I have replied to your review, please let me know how you want it handled.

In the meantime, I have managed to create a appveyor test job that
proves UWP configuration builds, please see
https://ci.appveyor.com/project/janisozaur/vcpkg/build/1.0.7
The job is marked as failed due to timeout after 60 minutes. The
configuration process takes an eternity with MSVC, due to your
checking of _every_single_type_or_function_ in CMakeLists.txt, it
takes 7 minutes and 30 seconds to configure on AppVeyor and then 9
seconds to actually build (you can hover over each line in build
output on appveyor to find when it happened).

I hope to come up with some patch to alleviate this situation a bit,
time allowing.

On Mon, Dec 4, 2017 at 11:24 AM, Michał Janiszewski
<janisozaur%gmail.com@localhost> wrote:
> 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



-- 
Michal Janiszewski