Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[libimobiledevice/*] Use original upstream #33246

Merged
merged 24 commits into from
Sep 13, 2023

Conversation

xiaozhuai
Copy link
Contributor

@xiaozhuai xiaozhuai commented Aug 18, 2023

  • Changes comply with the maintainer guide
  • SHA512s are updated for each updated download
  • The "supports" clause reflects platforms that may be fixed by this new version
  • Any fixed CI baseline entries are removed from that file.
  • Any patches that are no longer applied are deleted from the port's directory.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is added to each modified port's versions file.

There are more work to be done:

Since this pr is too big enough, I decide to open other PRs to solve them once this pr get merged.


These lines are added into ci.baseline.txt

idevicerestore:arm64-windows=fail
idevicerestore:x64-windows=fail
idevicerestore:x64-windows-static=fail
idevicerestore:x64-windows-static-md=fail
idevicerestore:x86-windows=fail
ideviceinstaller:arm64-windows=fail
ideviceinstaller:x64-windows=fail
ideviceinstaller:x64-windows-static=fail
ideviceinstaller:x64-windows-static-md=fail
ideviceinstaller:x86-windows=fail
usbmuxd:arm64-windows=fail
usbmuxd:x64-windows=fail
usbmuxd:x64-windows-static=fail
usbmuxd:x64-windows-static-md=fail
usbmuxd:x86-windows=fail

idevicerestore, ideviceinstaller and usbmuxd are tool only port.
Before this pr, they only supports windows.
But as the upstream said, it should support other platforms.
Now it supports !uwp & !android & !ios & !xbox, but more patch needed to get windows work, and I would like to leave this job to others who are interested. This is why I add these lines into ci baseline.


Also tested the following triplet:

  1. x64-osx-dynamic
  2. x64-linux-dynamic

@Adela0814 Adela0814 added category:new-port The issue is requesting a new library to be added; consider making a PR! category:port-update The issue is with a library, which is requesting update new revision labels Aug 18, 2023
@xiaozhuai xiaozhuai force-pushed the dev-imobiledevice branch 8 times, most recently from 59ecaa9 to 9539e3c Compare August 21, 2023 08:24
@xiaozhuai
Copy link
Contributor Author

xiaozhuai commented Aug 22, 2023

@Adela0814 @LilyWangL @dg0yt
I'm able to patch original upstream and build these libs on windows with msvc.
The upstream's dllexport must be patched to build dynamic library, that makes the patch very big, as you can see.
What's your opinion if I make these libs static only, so I can make the patches much smaller?
Or maybe point the repo to my fork in https://github.com/orgs/libimobiledevice-vcpkg/repositories
BTW, there are 5 more ports left.
Before statring fix them, I would like to hear your opinions.


And the win32 fork's version is great than the original, which make we can not use the originial upstrem's version. That sucks.

@dg0yt
Copy link
Contributor

dg0yt commented Aug 22, 2023

I appreciate these port being changed to use the original upstream.

You shouldn't move the CMakeLists.txt into patches. It make things more difficult.

And the win32 fork's version is great than the original, which make we can not use the originial upstrem's version. That sucks.

True, this was one of the show-stoppers for me when I looked at these ports in the past.
I wish I had a good answer.
(But I really see this as a grave port bug: Choosing an inofficial fork leading to wrong versioning w.r.t official upstream.)

The upstream's dllexport must be patched to build dynamic library, that makes the patch very big, as you can see.

Did you consider using a DEF file instead?
(My most recent approach to semi-automated generation of DEF files for maintainers is in port minizip, cf. 9321697. After all, I have to do this in vcpkg CI, just accessing the failure logs.)

@xiaozhuai
Copy link
Contributor Author

Did you consider using a DEF file instead?

@dg0yt
Looks like indeed what I need. I'll give it a try.


BTW, there are some tool only ports, idevicerestore, ideviceinstaller, usbmuxd. Should I fix them or remove them?
As far as I know, vcpkg does not accept tool only port.

@dg0yt
Copy link
Contributor

dg0yt commented Aug 22, 2023

BTW, there are some tool only ports, idevicerestore, ideviceinstaller, usbmuxd. Should I fix them or remove them?
As far as I know, vcpkg does not accept tool only port.

Removal is more sensitive than addition.
You may propose this in another PR (unless it becomes a barrier), or annotate the initial post.
For now, they are probably useful as an integration test in vcpkg ci (if not skipped).

@xiaozhuai xiaozhuai force-pushed the dev-imobiledevice branch 15 times, most recently from 3387f1b to e0594b8 Compare August 22, 2023 10:42
@xiaozhuai
Copy link
Contributor Author

@BillyONeal @dan-shaw @dg0yt
I think this PR is stable now. Time to merge : )

@xiaozhuai
Copy link
Contributor Author

xiaozhuai commented Sep 5, 2023

@BillyONeal @dan-shaw @dg0yt
One more thing.
The getopt-win32 port use libimobiledevice-win32/getopt, which is also not right.
I decide to open another pr to fix it after this pr got merged.
But I didn't find where the original upstram is.

@BillyONeal
Copy link
Member

The problem is that for all existing customers of these ports, these changes break them. The fork is suboptimal, absolutely. We would not add ports using the fork today. However, as long as there is no platform support intersection between upstream and the fork, they can both be in the curated registry, and I think we owe not breaking everyone using these bits.

However I'd really like other maintainers to weigh in here.

@BillyONeal BillyONeal added requires:discussion and removed info:reviewed Pull Request changes follow basic guidelines labels Sep 7, 2023
@xiaozhuai
Copy link
Contributor Author

xiaozhuai commented Sep 7, 2023

However, as long as there is no platform support intersection between upstream and the fork, they can both be in the curated registry

@BillyONeal

You mean split idevicerestore and usbmuxd into *-unix and *-win?

@BillyONeal
Copy link
Member

You mean split idevicerestore and usbmuxd into *-unix and *-win?

I think so but before spending a bunch of time doing that let's see what other maintainers think tomorrow.

@BillyONeal
Copy link
Member

@ras0219-msft @JavierMatosD @dan-shaw and I discussed this today. We believe the following:

  1. The name that corresponds with upstream must be changed to point at that upstream. (Which this PR does)
  2. Within the port that normally points to upstream, we do not want to fetch a fork with different, possibly out of date contents. (That is, if we use the fork, there must be a metaport that chooses)
  3. We could relax (2) if the fork is 'morally indistinguishable' from the original and tracks very quickly with identical versioning etc. (That is not the case here)

Other observations:

  • How large are the changes in the fork to work with Windows? Is it just .def files or something else?
  • We looked at telemetry for these ports and they are installed extremely rarely. As a result we are less concerned about introducing breaking changes to those customers. For instance, this was installed .02% as frequently as the most frequently installed port.

Agreed upon outcome:

  • We should give @qmfrederik (and any other contributors we can find if possible) ~a week to respond before proceeding. If they reply that they are heavily using the fork, we should add libimobiledevice-win32 which is morally identical to the old ports for them to use. Note that the set intersection of the -win32 ports and the real upstream ports is empty so they can both be in the registry.
  • Otherwise, what's in this PR is good to go. ✅

=====================

Other things we discussed and discarded:

  • Change these ports (for example libimobiledevice) to point at the real upstream, but when a platform upstream can't support is used, they become empty and depend on forked ports (for example libimobiledevice-win32)
  • Do exactly what's in this pr, but add libimobiledevice-win32 which points at the forks. There's no metaport here, so folks who want the status quo have to change their declared dependencies. The supports expression intersections will be the empty set, so they can both be in the curated registry.
  • Do exactly what's in this pr, but rename to libmobibledevice-original (name to be bikeshed) and deindex the original names currently used by the fork.

@BillyONeal BillyONeal marked this pull request as draft September 7, 2023 21:21
@qmfrederik
Copy link
Contributor

This is the right thing to do. My oldest patches for adding Visual Studio support to libimobiledevice date back to 2015 (libimobiledevice/libplist#76, libimobiledevice/libusbmuxd#28, libimobiledevice/libimobiledevice#196), when CMake was relatively new, Visual Studio had no editor support for it, and the clang front-end for the msvc compiler wasn't a thing yet.

I've been wanting to redo libimobiledevice-win32 as a CMake project for a long time, and it's great to that someone else finally got round to doing this. Thanks, @xiaozhuai!

Getting usbmuxd to work on Windows is non-trivial; the only way I found to make it work is to use libusb-win32 and use a libusb driver for your iOS device. In virtually all cases, it's easier to use the Apple Mobile Device service instead.

@BillyONeal BillyONeal marked this pull request as ready for review September 13, 2023 23:51
@BillyONeal
Copy link
Member

@qmfrederik Thanks for confirming!

@BillyONeal BillyONeal merged commit 29151e1 into microsoft:master Sep 13, 2023
15 checks passed
@BillyONeal
Copy link
Member

@xiaozhuai Thanks for the PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:new-port The issue is requesting a new library to be added; consider making a PR! category:port-update The issue is with a library, which is requesting update new revision
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants