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

doc: markdown transition fixes #208407

Merged
merged 3 commits into from
Jan 5, 2023
Merged

doc: markdown transition fixes #208407

merged 3 commits into from
Jan 5, 2023

Conversation

pennae
Copy link
Contributor

@pennae pennae commented Dec 30, 2022

Description of changes

#175586 (comment) reported problem for downstream users of the docs infra. changes to the option factories shouldn't be controversial, but the change to _module.args is different. not sure whether this is a good way to solve the problem, very much open to other ideas.

should probably be backported to 22.11 in the end

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 23.05 Release Notes (or backporting 22.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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@ncfavier
Copy link
Member

A possible alternative to adding more *MD variants would be to treat mdDoc as a generic Markdown marker:

  • mkPackageOption pkgs (mdDoc "foobar")
  • mkAliasOptionModule [ "old" ] (mdDoc [ "new" ]) or something (this is probably terrible)

Independently from this PR, we probably want to make _module.*'s visibility opt-in anyway: see NixOS/nixos-search#528. Standalone NixOS module docs like https://nix-community.github.io/home-manager/nixos-options.html don't really need to include _module.args. In fact I think ideally _module.* options should be documented somewhere in the main NixOS manual rather than in the "configuration options" annex. But for now, maybe we can just use specialArgs._moduleVisible and set _module.*.visible based on that?

@pennae
Copy link
Contributor Author

pennae commented Dec 31, 2022

A possible alternative to adding more *MD variants would be to treat mdDoc as a generic Markdown marker

had thought about that, but that would somewhat violate the current invariant that mdDoc wraps only text. it'd be okay here because it's immediately unwrapped again, but we'd rather avoid that inconsistency. (mkEnableOption uses this approach instead of an *MD variant because there mdDoc does wrap only text)

But for now, maybe we can just use specialArgs._moduleVisible and set _module.*.visible based on that?

certainly, but we'd still want a more elaborate name for the new specialArg just to be extra safe there's no collisions with users.

@roberth
Copy link
Member

roberth commented Dec 31, 2022

I think we should try to keep it simple for two reasons

  • the markdown transition is a temporary state of affairs
  • _module is added to every module, including all submodules, so doing anything extra there will have a sizable performance impact

Also simple is good.
I'd be ok with hiding _module.args docs unconditionally for now, and enable it later. (Sorry @infinisil)

@ncfavier
Copy link
Member

ncfavier commented Jan 2, 2023

We should probably mention mkAliasOptionModuleMD and mkPackageOptionMD in the error message in mergeJSON.py

@pennae
Copy link
Contributor Author

pennae commented Jan 2, 2023

added mentions of those functions. whitespace separating the examples is not missing, we purposely omitted it and grouped the last three examples together to have the error message still fit within nix's "last 10 log lines" excerpt.

mkAliasOptionModule should not default to mdDoc descriptions because
that can break out-of-tree users of documentation infrastructure. add an
explicitly-MD variant for now, to be removed some time after the MD
transition is complete.
another transitional option factory, like mkAliasOptionModuleMD.
unfortunately we can't unconditionally make this text markdown without
impacting downstream users of docs generation (as noted in #175586).
hide it entirely until the transition is complete.
@pennae
Copy link
Contributor Author

pennae commented Jan 2, 2023

and rebased to master to fix a module that has appeared since too 😓

Copy link
Member

@ncfavier ncfavier left a comment

Choose a reason for hiding this comment

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

Half-hearted approval because the transitional period is taking way too long :/

@pennae
Copy link
Contributor Author

pennae commented Jan 2, 2023

Half-hearted approval because the transitional period is taking way too long :/

well, we're going to be at it for another two years or so either way at the current speed ...

@pennae pennae merged commit e912c7b into NixOS:master Jan 5, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jan 5, 2023

Backport failed for release-22.11, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally.

git fetch origin release-22.11
git worktree add -d .worktree/backport-208407-to-release-22.11 origin/release-22.11
cd .worktree/backport-208407-to-release-22.11
git checkout -b backport-208407-to-release-22.11
ancref=$(git merge-base c735db25e8fc3c4bb54d17db15d7049a3c593062 3c575a659f10a8564a1c4a661570ee933e31ea2e)
git cherry-pick -x $ancref..3c575a659f10a8564a1c4a661570ee933e31ea2e

@zowoq
Copy link
Contributor

zowoq commented Jan 5, 2023

https://github.com/NixOS/nixpkgs/actions/runs/3843384881/jobs/6545596726

Manual checks are failing since this was merged.

@pennae
Copy link
Contributor Author

pennae commented Jan 5, 2023

@zowoq #209130

ncfavier added a commit to ncfavier/nixpkgs that referenced this pull request Jan 8, 2023
Follow-up to NixOS#208407

Removing `mdDoc` isn't enough, we need to emit actual DocBook.
github-actions bot pushed a commit that referenced this pull request Jan 9, 2023
Follow-up to #208407

Removing `mdDoc` isn't enough, we need to emit actual DocBook.

(cherry picked from commit fb1bc8d)
github-actions bot pushed a commit to arcnmx/nixpkgs-lib that referenced this pull request Jan 10, 2023
Follow-up to NixOS/nixpkgs#208407

Removing `mdDoc` isn't enough, we need to emit actual DocBook.
@pennae pennae deleted the mddoc-fixes branch January 14, 2023 09:32
github-actions bot pushed a commit to nix-community/nixpkgs.lib that referenced this pull request Jan 15, 2023
Follow-up to NixOS/nixpkgs#208407

Removing `mdDoc` isn't enough, we need to emit actual DocBook.
adamcstephens pushed a commit to adamcstephens/nixpkgs that referenced this pull request Jan 25, 2023
Follow-up to NixOS#208407

Removing `mdDoc` isn't enough, we need to emit actual DocBook.
@schnusch schnusch mentioned this pull request Feb 8, 2023
13 tasks
github-actions bot pushed a commit to nix-community/nixpkgs.lib that referenced this pull request May 31, 2023
Follow-up to NixOS/nixpkgs#208407

Removing `mdDoc` isn't enough, we need to emit actual DocBook.
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.

4 participants