List Archive
Thread
- Re: UWP builds, (continued)
- Message not available
- Re: UWP builds, Michał Janiszewski (2017/11/30 22:49:14)
- Re: UWP builds, Thomas Klausner (2017/12/04 10:18:10)
- Re: UWP builds, Michał Janiszewski (2017/12/04 10:24:47)
- Re: UWP builds, Michał Janiszewski (2017/12/04 16:33:56)
Message
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
Made by MHonArc.