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

nixos/davfs2: fix rfc42 conversion, make settings and extraConfig mutually exclusive, and other cleanup #302689

Merged
merged 7 commits into from
Apr 12, 2024

Conversation

eclairevoyant
Copy link
Contributor

@eclairevoyant eclairevoyant commented Apr 8, 2024

Description of changes

Fixes #302243.
Also fixes some additional issue around spurious warnings, mentioned in the comments of #297014.

Also, update the settings option to allow multiple sections (breaking change), and ensure it's mutually exclusive with extraConfig (breaking change).

Finally, update the config generation to conform to the following rules:

General Syntax Rules

Lines that only contain spaces and tabs (empty lines) are ignored.

# indicates a comment. The rest of the line is ignored.

\ is the escape character.

"" is used for quotation.

If a value contains one of the special characters space, tab, #, \, or ", this character must be escaped by a preceding \. Use \ instead of , \# instead of #, \\ instead of \ and \" instead of ".

Values containing spaces, tabs or # may instead be enclosed in double quotes. But " and must be escaped even within double quotes. If the starting line of a section is enclosed in double quotes, the square brakets must be within the quotes (like "[/home/otto/with space]").

Boolean option values (yes/no) must be given as numerical value. 0 for no, 1 for yes.

  • fix the warning that deprecates extraConfig
  • clarify warning message
  • remove lib.mdDoc (no-op)
  • remove with lib;
  • fix settings option
  • ensure extraConfig and settings are mutually exclusive

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.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Apr 8, 2024
@eclairevoyant eclairevoyant changed the title nixos/davfs2: fix rfc42 conversion nixos/davfs2: fix rfc42 conversion, and other cleanup Apr 8, 2024
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 1-10 labels Apr 8, 2024
@eclairevoyant eclairevoyant marked this pull request as draft April 8, 2024 23:21
@eclairevoyant eclairevoyant added the 2.status: work-in-progress This PR isn't done label Apr 10, 2024
@eclairevoyant eclairevoyant force-pushed the davfs2-fix branch 5 times, most recently from bc482ad to 6cb5a0f Compare April 10, 2024 08:07
@eclairevoyant eclairevoyant removed the 2.status: work-in-progress This PR isn't done label Apr 10, 2024
@eclairevoyant eclairevoyant marked this pull request as ready for review April 10, 2024 08:12
@eclairevoyant
Copy link
Contributor Author

@firecat53 would you be able to test this? I've verified that (at least with only specifying settings and not extraConfig) the config format follows the manual, and doesn't spit out warnings when building, but I'm not familiar with davfs.

@happysalada
Copy link
Contributor

This looks good to me, i have to be honest i dont have a lot of time to spend here. Ill check again in a few days and will merge (ideally when someone has been able to test)

@firecat53
Copy link

firecat53 commented Apr 10, 2024 via email

@eclairevoyant eclairevoyant marked this pull request as draft April 11, 2024 07:27
@firecat53
Copy link

Thanks for your patience! Finally figured out how to test and it works for me. Appreciate the support!

@therealpxc
Copy link
Contributor

Thanks for your patience! Finally figured out how to test and it works for me. Appreciate the support!

👋 I was just about to ask you on Discourse to come here and share your experience with the PR to help move it forward, but I see now that's what you were working to do all along. Thanks :)

@eclairevoyant eclairevoyant marked this pull request as ready for review April 11, 2024 20:09
@eclairevoyant eclairevoyant changed the title nixos/davfs2: fix rfc42 conversion, and other cleanup nixos/davfs2: fix rfc42 conversion, make settings and extraConfig mutually exclusive, and other cleanup Apr 11, 2024
@eclairevoyant
Copy link
Contributor Author

Okay, I think the manpage is wrong; it claims that " and ' need to be escaped when quoting, but I don't see a reason for escaping ' - they probably intended to say that " and \ should be escaped, and messed up a copy-paste somewhere...

@happysalada
Copy link
Contributor

@eclairvoyant are we good to go , or do you want to make further changes ?

@eclairevoyant
Copy link
Contributor Author

Fixed it to escape double-quotes and backslashes. I think that makes far more sense given the context of the manpage.

Should be good to go now.

@happysalada happysalada merged commit b008f50 into NixOS:master Apr 12, 2024
21 checks passed
@eclairevoyant eclairevoyant deleted the davfs2-fix branch April 12, 2024 23:05
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/can-we-please-stop-breaking-stuff-willy-nilly/48496/39

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: clean-up 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

davfs2.conf created with = between keys and values which causes mount failure
5 participants