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

linuxManualConfig: Use a pure-Nix readConfig #302802

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dramforever
Copy link
Contributor

@dramforever dramforever commented Apr 9, 2024

Description of changes

Use a pure-Nix implementation to parse the configfile so that IFD can be
avoided in many cases.

The original decade-old readConfig function uses a bash script and requires an
IFD even when readFile would suffice. This is really unnecessary for many uses
of linuxManualConfig.

I'm not sure whether this is a good way to do it. Please advise on code organization and whether the interface should work like this.

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.05 Release Notes (or backporting 23.05 and 23.11 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.

Use a pure-Nix implementation to parse the configfile so that IFD can be
avoided in many cases.
There's no need for allowImportFromDerivation if configfile is from a
path. An explanation of this option is added instead.
@dramforever
Copy link
Contributor Author

Ideally this is 0 rebuild (disregarding docs)... If it isn't I messed something up

@dramforever dramforever marked this pull request as ready for review April 9, 2024 15:24
@dramforever
Copy link
Contributor Author

I also checked a random config I had and this yielded exactly the same derivation as the original code.

@tpwrules
Copy link
Contributor

tpwrules commented Apr 9, 2024

I've shipped an implementation here for about a year and not had any problems, but never quite got around to submitting it upstream.

I think there is some additional subtlety around combining with kernel patch options and string options.

I think IFD is indeed still needed to use builtins.readFile on a derivation output instead of just a path. Is there a good reason to have that? Should we just let that happen?

@dramforever
Copy link
Contributor Author

I do handle strings. These are everywhere and it wouldn't work without.

I think configfile is supposed to come after patches have been applied and in fact completely override patch options? You just give it a .config file instead of extraConfig in the format expected by nixpkgs. At least the old one didn't bother.

You're right about the last point, we should probably just ignore allowImportFromDerivation flag altogether, but idk if that would be good, or if we should just delete it altogether.

@tpwrules
Copy link
Contributor

tpwrules commented Apr 9, 2024

I was just commenting that one of the uncertainties I had about my changes is whether or not it handled all varieties of string quoting (if multiple are even supported) in all circumstances, etc. It hasn't been tested or examined very thoroughly.

It does look like in the normal case extraConfig in patches is handled by the generic configuration stuff. I had to add it separately because my stuff doesn't use that (though it should in the future).

It's unclear if the config passed by the generic builder is even right?

I think this PR is okay, it's just not as much as I'd like. It's a step in the right direction though. I need to double check that it works properly with my stuff in that repo.

@dramforever
Copy link
Contributor Author

ah, yes, i almost forgot. i just delegated string parsing to builtins.fromJSON which may or may not be a bad idea.... i think the string extraction regex should be correct, but the parsing may need some more care for people who have say quotes in cflags or something.

@tpwrules
Copy link
Contributor

tpwrules commented Apr 9, 2024

I'm trying to look if there's a defined syntax for that, I don't know what the possibilities are. My code just strips the outer quotes and assumes there's none within.

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.

3 participants