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

make-options-doc: fix string context issues #73966

Merged
merged 1 commit into from Nov 25, 2019

Conversation

@d-goldin
Copy link
Contributor

d-goldin commented Nov 23, 2019

Motivation for this change

When using documentation.nixos.includeAllModules = true; with external
modules, the string context might contain dependencies to derivations
and so toFile refuses to evaluate;

error: in 'toFile': the file 'options.xml' cannot refer to derivation outputs, at
[...]/nixpkgs/nixos/lib/make-options-doc/default.nix:89:16

This is not an issue when using writeText (instead of manually
stripping the context).

Possibly addresses: #73743

For my particular configuration utilizing home-manager this fix is insufficient, because some of the formatting for some home-manager options fail validation;

manual-combined.xml:19469: element link: validity error : IDREF attribute linkend references an unknown ID "opt-xsession.windowManager.i3.config.assigns"
[...]

But maybe this is one step forward anyways.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @domenkozar

When using `documentation.nixos.includeAllModules = true;` with external
modules, the string context might contain dependencies to derivations
and so `toFile` refuses to evaluate;

```
error: in 'toFile': the file 'options.xml' cannot refer to derivation outputs, at
[...]/nixpkgs/nixos/lib/make-options-doc/default.nix:89:16
```

This is not an issue when using `writeText` (instead of manually
stripping the context).
@domenkozar
Copy link
Member

domenkozar commented Nov 25, 2019

iirc this will fail regardless, there should be no /nix/store paths in the manual (there's a check for that).

@d-goldin
Copy link
Contributor Author

d-goldin commented Nov 25, 2019

@domenkozar: I don't know those parts too well yet, but saw the check. The check emits a better error message in comparison to the toFile error. I thought it would make sense to get this out of the way in the context of documentation.nixos.includeAllModules to maybe reduce confusion.

Maybe it would also make sense to include some additional pointers of how to go about it - or some type of flag to ignore if it's just for local use in the manpage.

Otoh, in general it seems to me like It greatly reduces usefulness if one would have to go and fix upstream modules from repos like home-manager, just to have the options being shown in the manpage.

What do you think?

@domenkozar
Copy link
Member

domenkozar commented Nov 25, 2019

I agree about the error.

@domenkozar domenkozar merged commit 2c8c199 into NixOS:master Nov 25, 2019
12 checks passed
12 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@d-goldin d-goldin deleted the d-goldin:nixosoptions_xml_strctx branch Nov 25, 2019
@edolstra
Copy link
Member

edolstra commented Apr 1, 2020

This change causes a giant options.xml.drv file to be generated (#83863). Aside from that, this fix seems wrong since options.xml shouldn't depend on derivations to begin with! After all, it can things to be built that wouldn't otherwise be built. (This is why we have literalExample, BTW.)

@domenkozar
Copy link
Member

domenkozar commented Apr 1, 2020

+1 and add a comment.

@jtojnar
Copy link
Contributor

jtojnar commented Apr 1, 2020

Reverted in 513cec9 and cab6b01, backported to 20.03 in 9412ae3. Verified that jobsets now evaluate.

@worldofpeace
Copy link
Member

worldofpeace commented Apr 29, 2020

@edolstra This was an issue for me in cc94d5e.

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

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.