Skip to content

uefitoolPackages.new-engine: A62 -> A71#240420

Merged
Lassulus merged 1 commit intoNixOS:masterfrom
athre0z:update-uefitool
May 20, 2025
Merged

uefitoolPackages.new-engine: A62 -> A71#240420
Lassulus merged 1 commit intoNixOS:masterfrom
athre0z:update-uefitool

Conversation

@athre0z
Copy link
Member

@athre0z athre0z commented Jun 28, 2023

Description of changes

This commit upgrades uefiToolPackages.new-engine to the latest version as of writing. Version A68 is now using Qt6 and no longer comes with a custom build script, so the build logic compared to the old-engine variant has become sufficiently divergent that I decided that it would be easier to split them into two entirely different derivations.

Additionally, I:

  • added myself as a maintainer
  • added a patch that allows the new-engine variant to build on Darwin
  • correctly set mainProgram in the old-engine variant

A68 release on GitHub: https://github.com/LongSoft/UEFITool/releases/tag/A71

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux (via qemu)
    • x86_64-darwin (via rosetta)
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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.

@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by a maintainer of all the package it changes. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Jun 28, 2023
@ajs124
Copy link
Member

ajs124 commented Jul 28, 2023

is the old-engine variant still useful? if not, maybe we can just drop it.

@athre0z
Copy link
Member Author

athre0z commented Jul 28, 2023

Theoretically the A in the version number of the new-engine variant stands for alpha, but on the other hand the old engine hasn't seen an update in 3 years. Not sure if people are still using it. It's not really much of a hassle to keep it, so I figured I'd leave it in until the devs condider the new engine to no longer be "alpha".

@jiegec
Copy link
Member

jiegec commented Aug 12, 2023

Result of nixpkgs-review pr 240420 run on x86_64-linux 1

1 package built:
  • uefitool (uefitoolPackages.new-engine)

@jiegec
Copy link
Member

jiegec commented Aug 12, 2023

Result of nixpkgs-review pr 240420 run on aarch64-darwin 1

1 package built:
  • uefitool (uefitoolPackages.new-engine)

@athre0z
Copy link
Member Author

athre0z commented Sep 19, 2023

Rebased on master -- no other changes.

@athre0z athre0z changed the title uefitoolPackages.new-engine: A62 -> A67 uefitoolPackages.new-engine: A62 -> A68 Nov 4, 2023
@athre0z
Copy link
Member Author

athre0z commented Nov 4, 2023

Rebased on master and updated to A68.

@athre0z athre0z force-pushed the update-uefitool branch from d737dd8 to 86be174 Compare May 19, 2024 17:54
@athre0z
Copy link
Member Author

athre0z commented May 19, 2024

Rebased on master.

@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Sep 10, 2024
@ghost ghost closed this Dec 19, 2024
@ghost ghost reopened this Jan 22, 2025
@lopter
Copy link
Contributor

lopter commented Feb 7, 2025

Hello,

FWIW, I did the rebase, see: 84e5df5

Procedure was to checkout old version of Nixpkgs, apply @athre0z's patch, and re-apply conflicting changes since then.

@athre0z
Copy link
Member Author

athre0z commented Feb 7, 2025

Happy to rebase this if it helps. I had sort of abandoned it after it hadn't been reviewed / approved for more than a year.

@lopter
Copy link
Contributor

lopter commented Feb 9, 2025

Happy to rebase this if it helps. I had sort of abandoned it after it hadn't been reviewed / approved for more than a year.

I guess it would be nice to update the PR with the rebase I did, maybe you can get a fresh checkout of nixpkgs, apply the diff from the commit I made earlier, and update the PR?

Anyway, thank you for the work, I am trying to help with some old Thinkpad, and it needs Coreboot in order to make some hardware upgrade, and apparently uefitool is needed in the process.

@athre0z athre0z force-pushed the update-uefitool branch 2 times, most recently from 97a988e to e6b2f86 Compare February 16, 2025 00:38
@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Feb 16, 2025
@athre0z
Copy link
Member Author

athre0z commented Feb 16, 2025

Thanks for providing the updated diff: did as you suggested! :)

@athre0z athre0z force-pushed the update-uefitool branch 2 times, most recently from df8d646 to 452b8d8 Compare February 16, 2025 00:46
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Mar 16, 2025
@zimward
Copy link
Contributor

zimward commented May 1, 2025

whats the status of this? also in the mean time upstream released version A71

@athre0z
Copy link
Member Author

athre0z commented May 1, 2025

Happy to update it, though at this point I don't exactly have high hopes of it being reviewed & merged. Not sure how to proceed here.

@zimward
Copy link
Contributor

zimward commented May 1, 2025

if the conflict gets resolved and you update it i am sure that we can get it merged in the not too soon future.

This commit upgrades uefiToolPackages.new-engine to the latest version
as of writing. Version A68 is now using Qt6 and no longer comes with
a custom build script, so the build logic compared to the old-engine
variant has become sufficiently divergent that I decided that it would
be easier to split them into two entirely different derivations.

Additionally, I:

- added myself as a maintainer
- added a patch that allows the new-engine variant to build on Darwin
- correctly set `mainProgram` in the old-engine variant
@athre0z athre0z force-pushed the update-uefitool branch from 452b8d8 to a2edcee Compare May 2, 2025 15:56
@athre0z athre0z changed the title uefitoolPackages.new-engine: A62 -> A68 uefitoolPackages.new-engine: A62 -> A71 May 2, 2025
@athre0z
Copy link
Member Author

athre0z commented May 2, 2025

Rebased and updated to A71. Let's see if we can get it merged this time.

Copy link
Contributor

@zimward zimward left a comment

Choose a reason for hiding this comment

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

A few conventions changed since you first opened the PR, after that i think it should be in a pretty good state.

@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label May 3, 2025
@athre0z athre0z force-pushed the update-uefitool branch from a2edcee to 32b43f6 Compare May 3, 2025 19:10
@athre0z
Copy link
Member Author

athre0z commented May 3, 2025

Thank you! I addressed all feedback. I slightly adjusted your suggested meta.mainProgram to "uefitool" since the executable name has changed in the new engine variant.

Copy link
Contributor

@zimward zimward left a comment

Choose a reason for hiding this comment

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

LGTM

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one person. label May 4, 2025
Copy link
Contributor

@LordGrimmauld LordGrimmauld left a comment

Choose a reason for hiding this comment

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

Have some nits.
Packages do build on x86_64-linux. Cross is broken because qt cross is a mess right now, can't really test the package itself. Building against musl and LLVM right now, but that'll take a few hours and is probably not blocking either.

@athre0z athre0z force-pushed the update-uefitool branch from 32b43f6 to c570df8 Compare May 4, 2025 17:46
@LordGrimmauld
Copy link
Contributor

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 240420


x86_64-linux

✅ 1 package built:
  • uefitool (uefitoolPackages.new-engine)

Copy link
Contributor

@LordGrimmauld LordGrimmauld left a comment

Choose a reason for hiding this comment

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

Diff looks good and the thing builds, though i do not feel competent judging the functionality of the actual software

@zimward
Copy link
Contributor

zimward commented May 4, 2025

functionality wise it looks fine. i inspected some bios images and everything looks like i would expect. (editing is sadly still not supported in NE, so not much more to test)

@wegank wegank added 12.approvals: 2 This PR was reviewed and approved by two persons. and removed 12.approvals: 1 This PR was reviewed and approved by one person. labels May 5, 2025
@Lassulus Lassulus merged commit 57ea4e4 into NixOS:master May 20, 2025
28 checks passed
@athre0z athre0z deleted the update-uefitool branch May 20, 2025 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 11.by: package-maintainer This PR was created by a maintainer of all the package it changes. 12.approvals: 2 This PR was reviewed and approved by two persons.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants