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-drvs: update all with major rebuilds #342961

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

Frontear
Copy link
Member

@Frontear Frontear commented Sep 19, 2024

Description of changes

Blocked by #349567, as a derivation here relies on said PR to be merged.

Updates derivations from https://github.com/libimobiledevice that cause mass rebuilds. These were accompanied by cleanup that is shared between both PRs.

Cleanup includes:

  • Adding unstableGitUpdater to all derivations (see explanation by @restoration578 here) (considering removal, see my latest comment)
  • Marking meta.license more consistently to upstream
  • Using the GitHub repo as meta.homepage
  • Using platforms.unix for all derivations in meta.platforms
  • Moving all packages to pkgs/by-name
  • Adding myself as maintainer (considering removal, see my latest comment)

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@ofborg ofborg bot added 8.has: clean-up 8.has: package (new) This PR adds a new package labels Sep 19, 2024
@Frontear Frontear marked this pull request as ready for review September 19, 2024 19:28
@restoration578
Copy link

Let's wait a bit before merging this for things to settle down, since some development has been going on these past days out of nowhere, probably because of the sequoia/ios18 release and we'll want to get the latest commit with these changes, so it works on the newer devices/systems as well.

pkgs/by-name/li/libideviceactivation/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/li/libideviceactivation/package.nix Outdated Show resolved Hide resolved
@Frontear
Copy link
Member Author

Let's wait a bit before merging this for things to settle down, since some development has been going on these past days out of nowhere, probably because of the sequoia/ios18 release and we'll want to get the latest commit with these changes, so it works on the newer devices/systems as well.

Blocked. Waiting about a week or so before merging in.

@ofborg ofborg bot requested a review from clebs September 23, 2024 13:40
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Sep 27, 2024
@Frontear
Copy link
Member Author

Frontear commented Sep 27, 2024

EDIT: Also noticing @ofborg is failing to evaluate given this missed the last staging cycle. What's the acceptable fix here? Leaving it as is or performing a rebase?

24.11 will soon be cut for release: #344920

It's imperative this gets through before then to avoid leaving this PR dangling for too long. I will check upstream activity and if it has died down, I'm unblocking this PR and bumping ALL sources to their current latest.

@Frontear Frontear marked this pull request as draft September 28, 2024 01:28
@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Sep 28, 2024
@Frontear Frontear marked this pull request as ready for review September 28, 2024 02:37
@Frontear Frontear force-pushed the update-libimobiledevice-drvs branch 2 times, most recently from 2ea66a3 to a3e6a1e Compare September 28, 2024 03:19
@RossComputerGuy
Copy link
Member

EDIT: Also noticing @ofborg is failing to evaluate given this missed the last staging cycle. What's the acceptable fix here? Leaving it as is or performing a rebase?

Rebase

It's imperative this gets through before then to avoid leaving this PR dangling for too long. I will check upstream activity and if it has died down, I'm unblocking this PR and bumping ALL sources to their current latest.

I'll take a look at this soon.

@RossComputerGuy
Copy link
Member

I think at this point with how many rebuilds we've got and how many fail, it might be best to retarget this to staging and throw it into this cycle.

@RossComputerGuy
Copy link
Member

Maybe it'll be better to split this up across multiple PR's, is there any reason why this needs to get in before 24.11?

@Frontear
Copy link
Member Author

Frontear commented Oct 9, 2024

Maybe it'll be better to split this up across multiple PR's, is there any reason why this needs to get in before 24.11?

Not really tbh. At first I was hoping to squeeze it in before we locked out commits, but given the fact that it's been a real PITA I don't see the point in rushing it. I do agree that throwing it into staging may be a good idea, because it does eventually succeed, just not fast.

Perhaps we should disable or bump the timeouts on some of the failing tests? Seems awfully annoying if they're going to continue interrupting the build this way.

@RossComputerGuy
Copy link
Member

Not really tbh.

Why? It's 19 commits but it'll be more reliable to test.

Perhaps we should disable or bump the timeouts on some of the failing tests? Seems awfully annoying if they're going to continue interrupting the build this way.

I don't think we can bump or disable the timeouts on Ofborg.

@Frontear
Copy link
Member Author

Frontear commented Oct 9, 2024

Not really tbh.

Why? It's 19 commits but it'll be more reliable to test.

Perhaps we should disable or bump the timeouts on some of the failing tests? Seems awfully annoying if they're going to continue interrupting the build this way.

I don't think we can bump or disable the timeouts on Ofborg.

Ah sorry I should clarify, my "Not really tbh" was an answer to "Is it necessary to get in before the cutoff".

I am absolutely a-okay with splitting it up, though I will say from my previous rebasing testing, libusbmuxd is the rebase step that caused a ton of rebuilds. It may even be the only one, but I cannot confirm that as I didn't directly test beyond that point.

As for the timeouts, I meant directly in the testing code that checkPhase runs. A lot of the timeouts are around 15-30 seconds, I think bumping them to a more comfortable 60s could possibly help with evaluation. Though on the flipside a majority of these tests fail purely due to resource exhaustion, so maybe it wouldn't do anything but hang longer per test.

I could do another rebase and determine the scope of rebuilds each commit causes, by just doing an evaluation. I could then grab all the small/no rebuild commits and move them to a separate PR, then rebase this one back onto staging and keep only the massive rebuilds here. Would that be acceptable?

@Frontear
Copy link
Member Author

Frontear commented Oct 18, 2024

As requested, minor rebuilds have been separated into a different commit (see #349567).

I kept all the massive rebuilds here, and I did not separate each commit into its own PR because they largely all target the same derivations, ie there are too many common rebuilds to justify separation, as each commit would cause approximately the same amount of rebuilds anyways.

I cannot commit to another cycle of testing, so I will refrain from bumping these drvs again, despite the fact that upstream has new commits. I am also considering removing unstableGitUpdater from these drvs, as they seem too volatile to update arbitrarily, given the load of build timeouts -> failures that we've seen.

Furthermore, I am considering dropping myself from maintainer for just these packages for the same aforementioned reasons. I cannot commit myself to building and testing these packages, as my system is simply incapable of such a task, and I think it would be unwise for me to act as maintainer without being able to fulfill such a basic responsibility. This would unfortunately leave some of them orphaned, hopefully other members in the community with an interest can adopt them.

cc @RossComputerGuy @restoration578 I would appreciate if you both could roll with another nixpkgs-review on this PR.

NOTE: I have temporarily kept the libtatsu commit here, because libimobiledevice depends on it for building. Once the other PR merges, I can drop the commit from here and rebase onto master, whichll have the libtatsu init commit.

@Frontear Frontear added the 2.status: blocked by pr/issue Another PR or issue is preventing this from being completed label Oct 18, 2024
@Frontear Frontear marked this pull request as draft October 18, 2024 16:18
@Frontear
Copy link
Member Author

Marking as draft to prevent accidental merge

@Frontear Frontear changed the title libtatsu: init + update libimobiledevice drvs libimobiledevice-drvs: update all with major rebuilds Oct 18, 2024
@greg-hellings
Copy link
Contributor

I saw 2 failed builds, and 8 packages that couldn't build as a result of those, on x86_64-linux. gnome-control-center and falkon both failed to build, and that left the total unbuilt package list at: nome-control-center gnome-control-center.debug kdePackages.falkon kdePackages.falkon.debug kdePackages.falkon.dev kdePackages.falkon.devto ols phosh phosh-mobile-settings

@Frontear
Copy link
Member Author

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 342961


x86_64-linux

❌ 4 packages failed to build:
  • kdePackages.falkon
  • kdePackages.falkon.debug (kdePackages.falkon.debug.debug ,kdePackages.falkon.debug.dev ,kdePackages.falkon.debug.devtools)
  • kdePackages.falkon.dev (kdePackages.falkon.dev.debug ,kdePackages.falkon.dev.dev ,kdePackages.falkon.dev.devtools)
  • kdePackages.falkon.devtools (kdePackages.falkon.devtools.debug ,kdePackages.falkon.devtools.dev ,kdePackages.falkon.devtools.devtools)
✅ 2 packages built:
  • gnome-control-center
  • gnome-control-center.debug (gnome-control-center.debug.debug)

@Frontear
Copy link
Member Author

Frontear commented Oct 20, 2024

I saw 2 failed builds, and 8 packages that couldn't build as a result of those, on x86_64-linux. gnome-control-center and falkon both failed to build, and that left the total unbuilt package list at: nome-control-center gnome-control-center.debug kdePackages.falkon kdePackages.falkon.debug kdePackages.falkon.dev kdePackages.falkon.devto ols phosh phosh-mobile-settings

I'm able to build gnome-control-center, so I suspect that may have been due to your system timing out, which happened a lot. Will investigate the kdePackages.falkon issue. kdePackages.falkon is broken on master, thereby unrelated to this PR. Confirmed with nix build github:NixOS/nixpkgs#kdePackages.falkon and hydra

@TheRealKeto
Copy link
Contributor

@Frontear I was looking to update libimobiledevice-glue to its latest released version, 1.3.1. Would be best to have it point to a stable release now that there's one supporting libtatsu.

@Frontear
Copy link
Member Author

Frontear commented Oct 25, 2024

@Frontear I was looking to update libimobiledevice-glue to its latest released version, 1.3.1. Would be best to have it point to a stable release now that there's one supporting libtatsu.

I'd do this but as you've probably seen from upstream's behaviour they don't leverage proper releases often enough for this change to be worth it.

At best, we'd move to their latest release, then a couple months down the line the release would need to point back onto an unstable revision because no new tags have happened since.

To clarify, I can absolutely bump the PR no problem, but I don't think its worth to use their new tag, instead I'd just point to the latest commit.

@TheRealKeto
Copy link
Contributor

To clarify, I can absolutely bump the PR no problem, but I don't think its worth to use their new tag, instead I'd just point to the latest commit.

@Frontear Sounds good. Upstream has already pushed two commits that are untagged, so I understand the rationale.

@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: blocked by pr/issue Another PR or issue is preventing this from being completed 2.status: merge conflict This PR has merge conflicts with the target branch 8.has: clean-up 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 11-100 10.rebuild-linux: 501-1000 10.rebuild-linux: 501+ 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants