[winsparkle] inital port #17563

daschuer · 2021-04-28T21:56:55Z

This PR adds winsparkle to the ports

Fixes #16607

Which triplets are supported/not supported? Have you updated the CI baseline?

winsparkle:arm-uwp=fail
winsparkle:x64-linux=fail
winsparkle:x64-osx=fail
winsparkle:x64-uwp=fail

Does your PR follow the maintainer guide?

Yes

If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

Yes

If you are still working on the PR, open it as a Draft: https://github.blog/2019-02-14-introducing-draft-pull-requests/

ghost · 2021-04-28T21:57:07Z

All CLA requirements met.

NancyLi1013 · 2021-04-29T02:28:05Z

Hi @daschuer

Could you please sign CLA first?

ports/winsparkle/portfile.cmake

NancyLi1013 · 2021-04-29T02:30:30Z

ports/winsparkle/portfile.cmake

+vcpkg_extract_source_archive_ex(
+    OUT_SOURCE_PATH SOURCE_PATH
+    ARCHIVE ${ARCHIVE}
+)


Why do you not use vcpkg_from_github() to download source codes?

Because the repo contains submodules and the package I have picked here has the submodules included.
I have tried to use vcpkg_from_github calls the compose the same on the fly, but I failed.

ports/winsparkle/vcpkg.json

scripts/ci.baseline.txt

ports/winsparkle/portfile.cmake

ports/winsparkle/vcpkg.json

NancyLi1013 · 2021-04-29T02:42:45Z

Please run vcpkg format-manifest ./ports/winsparkle/vcpkg.json to format the json file.

NancyLi1013 · 2021-09-29T07:41:17Z

Hi @daschuer
Is work still being done for this PR?

daschuer · 2021-09-29T07:53:06Z

I consider this as done so far.
It is missing the evidence that it is working in a productive environment, because we have not yet integrated it into Mixxx. I am afraid this will take us some time because we are currently fighting other issues.

NancyLi1013 · 2021-09-29T08:25:13Z

Thanks for your quick response. @daschuer

Looking forward to new progress of solving the problem in this PR.

Cheney-W · 2021-12-23T11:57:18Z

@daschuer Is work still being done for this PR?

daschuer · 2021-12-23T18:34:29Z

The status is unchanged since my last post.
Do you consider to use this port?

Cheney-W · 2021-12-24T02:51:57Z

Thanks for your reply!
No, I don't use this port, I'm just asking about the current status of this PR based on the PR review process.
Also, If you don't have time to do this PR at the moment, I think we can close it and reopen it when you have time to do it. What do you think?

daschuer · 2021-12-24T11:46:32Z

My idea was to leave it open, that it is visible.
Without it, there is a risk that someone else's repeat my work.
Alternative I can polish it for passing the CI and merge without a using project. If one discovers that it is not working in his environment, we have at least a good start.

Cheney-W · 2021-12-27T01:46:59Z

LGTM.

Cheney-W · 2021-12-28T07:11:29Z

ports/winsparkle/portfile.cmake

@@ -0,0 +1,44 @@
+vcpkg_fail_port_install(ON_TARGET "osx" "linux" "uwp")


This sentence could be remove because the vcpkg.json has include the "supports" item.

Co-authored-by: NancyLi1013 <46708020+NancyLi1013@users.noreply.github.com>

Cheney-W · 2021-12-31T06:24:57Z

/azp run

azure-pipelines · 2021-12-31T06:25:07Z

Azure Pipelines successfully started running 1 pipeline(s).

strega-nil-ms · 2022-01-07T22:44:59Z

We need to patch out the usage of submodules here; building openssl and everything instead of using openssl from vcpkg is not great. Can you take a look at this @daschuer? You should be able to patch out all the 3rdparty references, and then pass USE_VCPKG_INTEGRATION to vcpkg_build_msbuild. You can also then use vcpkg_from_github without worrying about submodules

daschuer · 2022-01-09T22:03:11Z

Unfortunately I don't know how to make the changes in the msbuild project. There is also a broken cmake build system with a pending PR vslavik/winsparkle#230
Once this is merged a cmake based solution would be easy for me.
However I am unsure if it a good idea to do this without upstream support. This will put the maintenance burden on us to maintain and extra build system which is not tested by upstream.

I have filed an issue to here @vslavik opinion:
vslavik/winsparkle#238

Cheney-W · 2022-01-14T11:14:49Z

@daschuer
I'll get to it, but it may take some time, in the meantime, can we convert this PR to draft?

daschuer · 2022-01-14T11:34:24Z

Yes sure, thank you.

vslavik · 2022-01-15T17:55:59Z

