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

writableDirWrapper: init #361114

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

Conversation

pluiedev
Copy link
Contributor

@pluiedev pluiedev commented Dec 2, 2024

An unfortunate fact of life is that some packages (mostly video games) really want a writable state directory that also has all assets and resources in it. This helper creates a wrapper script that helps in automatically linking resources and updating the links when versions are updated, while keeping config files and other state files intact.

Dependents:

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

@github-actions github-actions bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Dec 2, 2024
@nix-owners nix-owners bot requested a review from philiptaron December 2, 2024 15:08
Copy link
Contributor

@ambroisie ambroisie left a comment

Choose a reason for hiding this comment

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

Does the documentation get linked into the manual automatically?

Otherwise LGTM.

@keenanweaver
Copy link
Member

If you're looking for test games, ASCII Sector writes stuff to the current directory of the executable (logs, saves, options).

Copy link
Contributor

@philiptaron philiptaron left a comment

Choose a reason for hiding this comment

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

This looks like an intriguing start.

Requesting changes for:

  1. Passing the editorconfig check (no trailing whitespace, looks like)
  2. A test

I'm a little surprised that one of the existing wrappers, like buildFHSEnv in particular don't work for this usecase.

Comment on lines 97 to 98
```console
SGVsbG8gd29ybGQhCg==
```
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a test for this? For an example, take a look at pkgs/test/replace-vars/ and commit b0b3172 for when I did this for the replaceVars build support helper.

pkgs/build-support/writable-dir-wrapper/default.nix Outdated Show resolved Hide resolved
pkgs/build-support/writable-dir-wrapper/default.nix Outdated Show resolved Hide resolved
pkgs/build-support/writable-dir-wrapper/default.nix Outdated Show resolved Hide resolved
pkgs/build-support/writable-dir-wrapper/default.nix Outdated Show resolved Hide resolved
pkgs/build-support/writable-dir-wrapper/default.nix Outdated Show resolved Hide resolved
pkgs/build-support/writable-dir-wrapper/default.nix Outdated Show resolved Hide resolved
@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Dec 2, 2024
@pluiedev
Copy link
Contributor Author

pluiedev commented Dec 2, 2024

I'm a little surprised that one of the existing wrappers, like buildFHSEnv in particular don't work for this usecase.

To my knowledge buildFHSEnv lives completely within the store, which is obviously read-only. The whole point is to make a writable directory (say, in the user's XDG_DATA_HOME for apps or in /var/lib/something for services) for programs that expect full control over its runtime directory.

@pluiedev
Copy link
Contributor Author

pluiedev commented Dec 2, 2024

Does the documentation get linked into the manual automatically?

@ambroisie TBH, I'm not even sure if I got the documentation format completely correct — existing examples all seem to be contradictory and I'm only following what doc/README.md says here

@pluiedev pluiedev mentioned this pull request Dec 2, 2024
13 tasks
@wegank wegank removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Dec 2, 2024
@philiptaron
Copy link
Contributor

With a test, I'm inclined to merge.

@pluiedev
Copy link
Contributor Author

pluiedev commented Dec 2, 2024

I think this might need multiple tests, purely because there's a lot of edge cases and bugs I discovered in other derivations that actually use it — however it's quite late for me right now, so I'll look into writing a test suite tomorrow

@philiptaron
Copy link
Contributor

philiptaron commented Dec 2, 2024

I think this might need multiple tests, purely because there's a lot of edge cases and bugs I discovered in other derivations that actually use it — however it's quite late for me right now, so I'll look into writing a test suite tomorrow

Oh yes, multiple tests are great. Take all the time you need. No timer is ticking!

Copy link
Member

@getchoo getchoo left a comment

Choose a reason for hiding this comment

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

Overall the current implementation is good, but I think we could take this a step further

The way this is actually a function (and even takes arguments like name and version) lead me to believe this was more akin to symlinkJoin or linkFarm, where it would just create a store path with the specified folder structure + extra commands; but obviously this isn't the case

I think a much better approach here would be to base this off other application wrappers, using CLI arguments and environment variables over a traditional Nix function -- sorta like wrapGAppsHook. This would hopefully make things a bit less confusing (as you would no longer be dealing with a Nix function to create a script wrapper that will also have it's own arguments), composable, and be a bit more efficient by allowing all derivations to use one writable-dir-wrapper rather than building a new one each time

I'm not going to block on a complete rework like this though, as the actual reviews I left would make this Good Enough ™️ IMO, but I would really encourage giving it a try. Let me know if I can help :)

pkgs/build-support/writable-dir-wrapper/default.nix Outdated Show resolved Hide resolved
pkgs/build-support/writable-dir-wrapper/default.nix Outdated Show resolved Hide resolved
pkgs/build-support/writable-dir-wrapper/default.nix Outdated Show resolved Hide resolved
pkgs/build-support/writable-dir-wrapper/default.nix Outdated Show resolved Hide resolved
pkgs/build-support/writable-dir-wrapper/default.nix Outdated Show resolved Hide resolved
pkgs/build-support/writable-dir-wrapper/default.nix Outdated Show resolved Hide resolved
pkgs/build-support/writable-dir-wrapper/default.nix Outdated Show resolved Hide resolved
pkgs/build-support/writable-dir-wrapper/default.nix Outdated Show resolved Hide resolved
Copy link
Contributor

github-actions bot commented Dec 3, 2024

Eval summary

  • Added packages: 0
  • Removed packages: 0
  • Changed packages: 0
  • Rebuild Linux: 0
  • Rebuild Darwin: 0

@pluiedev
Copy link
Contributor Author

pluiedev commented Dec 3, 2024

Thank you GitHub, very helpful

i approve

@nix-owners nix-owners bot requested a review from Ericson2314 December 4, 2024 13:47
@pluiedev
Copy link
Contributor Author

pluiedev commented Dec 4, 2024

I've made it more akin to a wrapProgram-style hook (it actually has the same API as makeWrapper/wrapProgram now, with some added flags). I still have to write some tests and revise the docs but honestly I'm kind of burnt out from working on this one PR (and its dependents) for 3 days straight and I'll come back to this a bit later

@pluiedev pluiedev force-pushed the push-qmpqqztwlpor branch 2 times, most recently from 76b3eae to 19935d0 Compare December 4, 2024 13:55
@github-actions github-actions bot added 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Dec 4, 2024
@pluiedev pluiedev force-pushed the push-qmpqqztwlpor branch 3 times, most recently from d31f802 to e41ae32 Compare December 4, 2024 14:26
@philiptaron
Copy link
Contributor

I really like the direction this is going. Please take the time you need, then mark this as non-draft when you're ready for reviews and merge.

@philiptaron philiptaron marked this pull request as draft December 4, 2024 20:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants