Skip to content

erofs-utils: enable running on darwin#239473

Merged
wegank merged 1 commit intoNixOS:masterfrom
nikstur:erofs-darwin
Jun 27, 2023
Merged

erofs-utils: enable running on darwin#239473
wegank merged 1 commit intoNixOS:masterfrom
nikstur:erofs-darwin

Conversation

@nikstur
Copy link
Contributor

@nikstur nikstur commented Jun 23, 2023

Description of changes

Enable erofs-utils to build on Darwin. This is especially interesting because it enables darwin.builder to use virtualisation.useNixStoreImage = true; and virtualisation.writableStore = false; at the same time. With this combination of options an erofs image is built just in time inside the script that calls QEMU.

Doesn't currently build because of #236560

#239248 introduces a fix.

As a stopgap solution, tests for perlPackages.Po4a are disabled in b226d46840f6b20ec0d2072c41678a5a5b184368
When a proper solution is added to Nixpkgs, the corresponding commit can be removed.

For reference, the homebrew formula for erofs-utils: https://github.com/Homebrew/homebrew-core/blob/e4660044d3e2d53f5a74efd079bf29af2bd3f12a/Formula/erofs-utils.rb

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • 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 the 6.topic: darwin Running or building packages on Darwin label Jun 23, 2023
@nikstur nikstur requested a review from RaitoBezarius June 23, 2023 22:11
@nikstur nikstur assigned Gabriella439 and unassigned Gabriella439 Jun 23, 2023
@nikstur nikstur requested review from Gabriella439 and emilazy June 23, 2023 22:11
@emilazy
Copy link
Member

emilazy commented Jun 23, 2023

Very cool. Successfully tested {mkfs,fsck}.erofs on x86_64-darwin; not familiar enough with erofs to do anything more expensive.

Does this mean we could avoid the Darwin builder having any persistent state at all? I didn't realize the remote builder protocol could work with an immutable store. Would there be a startup time impact from making the image or is it scoped down to only the required components for the builder that shouldn't change often?

@ofborg ofborg bot added the 8.has: package (new) This PR adds a new package label Jun 23, 2023
@ofborg ofborg bot requested review from a user and prusnak June 23, 2023 22:27
@ofborg ofborg bot added 10.rebuild-darwin: 101-500 This PR causes between 101 and 500 packages to rebuild on Darwin. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 2501-5000 This PR causes many rebuilds on Linux and should target the staging branches. labels Jun 23, 2023
@emilazy emilazy mentioned this pull request Jun 23, 2023
12 tasks
@nikstur
Copy link
Contributor Author

nikstur commented Jun 23, 2023

Would there be a startup time impact from making the image

Yes there would be an impact. See the original commit for more info: afc60d0

If we can build erofs images on MacOS too, however, we could get rid of the storeImage in the qemu-vm which uses lib/make-disk-img.nix, producing a derivation (which is then cached). To add persistence to the Store image we could just mount an overlayfs over it (like we already do in many other places). By doing that we could get rid of this warning: https://github.com/NixOS/nixpkgs/blob/master/nixos/modules/virtualisation/qemu-vm.nix#L934

Even if every test that uses a Nix Store image is a little bit slower, I think overall it would be still a big win to not cache so many mostly useless images anymore.

@nikstur nikstur force-pushed the erofs-darwin branch 2 times, most recently from a811671 to 8608c02 Compare June 25, 2023 17:41
@prusnak
Copy link
Member

prusnak commented Jun 25, 2023

Nit: platforms = platforms.unix is more appropriate

@ghost
Copy link

ghost commented Jun 26, 2023

Erofs is a Linux thing.

@RaitoBezarius
Copy link
Member

@ehmry Closing PRs like this is quite rude, EROFS is a Linux filesystem sure, but there's nothing that says you have to use Linux to write stuff related to that filesystem, is there?

@Janik-Haag
Copy link
Member

Janik-Haag commented Jun 26, 2023

I'm reopening this since it seems like this was closed prematurely and people doing the work to make stuff build on officially unsupported platforms usually is a good thing.

@Janik-Haag Janik-Haag reopened this Jun 26, 2023
@ghost
Copy link

ghost commented Jun 26, 2023

I wan't aware that FUSE was permitted by Apple, so I was in error to close this issue. I still don't think accommodating Apple users is helpful.

@Janik-Haag
Copy link
Member

Janik-Haag commented Jun 26, 2023

I still don't think accommodating Apple users is helpful.

that is another discussion entirely. I think piegames posted about this on discourse a few days ago.

@RaitoBezarius
Copy link
Member

I wan't aware that FUSE was permitted by Apple, so I was in error to close this issue. I still don't think accommodating Apple users is helpful.

Is your point, you don't want to maintain a Darwin-enabled erofs-utils? In that case, would someone else taking ownership of that part as a co-maintainer enough for you?

@emilazy
Copy link
Member

emilazy commented Jun 26, 2023

I don't think this is about FUSE. mkfs.erofs works fine on Darwin. Why close a working PR with a clearly-stated rationale?

@Janik-Haag
Copy link
Member

Janik-Haag commented Jun 26, 2023

@ehmry made a mistake, they acknowledge it. Everything should be fine now. Can we get back on topic of the pr? If you want further discussion I suggest you to move it to matrix or discourse but a pr is not the place for this,

@wegank wegank force-pushed the erofs-darwin branch 2 times, most recently from c8b888a to 22b7360 Compare June 27, 2023 08:43
@ofborg ofborg bot added the 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. label Jun 27, 2023
@ofborg ofborg bot added 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. and removed 10.rebuild-darwin: 101-500 This PR causes between 101 and 500 packages to rebuild on Darwin. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 2501-5000 This PR causes many rebuilds on Linux and should target the staging branches. labels Jun 27, 2023
@nikstur
Copy link
Contributor Author

nikstur commented Jun 27, 2023

Thank you @wegank for removing the "offending" commit that caused massive rebuilds :D.

@ehmry I'm happy to co-maintain the package to take some of the (Darwin) burden off of you.

@nikstur nikstur force-pushed the erofs-darwin branch 2 times, most recently from 8608c02 to b6abb4d Compare June 27, 2023 11:02
@ofborg ofborg bot added 2.status: merge conflict This PR has merge conflicts with the target branch 11.by: package-maintainer This PR was created by a maintainer of all the package it changes. and removed 2.status: merge conflict This PR has merge conflicts with the target branch labels Jun 27, 2023
@nikstur nikstur marked this pull request as ready for review June 27, 2023 11:45
@wegank wegank merged commit b1a839f into NixOS:master Jun 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: darwin Running or building packages on Darwin 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-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. 11.by: package-maintainer This PR was created by a maintainer of all the package it changes.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants