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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 5 commits into from
Closed

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

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 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 commented Oct 27, 2020

@GrahamcOfBorg build virtualbox coreboot-utils seabios xen

@delroth
Copy link
Contributor Author

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 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 bot commented Nov 21, 2020

@glittershark please review.

@glittershark
Copy link
Member

@delroth would you mind rebasing this?

@delroth
acpica-tools: 20200430 -> 20200925
e2ea5a9 
New upstream release. Note: the acpinames tool has been removed upstream
since v20200528 [1]

[1] acpica/acpica@876fd5a
@delroth
iasl: replace with acpica-tools
8911661 
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
treewide: cleanup references to iasl (-> acpica-tools)
780bbe5
@delroth
coreboot-utils: appease the EditorConfig gods
1a43341
@delroth
Copy link
Contributor Author

delroth commented Nov 25, 2020

@glittershark done

@glittershark
Copy link
Member

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

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

/status needs_merger

@marvin-mk2 marvin-mk2 bot requested a review from timokau November 25, 2020 15:51
timokau
timokau approved these changes Nov 25, 2020
View reviewed changes
Copy link
Member

@timokau timokau left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 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 commented Nov 25, 2020 •
edited

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.

SuperSandro2000
SuperSandro2000 requested changes Nov 25, 2020
View reviewed changes
pkgs/top-level/all-packages.nix
@@ -4696,8 +4696,6 @@ in

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@stale
Copy link

stale bot commented Jul 21, 2021

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

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 21, 2021
@delroth
Copy link
Contributor Author

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
This was referenced Aug 3, 2021
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SuperSandro2000 SuperSandro2000 SuperSandro2000 requested changes

@glittershark glittershark glittershark approved these changes

@timokau timokau timokau approved these changes

Assignees
No one assigned
Labels
2.status: merge conflict 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 8.has: clean-up 10.rebuild-darwin: 11-100 10.rebuild-linux: 11-100 awaiting_reviewer marvin
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@delroth @glittershark @timokau @SuperSandro2000