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

Revert "e2fsprogs: build fuse2fs on darwin" #339412

Merged
merged 1 commit into from
Sep 4, 2024

Conversation

emilazy
Copy link
Member

@emilazy emilazy commented Sep 4, 2024

Description of changes

This change, while fine in isolation, breaks evaluation in combination with #329721, as xar depends on e2fsprogs which now depends on macfuse-stubs which depends on xar. This broke staging-next.

A couple possible solutions are to disable the e2fsprogs dependency in the version of xar used for the bootstrap, or to build macfuse-stubs from source to avoid the xar dependency.

This reverts commit 0dfc820.

cc @KoHcoJlb @7c6f434c @tie @reckenrode

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

Add a 👍 reaction to pull requests you find important.

@ofborg ofborg bot added the 6.topic: darwin Running or building packages on Darwin label Sep 4, 2024
@emilazy emilazy marked this pull request as draft September 4, 2024 04:21
@emilazy
Copy link
Member Author

emilazy commented Sep 4, 2024

It seems like that PR hasn’t actually made it to staging-next, so even though this will cause issues, we don’t yet know what the report of an infinite recursion error that looked very much like this is about yet.

This change, while fine in isolation, breaks evaluation in combination
with <NixOS#329721>, as `xar` depends
on `e2fsprogs` which now depends on `macfuse-stubs` which depends on
`xar`. This broke `staging-next`.

A couple possible solutions are to disable the `e2fsprogs` dependency
in the version of `xar` used for the bootstrap, or to build
`macfuse-stubs` from source to avoid the `xar` dependency.

This reverts commit 0dfc820.
@emilazy emilazy marked this pull request as ready for review September 4, 2024 06:14
@emilazy
Copy link
Member Author

emilazy commented Sep 4, 2024

Okay, the commit landed in staging-next now and broke as expected.

@K900 K900 merged commit b9190b9 into NixOS:staging-next Sep 4, 2024
7 of 9 checks passed
@emilazy emilazy deleted the push-vqzxtynvmxuy branch September 4, 2024 06:16
@KoHcoJlb
Copy link
Contributor

KoHcoJlb commented Sep 4, 2024

Or xar could use e2fsprogs with fuse disabled
macfuse went closed-source awhile back, so It's not possible to build it from source

@emilazy
Copy link
Member Author

emilazy commented Sep 4, 2024

No, the relevant part of macFUSE is LGPL, as noted in our package definition for the stubs; here’s the repository. If it was non‐free the PR would have been a bigger issue as it would have made e2fsprogs non‐free by default :)

xar overriding e2fsprogs to disable FUSE like that seems reasonable, since it presumably only uses the library. @reckenrode pointed out that macFUSE could also just use a xar without ext2 support to unpack the .pkg and that would also break the loop enough to bring your PR back in. I’m not sure if we’d want to pull the macFUSE stubs into the stdenv bootstrap, though. If we do, we should probably build it from source, at least.

Sorry about the quick revert, but we needed to fix eval on staging-next rather than getting blocked on this. I’m happy to help get your PR back in if we can agree on what the best way to handle this is.

@reckenrode
Copy link
Contributor

Regardless of the macFUSE issue, the stdenv bootstrap probably shouldn’t include e2fsprogs. It’s fine for now, but I’ll probably include in the Darwin refactor using a stripped down xar for cctools and ld64.

One of my goals for the bootstrap is reducing the amount of unnecessary stuff that’s built (e.g., 24.05 builds Python five times during the bootstrap, but the refactor builds it only once). Using a xarMinimal is in line with that. I wonder if xarMinimal be a top-level package?

@emilazy
Copy link
Member Author

emilazy commented Sep 4, 2024

We’ve circled back around to #329721 (comment) here. I agree that we should just gate the problematic dependencies behind a single feature flag and use that for the compiler tools.

@KoHcoJlb
Copy link
Contributor

KoHcoJlb commented Sep 9, 2024

Did you agree on preferred solution?

@emilazy
Copy link
Member Author

emilazy commented Sep 13, 2024

We decided that ld64 should probably depend on a xarMinimal that overrides the optional dependencies to null, since it has no business reading ext2 images. That will require a staging cycle for the standard environment bootstrap.

That would break the circular dependency here by itself, but macfuse-stubs could also presumably use a xarMinimal. Ideally we would build it from source, though.

@KoHcoJlb
Copy link
Contributor

Is there anything I can help with? Just wondering how I can get my PR back)

@emilazy
Copy link
Member Author

emilazy commented Sep 13, 2024

So the problem is that because of the xar change, e2fsprogs is now an indirect dependency of ld64. Any change to e2fsprogs would rebuild the entire world on Darwin, and therefore needs to go to staging. @reckenrode plans to make ld64 use a stripped‐down xar in his upcoming SDK rework. That will happen for the next staging cycle in a couple of weeks, so unfortunately there’s nothing we can really do to speed that up.

The only thing I can think that we could do in the interim is to add a default‐on flag to e2fsprogs conditionalizing the FUSE stuff, and make sure it’s turned off by the xar in the stdenv bootstrap. If done carefully this will ensure that there are no mass rebuilds, but it will be somewhat ugly and get blown away by the next staging cycle.

Work that would be good for the future but wouldn’t immediately solve this would be to build the macFUSE stubs from source, perhaps even reusing the same derivation that is used for libfuse on Linux, to remove the xar dependency.

Sorry about this unfortunate collision of changes…

@KoHcoJlb
Copy link
Contributor

Ah, I see now, because stdenv now depends on xar/e2fsprogs any changes to them would cause mass rebuild thus need to go to staging.

So after stdenv is decoupled from e2fsprogs should I resubmit my original PR?
Also, if I'm not missing anything, original loop would remain (e2fsprogs -> macfuse-stubs -> xar -> e2fsprogs) which would need to be fixed separately by using xarMinimal in macfuse-stubs or building them from source.
Is this something taken care of as part of xarMinimal refactor or should I submit PR for this as well?

In the meantime I'll try to look into building macfuse-stubs from source, maybe I will submit PR for that too.
By the way will changes to macfuse-stubs cause mass rebuild?

@emilazy
Copy link
Member Author

emilazy commented Sep 18, 2024

Yes, it should be fine to revert the revert after the xarMinimal stuff goes in. macfuse-stubs should be able to use xarMinimal, since it’s just unpacking a .xar archive; that can go in the revert‐revert PR. A from‐source build would be ideal, of course, but shouldn’t be required.

I think that we don’t want macfuse-stubs to be part of the bootstrap, so hopefully it won’t cause mass rebuilds after the SDK rework.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants