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

acpica-tools: 20200430 -> 20200925 (and remove redundant iasl package) #101821

Closed
wants to merge 5 commits into from

Conversation

@delroth
Copy link
Contributor

@delroth delroth commented Oct 27, 2020

Motivation for this change

ACPICA version bump. The auto update PRs from ryantm haven't been getting through recently since one of the tools was removed upstream.

Along the way I removed the special "iasl" package which is just the same upstream as acpica-tools. We could keep split packages for closure size reasons, but we're looking at a 1-2MB difference for what is mostly a rarely used build / dev tool.

@GrahamcOfBorg build virtualbox coreboot-utils seabios

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
@delroth
Copy link
Contributor Author

@delroth delroth commented Oct 27, 2020

@dtzWill fyi since you've been maintaining the iasl package the most recently. I noticed in dd152db you've already been thinking about merging these two, wdyt?

@delroth
Copy link
Contributor Author

@delroth delroth commented Oct 27, 2020

@GrahamcOfBorg build virtualbox coreboot-utils seabios xen

@delroth
Copy link
Contributor Author

@delroth delroth commented Nov 17, 2020

/marvin opt-in
/status needs_reviewer

@marvin-mk2 marvin-mk2 bot added the marvin label Nov 17, 2020
@marvin-mk2
Copy link

@marvin-mk2 marvin-mk2 bot commented Nov 17, 2020

Hi! I'm an experimental bot. My goal is to guide this PR through its stages, hopefully ending with a merge. You can read up on the usage here.

@marvin-mk2
Copy link

@marvin-mk2 marvin-mk2 bot commented Nov 21, 2020

@glittershark please review.

pkgs/top-level/aliases.nix Outdated Show resolved Hide resolved
@glittershark
Copy link
Member

@glittershark glittershark commented Nov 24, 2020

@delroth would you mind rebasing this?

delroth added 4 commits Nov 25, 2020
New upstream release. Note: the acpinames tool has been removed upstream
since v20200528 [1]

[1] acpica/acpica@876fd5a
These are basically the same package, built from the same source, they
just happen to build different targets.

The closure size difference (~1-2MB) doesn't seem to make up for the
added maintenance cost of having two packages.
@delroth delroth force-pushed the acpica-20200925 branch from 6493d28 to 1a43341 Nov 25, 2020
@delroth
Copy link
Contributor Author

@delroth delroth commented Nov 25, 2020

@glittershark
Copy link
Member

@glittershark glittershark commented Nov 25, 2020

Result of nixpkgs-review pr 101821 1

2 packages marked as broken and skipped:
  • linuxPackages_hardkernel_4_14.virtualbox
  • linuxPackages_hardkernel_latest.virtualbox
47 packages built:
  • OVMF
  • OVMF-CSM
  • OVMF-secureBoot
  • acpica-tools
  • acpidump-all
  • coreboot-utils
  • fwts
  • gnome3.gnome-boxes
  • libguestfs
  • libvmi
  • linuxPackages-libre.virtualbox
  • linuxPackages.virtualbox (linuxPackages_5_4.virtualbox)
  • linuxPackages_4_14.virtualbox
  • linuxPackages_4_19.virtualbox
  • linuxPackages_4_4.virtualbox
  • linuxPackages_4_9.virtualbox
  • linuxPackages_5_8.virtualbox
  • linuxPackages_5_9.virtualbox (linuxPackages_latest.virtualbox)
  • linuxPackages_hardened.virtualbox
  • linuxPackages_latest-libre.virtualbox
  • linuxPackages_latest_hardened.virtualbox
  • linuxPackages_latest_xen_dom0.virtualbox
  • linuxPackages_latest_xen_dom0_hardened.virtualbox
  • linuxPackages_testing_bcachefs.virtualbox
  • linuxPackages_xen_dom0.virtualbox
  • linuxPackages_xen_dom0_hardened.virtualbox
  • linuxPackages_zen.virtualbox
  • python27Packages.guestfs
  • python37Packages.guestfs
  • python38Packages.guestfs
  • qemu_xen (qemu_xen_4_8)
  • qemu_xen-light (qemu_xen_4_8-light)
  • qemu_xen_4_10
  • qemu_xen_4_10-light
  • qubes-core-vchan-xen
  • seabios
  • vagrant
  • virtualbox
  • virtualboxHardened
  • virtualboxHeadless
  • virtualboxWithExtpack
  • xen (xenPackages.xen-vanilla ,xenPackages.xen_4_8-vanilla ,xen_4_8)
  • xen-light (xenPackages.xen-light ,xenPackages.xen_4_8-light ,xen_4_8-light)
  • xen-slim (xenPackages.xen-slim ,xenPackages.xen_4_8-slim ,xen_4_8-slim)
  • xen_4_10-light (xenPackages.xen_4_10-light)
  • xen_4_10-slim (xenPackages.xen_4_10-slim)
  • xen_4_10 (xenPackages.xen_4_10-vanilla)

@glittershark
Copy link
Member

@glittershark glittershark commented Nov 25, 2020

I don't have strong opinions one way or another about the package rename / deprecation issue, as I'm not sure what the norms are there.

@glittershark
Copy link
Member

@glittershark glittershark commented Nov 25, 2020

/status needs_merger

Copy link
Member

@timokau timokau left a comment

Looks good to me. I don't want to step over @SuperSandro2000's review though. Ping me again in ~3 days (marvin should also do that automatically) if they haven't responded.

@delroth
Copy link
Contributor Author

@delroth delroth commented Nov 25, 2020

In order to avoid wasting more combined reviewer/merger time on this I just removed the alias, as I said I didn't feel very strongly about it in the first place but wanted to better understand when to / not to add one (it seems like the guidelines are blurry though).

This should be ready to merge, as this was the last pending issue here.

(FYI the alias is still added in a commit and is then removed in the last commit of the branch. This is to make sure all commits evaluate cleanly, and to keep things working when e.g. bisecting.)

@timokau
Copy link
Member

@timokau timokau commented Nov 25, 2020

As @SuperSandro2000 originally suggested, if you remove the alias you should replace it with a throw that directs the user to the new package. If you have neither one it can be hard to fix a broken config.

This has most of the benefit of an alias (since does not cause big problems downstream) without any potential for confusion. Alias or throw are both fine in my book, but you should add one of them.

@@ -4696,8 +4696,6 @@ in

i-score = libsForQt514.callPackage ../applications/audio/i-score { };

iasl = callPackage ../development/compilers/iasl { };
Copy link
Member

@SuperSandro2000 SuperSandro2000 Nov 25, 2020

Please add an alias that throws that the package was removed.

@stale
Copy link

@stale stale bot commented Jul 21, 2021

I marked this as stale due to inactivity. → More info

@delroth
Copy link
Contributor Author

@delroth delroth commented Jul 21, 2021

Too much effort required for me to try and clean this up, abandoning this PR. Guess we'll just keep duplicate packages around.

@delroth delroth closed this Jul 21, 2021
@kidonng kidonng mentioned this pull request Aug 8, 2021
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants