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

lib.systems.inspect: isEfi → hasEfi rename #263711

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

lib.systems.inspect: isEfi → hasEfi rename #263711

wants to merge 1 commit into from

Conversation

RaitoBezarius
Copy link
Member

@RaitoBezarius RaitoBezarius commented Oct 27, 2023 •
edited
Loading

This PR offers one change related to EFI improvements required for #231951.

Move away from isEfi meaning "does it have an EFI platform" and onto isEfi (is this EFI system, doesn't exist in this PR yet.) and hasEfi (does it have an EFI platform).

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/)
  • 23.11 Release Notes (or backporting 23.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.

@RaitoBezarius RaitoBezarius requested a review from a user October 27, 2023 06:26
@github-actions github-actions bot added 6.topic: systemd 6.topic: lib The Nixpkgs function library labels Oct 27, 2023
This was referenced Oct 27, 2023
Draft
Closed
flokli
flokli reviewed Oct 27, 2023
View reviewed changes
Copy link
Contributor

@flokli flokli left a comment

Choose a reason for hiding this comment

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

This might be missing some context. Maybe the EFI topic should also be explained in doc/stdenv/platform-notes.chapter.md (not sure if there's a better place)

lib/systems/inspect.nix Show resolved Hide resolved
@RaitoBezarius
Copy link
Member Author

This might be missing some context. Maybe the EFI topic should also be explained in doc/stdenv/platform-notes.chapter.md (not sure if there's a better place)

Done.

@github-actions github-actions bot added the 8.has: documentation This PR adds or changes documentation label Oct 27, 2023
flokli
flokli reviewed Oct 27, 2023
View reviewed changes
doc/stdenv/platform-notes.chapter.md Outdated Show resolved Hide resolved
doc/stdenv/platform-notes.chapter.md Outdated Show resolved Hide resolved
doc/stdenv/platform-notes.chapter.md Outdated Show resolved Hide resolved
doc/stdenv/platform-notes.chapter.md Outdated Show resolved Hide resolved
@RaitoBezarius
Copy link
Member Author

@flokli PTAL.

@delroth delroth added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Oct 27, 2023
alyssais
alyssais requested changes Oct 27, 2023
View reviewed changes
doc/stdenv/platform-notes.chapter.md Outdated Show resolved Hide resolved
doc/stdenv/platform-notes.chapter.md Outdated Show resolved Hide resolved
doc/stdenv/platform-notes.chapter.md Outdated Show resolved Hide resolved
pkgs/build-support/bintools-wrapper/default.nix Outdated Show resolved Hide resolved
pkgs/build-support/bintools-wrapper/default.nix Outdated Show resolved Hide resolved
doc/stdenv/platform-notes.chapter.md Outdated Show resolved Hide resolved
@alyssais
Copy link
Member

Commit messages could be attribute-based: "wrapBintoolsWith: " for the first and "lib.systems.inspect: " for the second.

@RaitoBezarius RaitoBezarius force-pushed the stdenv-efi branch 2 times, most recently from add77ba to b80e6c9 Compare October 27, 2023 10:01
@delroth delroth removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Oct 27, 2023
@RaitoBezarius RaitoBezarius changed the title stdenv: some EFI-related improvements lib.systems.inspect: isEfi → hasEfi rename Oct 27, 2023
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 and removed 10.rebuild-darwin: 11-100 labels Oct 27, 2023
alyssais
alyssais reviewed Oct 28, 2023
View reviewed changes
lib/systems/inspect.nix Outdated Show resolved Hide resolved
@RaitoBezarius
lib.systems.inspect: isEfi -> hasEfi
2f76cfe 
This was never about "is this a UEFI system", but "does it support potentially a UEFI
environment".

Reflected in the rest of the call sites.
Ericson2314
Ericson2314 approved these changes Oct 28, 2023
View reviewed changes
Copy link
Member

@Ericson2314 Ericson2314 left a comment

Choose a reason for hiding this comment

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

I agree on the general idea of the destination; happy to let others figure out the deprecation cycle.

@delroth delroth added the 12.approvals: 2 This PR was reviewed and approved by two reputable people label Oct 28, 2023
ghost
ghost reviewed Oct 30, 2023
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Could we just get rid of isEfi completely?

EFI (or UEFI) is a property of a specific motherboard, not a CPU or an architecture.

This is only used in two places: systemd and gnu-efi. Frankly it should just be copied into their respective meta.platforms attributes and the isEfi attribute deprecated.

@RaitoBezarius it looks like your #231951 doesn't even use this?

I don't want to drag things out, but if we're going to deprecate isEfi let's not add another misfeature hasEfi that needs to be deprecated in the next cycle...

I'll submit an alternative that just moves this into systemd and gnu-efi. If you all still want to go with this PR, please at least apply the change below.

@ghost
Copy link

ghost commented Oct 30, 2023

Could we just get rid of isEfi completely?

Suggested implementation:

@alyssais alyssais mentioned this pull request Oct 30, 2023
Closed
13 tasks
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Mar 20, 2024
@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alyssais alyssais alyssais requested changes

@Ericson2314 Ericson2314 Ericson2314 approved these changes

@flokli flokli flokli approved these changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
2.status: merge conflict This PR has merge conflicts with the target branch 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 6.topic: lib The Nixpkgs function library 6.topic: systemd 8.has: documentation This PR adds or changes documentation 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 12.approvals: 2 This PR was reviewed and approved by two reputable people
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@RaitoBezarius @alyssais @flokli @Ericson2314 @delroth @wegank