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

nix/tests: run test help.sh only if nix is built with documentation #11729

Merged
merged 1 commit into from
Oct 23, 2024

Conversation

emhamm
Copy link
Contributor

@emhamm emhamm commented Oct 22, 2024

Motivation

tests/functional/help.sh calls nix-* commands with option --help
if nix is built without documentation the option --help throws an error because the man page it wants to display is missing

Context

With disabled docs nix can not be built because of a failing test.

Priorities and Process

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

@emhamm emhamm requested a review from edolstra as a code owner October 22, 2024 09:41
@github-actions github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label Oct 22, 2024
Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

One thing, and then LGTM

package.nix Outdated
Comment on lines 233 to 234
] ++ lib.optionals enableManual [
man
Copy link
Member

Choose a reason for hiding this comment

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

Since this is for execution ("native") during the tests, it should stay in nativeBuildInputs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, done.

My rational was, that man is needed for displaying the man-page anyway so i did put it into BuildInputs as i thought it will be a runtime dependency. Or am i wrong about this? I mean this code:

void showManPage(const std::string & name)

Or did i miss something?

Copy link
Member

@Mic92 Mic92 Oct 22, 2024

Choose a reason for hiding this comment

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

Adding it buildInputs doesn't make it a runtime dependency automatically. Using something like makeWrapper does. But I am unsure if we want to make nix depending on man all the time... It's only used for interactive usage, but nix will be also downloaded in many other places i.e. in CI environments.

@roberth also why nativeBuildInputs and not checkInputs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the clarification!
The help command will fail if man is disabled in the config then.
Should I add a wrapper or just leave it as it is and make this an issue for the future?

Copy link
Member

Choose a reason for hiding this comment

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

No I wouldn't add a wrapper and keep it an optional run time dependency for now.

Copy link
Member

Choose a reason for hiding this comment

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

why nativeBuildInputs and not checkInputs?

Because I haven't internalized this 2 year old parameter yet, but then it should be nativeInstallCheckInputs. That's the most accurate role for this, and iiuc it would let us get rid of the lib.optionals doInstallCheck [ bit in nativeBuildInputs.

Let's fix it in the old way first, and then we can refactor it more later without increasing the scope of this PR.

@roberth roberth added backport 2.18-maintenance Automatically creates a PR against the branch backport 2.24-maintenance Automatically creates a PR against the branch build-problem Nix fails to compile or test; also improvements to build process labels Oct 22, 2024
tests/functional/help.sh calls nix-* commands with option --help
if nix is built without documentation the option --help throws an error
because the man page it wants to display is missing
@emhamm emhamm force-pushed the nix-tests-help-only-if-docu branch from 108d90d to 85b0cd3 Compare October 22, 2024 12:16
@roberth roberth merged commit 3db75b0 into NixOS:master Oct 23, 2024
11 checks passed
@roberth
Copy link
Member

roberth commented Oct 23, 2024

Thank you @emhamm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.18-maintenance Automatically creates a PR against the branch backport 2.24-maintenance Automatically creates a PR against the branch build-problem Nix fails to compile or test; also improvements to build process with-tests Issues related to testing. PRs with tests have some priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants