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

buildEnv: support ignoreSingleFileOutputs (resumed) #353752

Merged

Conversation

ShamrockLee
Copy link
Contributor

@ShamrockLee ShamrockLee commented Nov 5, 2024

Add option to ignore single-file output instead of failing, which would be useful to fix Mic92/nixpkgs-review#408.

Cc: @corngood

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.11 Release Notes (or backporting 23.11 and 24.05 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.

Copy link
Member

@Mic92 Mic92 left a comment

Choose a reason for hiding this comment

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

Would be cool to have this because it's not possible from pure nix to know wether a derivation is a single file.

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Nov 5, 2024
@ShamrockLee
Copy link
Contributor Author

Would be cool to have this because it's not possible from pure nix to know wether a derivation is a single file.

I think we should enforce a policy to exclude all single-file outputs from meta.outputsToInstall for all packages exposed under pkgs with recurseIntoAttrs in the long run, but it is also crucial for buildEnv to be flexible enough to ignore those single-file outputs.

@philiptaron
Copy link
Contributor

@ShamrockLee, I'd like to merge this once the merge conflict is resolved.

@ShamrockLee ShamrockLee force-pushed the buildenv-ignore-file-outputs branch from 793c7f2 to 3843aed Compare December 2, 2024 17:59
@github-actions github-actions bot added 6.topic: python 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: haskell 8.has: documentation This PR adds or changes documentation 8.has: changelog 8.has: module (update) This PR changes an existing module in `nixos/` 6.topic: vim 6.topic: module system About "NixOS" module system internals 6.topic: docker tools 6.topic: vscode 6.topic: lib The Nixpkgs function library 6.topic: jupyter Interactive computing tooling: kernels, notebook, jupyterlab 6.topic: php 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` 6.topic: xen-project The Xen Project hypervisor labels Dec 2, 2024
@nix-owners
Copy link

nix-owners bot commented Dec 2, 2024

The PR's base branch is set to staging, but 248 commits from the master branch are included. Make sure you know the right base branch for your changes, then:

  • If the changes should go to the master branch, change the base branch to master
  • If the changes should go to the staging branch, rebase your PR onto the merge base with the staging branch:
    # git rebase --onto $(git merge-base upstream/staging HEAD) $(git merge-base upstream/master HEAD)
    git rebase --onto 7746ea988939fe49dfb60d917dae7ca55ba16afc cd519af995cc5fa35c594751a42a02ba6b705ee6
    git push --force-with-lease

@ShamrockLee ShamrockLee force-pushed the buildenv-ignore-file-outputs branch from 3843aed to 6bacc2f Compare December 2, 2024 18:03
@github-actions github-actions bot removed 8.has: documentation This PR adds or changes documentation 8.has: changelog 8.has: module (update) This PR changes an existing module in `nixos/` 6.topic: vim 6.topic: module system About "NixOS" module system internals 6.topic: docker tools 6.topic: vscode 6.topic: lib The Nixpkgs function library 6.topic: jupyter Interactive computing tooling: kernels, notebook, jupyterlab 6.topic: php 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` 6.topic: xen-project The Xen Project hypervisor labels Dec 2, 2024
@ShamrockLee
Copy link
Contributor Author

@philiptaron Rebased.

@ShamrockLee
Copy link
Contributor Author

ShamrockLee commented Dec 2, 2024

@Mic92 @philiptaron Regarding the argument name, do you think it should be ignoreFileOutputs or ignoreSingleFileOutputs?

@wegank wegank removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Dec 2, 2024
@philiptaron
Copy link
Contributor

I'm amenable to both, but ignoreSingleFileOutputs does more clearly say what it's doing.

@ShamrockLee ShamrockLee force-pushed the buildenv-ignore-file-outputs branch from 6bacc2f to 5ca3e70 Compare December 3, 2024 04:04
@ShamrockLee
Copy link
Contributor Author

Renamed!

@ShamrockLee ShamrockLee changed the title buildEnv: support ignoreFileOutputs (resumed) buildEnv: support ignoreSingleFileOutputs (resumed) Dec 3, 2024
Copy link
Contributor

@philiptaron philiptaron left a comment

Choose a reason for hiding this comment

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

One copy-editing nit on a comment.

pkgs/build-support/buildenv/default.nix Outdated Show resolved Hide resolved
Add option to ignore single-file output instead of failing.
@ShamrockLee ShamrockLee force-pushed the buildenv-ignore-file-outputs branch from 83eb710 to 70038f0 Compare December 3, 2024 04:14
@ShamrockLee
Copy link
Contributor Author

Rebasd and squashed the commit.

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