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/doc: dedocbookify #237557

Merged
merged 14 commits into from
Jun 19, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 0 additions & 21 deletions .github/workflows/compare-manuals.sh

This file was deleted.

10 changes: 1 addition & 9 deletions .github/workflows/manual-nixos.yml
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,5 @@ jobs:
# This cache is for the nixpkgs repo checks and should not be trusted or used elsewhere.
name: nixpkgs-ci
signingKey: '${{ secrets.CACHIX_SIGNING_KEY }}'
- name: Building NixOS manual with DocBook options
- name: Building NixOS manual
run: NIX_PATH=nixpkgs=$(pwd) nix-build --option restrict-eval true nixos/release.nix -A manual.x86_64-linux
- name: Building NixOS manual with Markdown options
run: |
export NIX_PATH=nixpkgs=$(pwd)
nix-build \
--option restrict-eval true \
--arg configuration '{ documentation.nixos.options.allowDocBook = false; }' \
nixos/release.nix \
-A manual.x86_64-linux
64 changes: 0 additions & 64 deletions .github/workflows/manual-rendering.yml

This file was deleted.

2 changes: 1 addition & 1 deletion lib/default.nix
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ let
mergeDefaultOption mergeOneOption mergeEqualOption mergeUniqueOption
getValues getFiles
optionAttrSetToDocList optionAttrSetToDocList'
scrubOptionValue literalExpression literalExample literalDocBook
scrubOptionValue literalExpression literalExample
showOption showOptionWithDefLocs showFiles
unknownModule mkOption mkPackageOption mkPackageOptionMD
mdDoc literalMD;
Expand Down
24 changes: 7 additions & 17 deletions lib/modules.nix
Original file line number Diff line number Diff line change
Expand Up @@ -134,11 +134,6 @@ let
${if prefix == []
then null # unset => visible
else "internal"} = true;
# TODO: hidden during the markdown transition to not expose downstream
# users of the docs infra to markdown if they're not ready for it.
# we don't make this visible conditionally because it can impact
# performance (https://github.com/NixOS/nixpkgs/pull/208407#issuecomment-1368246192)
visible = false;
Comment on lines -137 to -141
Copy link
Member

Choose a reason for hiding this comment

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

Home Manager is still not ready to properly render Markdown.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we're a bit less inclined to hide it again this time since the issue has been known for months (and, as nix-darwin has found, disabling the docs build is sufficient to unbreak everything). if you want it hidden again we'd merge it, but we'd much prefer HM fixed its docs system :(

Copy link
Member

Choose a reason for hiding this comment

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

I'd also much prefer that, but I'm not sure exactly what needs to happen and I don't have the energy to figure it out. You made suggestions in nix-community/home-manager#3680 (comment) , do you want to try to open a MR at https://gitlab.com/rycee/nmd/ so that we can check if it resolves the breakage, now that we have a reproducer?

Copy link
Member

Choose a reason for hiding this comment

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

Looks like it's been taken care of :)

# TODO: Change the type of this option to a submodule with a
# freeformType, so that individual arguments can be documented
# separately
Expand Down Expand Up @@ -1146,14 +1141,11 @@ let
use = id;
};

/* Transitional version of mkAliasOptionModule that uses MD docs. */
mkAliasOptionModuleMD = from: to: doRename {
inherit from to;
visible = true;
warn = false;
use = id;
markdown = true;
};
/* Transitional version of mkAliasOptionModule that uses MD docs.

This function is no longer necessary and merely an alias of `mkAliasOptionModule`.
*/
mkAliasOptionModuleMD = mkAliasOptionModule;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should mkAliasOptionModuleMD be marked as deprecated? I can't find information about deprecating functions in the manual, but maybe a simple wrapper which prints a deprecation message could be used here?

Ditto for the other no-op aliases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that would be a bit too soon. deprecating those would require another round through the entire modules tree to remove them again, or folks would get unhelpful deprecation warnings they cannot act upon. once that's done we can deprecate them.

Copy link
Contributor

Choose a reason for hiding this comment

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

There's only ~10 calls to mkAliasOptionModuleMD in nixpkgs master. I'd say ideally we'd just replace all calls in-tree with mkAliasOptionModule in this PR, and then do another PR to remove mkAliasOptionModuleMD itself. Otherwise this deprecated function could stay around for a long time until someone takes the time to clean up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there may be uses out of tree we don't know about. keeping the alias doesn't hurt while we're not sure whether it's used anywhere else, and we'd really rather keep this pr focused on removing docbook instead of tacking on refactorings that removing docbook allows to happen.

Copy link
Contributor

@l0b0 l0b0 Jun 14, 2023

Choose a reason for hiding this comment

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

keeping the alias doesn't hurt while we're not sure whether it's used anywhere else

I wasn't suggesting removing the alias in this PR, just calling mkAliasOptionModule instead of mkAliasOptionModuleMD everywhere. This would mean no actual changes in functionality, while making sure mkAliasOptionModuleMD only shows up in one place, meaning it's easy to deprecate later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we can't add loud deprecation warnings to any of these public apis before all supported nixos versions agree on what the documentation language is, otherwise we'll risk breaking out-of-tree modules, backports (official in nixpkgs and unofficial by users), and who knows what else. this applies to all functions that are now aliases or no-ops.

refactorings in nixpkgs modules are less critical, but we're not going to do any. out-of-tree modules wouldn't be affected by this, but backports would be, and we're frankly sick of refactoring docs. if you want to see this done you're totally free to open a follow-up pr once this is done, but we won't be doing it.

Copy link
Contributor

@l0b0 l0b0 Jun 14, 2023

Choose a reason for hiding this comment

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

we can't add loud deprecation warnings to any of these public apis before all supported nixos versions agree on what the documentation language is, otherwise we'll risk breaking out-of-tree modules, backports (official in nixpkgs and unofficial by users), and who knows what else. this applies to all functions that are now aliases or no-ops.

I think something must've been lost in translation, because my latest comment had nothing to do with deprecation. Basically just sed -i -e s/mkAliasOptionModuleMD/mkAliasOptionModule/g in all files except for the mkAliasOptionModuleMD = mkAliasOptionModule; line itself. This changes no functionality anywhere, doesn't introduce any deprecation warnings, and makes it pretty obvious that we can remove mkAliasOptionModuleMD ASAP.

refactorings in nixpkgs modules are less critical, but we're not going to do any. out-of-tree modules wouldn't be affected by this, but backports would be, and we're frankly sick of refactoring docs. if you want to see this done you're totally free to open a follow-up pr once this is done, but we won't be doing it.

Fair enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This changes no functionality anywhere, doesn't introduce any deprecation warnings, and makes it pretty obvious that we can remove mkAliasOptionModuleMD ASAP.

this does turn backports of newly aliased options into a trap though because 23.05 uses a different language to render the non-MD variant text, potentially causing the docs build on 23.05 to fail as a result. the manual isn't checked by CI on stable branches, so this has the potential to create channel blockers. refactoring shouldn't come with that risk.

I think something must've been lost in translation, because my latest comment had nothing to do with deprecation.

yes, but the point is that deprecation is a ways out still. worrying about how it might go now may not be the wisest use of resources, especially if the changes aren't even finalized yet.


/* mkDerivedConfig : Option a -> (a -> Definition b) -> Definition b

Expand All @@ -1175,7 +1167,7 @@ let
(opt.highestPrio or defaultOverridePriority)
(f opt.value);

doRename = { from, to, visible, warn, use, withPriority ? true, markdown ? false }:
doRename = { from, to, visible, warn, use, withPriority ? true }:
{ config, options, ... }:
let
fromOpt = getAttrFromPath from options;
Expand All @@ -1186,9 +1178,7 @@ let
{
options = setAttrByPath from (mkOption {
inherit visible;
description = if markdown
then lib.mdDoc "Alias of {option}`${showOption to}`."
else "Alias of <option>${showOption to}</option>.";
description = "Alias of {option}`${showOption to}`.";
apply = x: use (toOf config);
} // optionalAttrs (toType != null) {
type = toType;
Expand Down
33 changes: 8 additions & 25 deletions lib/options.nix
Original file line number Diff line number Diff line change
Expand Up @@ -100,10 +100,7 @@ rec {
name: mkOption {
default = false;
example = true;
description =
if name ? _type && name._type == "mdDoc"
then lib.mdDoc "Whether to enable ${name.text}."
else "Whether to enable ${name}.";
description = "Whether to enable ${name}.";
type = lib.types.bool;
};

Expand Down Expand Up @@ -185,10 +182,10 @@ rec {
(if isList example then "pkgs." + concatStringsSep "." example else example);
});

/* Like mkPackageOption, but emit an mdDoc description instead of DocBook. */
mkPackageOptionMD = pkgs: name: extra:
let option = mkPackageOption pkgs name extra;
in option // { description = lib.mdDoc option.description; };
/* Alias of mkPackageOption. Previously used to create options with markdown
documentation, which is no longer required.
*/
mkPackageOptionMD = mkPackageOption;

/* This option accepts anything, but it does not produce any result.

Expand Down Expand Up @@ -344,26 +341,12 @@ rec {
if ! isString text then throw "literalExpression expects a string."
else { _type = "literalExpression"; inherit text; };

literalExample = lib.warn "literalExample is deprecated, use literalExpression instead, or use literalDocBook for a non-Nix description." literalExpression;


/* For use in the `defaultText` and `example` option attributes. Causes the
given DocBook text to be inserted verbatim in the documentation, for when
a `literalExpression` would be too hard to read.
*/
literalDocBook = text:
if ! isString text then throw "literalDocBook expects a string."
else
lib.warnIf (lib.isInOldestRelease 2211)
"literalDocBook is deprecated, use literalMD instead"
{ _type = "literalDocBook"; inherit text; };
literalExample = lib.warn "literalExample is deprecated, use literalExpression instead, or use literalMD for a non-Nix description." literalExpression;

/* Transition marker for documentation that's already migrated to markdown
syntax.
syntax. This is a no-op and no longer needed.
*/
mdDoc = text:
if ! isString text then throw "mdDoc expects a string."
else { _type = "mdDoc"; inherit text; };
mdDoc = lib.id;

/* For use in the `defaultText` and `example` option attributes. Causes the
given MD text to be inserted verbatim in the documentation, for when
Expand Down
Loading