However I am unsure if it a good idea to do this without upstream support. This will put the maintenance burden on us to maintain and extra build system which is not tested by upstream.

…as well as additional burden on upstream who'd now have to deal with issues caused by the non-standard build…

It is an explicit design goal for WinSparkle to be a single self-contained DLL with no external dependencies (to the point that it even links to static CRT!). This matters for e.g. in-app delta updates or re-launching the app after update (which, OK, aren't implemented yet) when WinSparkle code requires special handling to keep running. As long as that means just a single WinSparkle.dll and nothing else, it is manageable.

As upstream, I'd really appreciate it if something as high-profile as Vcpkg didn't provide non-standard builds.

daschuer · 2022-01-18T07:49:56Z

OK, that means vcpkg should produce always a dynamic linked DLL, even If the whole application is build static and this DLL shall link to the static version of the depended libraries, right?

I am afraid afraid vcpkg is not prepared for that right now. There is no way to identify the matching dynamic triplet in a static build and wise versa. There is also no general solution in sight, so there is no point of hacking around the upstream build system AND the vcpkg dependency tree.

For me the private build of dependency is a good solution and the current state is ready to be merged.

dg0yt · 2022-01-18T08:29:19Z

OK, that means vcpkg should produce always a dynamic linked DLL, even If the whole application is build static and this DLL shall link to the static version of the depended libraries, right?

I am afraid afraid vcpkg is not prepared for that right now.

IIUC this is done by

vcpkg_check_linkage(ONLY_DYNAMIC_LIBRARY)

daschuer · 2022-01-18T11:33:29Z

vcpkg_check_linkage(ONLY_DYNAMIC_LIBRARY)

Does not work, because all other packages will be created with the same triplet. But we want WinSparkles to be dll, statically linked to it's dependencies.

dg0yt · 2022-01-18T11:39:05Z

Does not work, because all other packages will be created with the same triplet.

Pardon? It will create a dll in a triplet where all dependencies only exist as static libs.

daschuer · 2022-01-18T12:06:28Z

Yes, but only if all other libraries build are static.
The requirement here is to always link the depending libraries statically and use CRT.

So the issue is not about the port itself, it is the question how to link the dependencies statically. The current solution is to build them together with the port.

The alternative would be a kind of package dependency that includes a triplet, depending on the selected main triplet and a we need to hack around the upstream build environment ant pull the related maintenance to vcpkg.

strega-nil-ms · 2022-01-20T19:53:49Z

So given the specific case here, where the DLL is completely encapsulated - there are no libraries that both the DLL and the consumer depend on that can step on each other's toes, and the author also told us not to do anything else besides this, contrary to our normal de-vendoring policies - it seems to me that there are no benefits to building this library, rather than downloading the pre-built binary. There aren't any ABI concerns that building the source would solve, in this case.

@daschuer, would you mind making this change? (downloading WinSparkle-0.7.0.zip instead of WinSparkle-0.7.0-src.zip, and then just manually copying files over instead of building).

Cheney-W · 2022-03-03T06:26:13Z

Can this pr be closed because #23194 has been merged?

daschuer · 2022-03-03T06:45:56Z

Yes, I have kept this open for reference only.

daschuer marked this pull request as draft April 28, 2021 22:04

JonLiu1993 assigned NancyLi1013 Apr 29, 2021

NancyLi1013 added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Apr 29, 2021

NancyLi1013 suggested changes Apr 29, 2021

View reviewed changes

NancyLi1013 added the requires:author-response label Apr 29, 2021

NancyLi1013 removed the requires:author-response label May 8, 2021

PhoebeHui assigned Cheney-W and unassigned NancyLi1013 Nov 22, 2021

daschuer force-pushed the winsparkle branch from 5cae679 to c381579 Compare December 27, 2021 23:15

github-actions bot approved these changes Dec 27, 2021

View reviewed changes

Cheney-W reviewed Dec 28, 2021

View reviewed changes

Cheney-W added the requires:author-response label Dec 28, 2021

daschuer and others added 2 commits December 28, 2021 11:46

[winsparkle] inital port

abcd01b

Co-authored-by: NancyLi1013 <46708020+NancyLi1013@users.noreply.github.com>

[winsparkle] update baseline.json

d191f0f

daschuer force-pushed the winsparkle branch from c381579 to 54251ea Compare December 28, 2021 10:57

github-actions bot approved these changes Dec 28, 2021

View reviewed changes

daschuer and others added 3 commits December 28, 2021 19:13

[winsparkle] use supports filed in vcpkg.json

13f2734

Co-authored-by: NancyLi1013 <46708020+NancyLi1013@users.noreply.github.com>

[winsparkle] use ${PORT}

f429cc2

Co-authored-by: NancyLi1013 <46708020+NancyLi1013@users.noreply.github.com>

[winsparkle] format vcpkg.json

822d56e

daschuer marked this pull request as ready for review December 31, 2021 11:18

Cheney-W added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Jan 7, 2022

strega-nil-ms added requires:author-response and removed info:reviewed Pull Request changes follow basic guidelines labels Jan 7, 2022

daschuer mentioned this pull request Jan 9, 2022

vcpkg integration vslavik/winsparkle#238

Open

daschuer marked this pull request as draft January 14, 2022 11:34

Cheney-W removed the requires:author-response label Jan 14, 2022

daschuer requested a review from Cheney-W January 18, 2022 07:50

daschuer marked this pull request as ready for review January 18, 2022 07:51

Cheney-W added the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Jan 19, 2022

strega-nil-ms added requires:author-response and removed requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. labels Jan 20, 2022

daschuer mentioned this pull request Feb 20, 2022

[winsparkle] Intitial commit with binary copy #23194

Merged

daschuer closed this Mar 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[winsparkle] inital port #17563

[winsparkle] inital port #17563

daschuer commented Apr 28, 2021

ghost commented Apr 28, 2021 •

edited by ghost

Loading

NancyLi1013 commented Apr 29, 2021

NancyLi1013 Apr 29, 2021

daschuer Apr 29, 2021

NancyLi1013 commented Apr 29, 2021

NancyLi1013 commented Sep 29, 2021

daschuer commented Sep 29, 2021

NancyLi1013 commented Sep 29, 2021

Cheney-W commented Dec 23, 2021

daschuer commented Dec 23, 2021

Cheney-W commented Dec 24, 2021

daschuer commented Dec 24, 2021

Cheney-W commented Dec 27, 2021

Cheney-W Dec 28, 2021

Cheney-W commented Dec 31, 2021

azure-pipelines bot commented Dec 31, 2021

strega-nil-ms commented Jan 7, 2022 •

edited

Loading

daschuer commented Jan 9, 2022

Cheney-W commented Jan 14, 2022

daschuer commented Jan 14, 2022

vslavik commented Jan 15, 2022

daschuer commented Jan 18, 2022

dg0yt commented Jan 18, 2022

daschuer commented Jan 18, 2022

dg0yt commented Jan 18, 2022

daschuer commented Jan 18, 2022

strega-nil-ms commented Jan 20, 2022

Cheney-W commented Mar 3, 2022

daschuer commented Mar 3, 2022

		@@ -0,0 +1,44 @@
		vcpkg_fail_port_install(ON_TARGET "osx" "linux" "uwp")

[winsparkle] inital port #17563

[winsparkle] inital port #17563

Conversation

daschuer commented Apr 28, 2021

Which triplets are supported/not supported? Have you updated the CI baseline?

Does your PR follow the maintainer guide?

If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

ghost commented Apr 28, 2021 • edited by ghost Loading

NancyLi1013 commented Apr 29, 2021

NancyLi1013 Apr 29, 2021

Choose a reason for hiding this comment

daschuer Apr 29, 2021

Choose a reason for hiding this comment

NancyLi1013 commented Apr 29, 2021

NancyLi1013 commented Sep 29, 2021

daschuer commented Sep 29, 2021

NancyLi1013 commented Sep 29, 2021

Cheney-W commented Dec 23, 2021

daschuer commented Dec 23, 2021

Cheney-W commented Dec 24, 2021

daschuer commented Dec 24, 2021

Cheney-W commented Dec 27, 2021

Cheney-W Dec 28, 2021

Choose a reason for hiding this comment

Cheney-W commented Dec 31, 2021

azure-pipelines bot commented Dec 31, 2021

strega-nil-ms commented Jan 7, 2022 • edited Loading

daschuer commented Jan 9, 2022

Cheney-W commented Jan 14, 2022

daschuer commented Jan 14, 2022

vslavik commented Jan 15, 2022

daschuer commented Jan 18, 2022

dg0yt commented Jan 18, 2022

daschuer commented Jan 18, 2022

dg0yt commented Jan 18, 2022

daschuer commented Jan 18, 2022

strega-nil-ms commented Jan 20, 2022

Cheney-W commented Mar 3, 2022

daschuer commented Mar 3, 2022

If you have added/updated a port: Have you run `./vcpkg x-add-version --all` and committed the result?

ghost commented Apr 28, 2021 •

edited by ghost

Loading

strega-nil-ms commented Jan 7, 2022 •

edited

Loading