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

stdenv: Disallow output changes in installCheck #143862

Closed

Conversation

roberth
Copy link
Member

@roberth roberth commented Oct 30, 2021

Motivation for this change

Spot installation script problems and avoid tests polluting the installation.

This will indicate errors in the install script fixed up by the
installed software, or software or configuration that relies on
a writable installation directory.

NOTE: this only works for derivations which use the installCheck phase as intended. Some language integrations, like pythonPackages, need to call the new functions on their own.

Closes #120053

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 via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • 21.11 Release Notes (or backporting 21.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.

@roberth roberth force-pushed the disallow-output-changes-in-installCheck branch 2 times, most recently from 17e5af2 to f395531 Compare October 30, 2021 19:31
@symphorien
Copy link
Member

Maybe we can chmod -w the outputs before installCheckPhase as well. What do you think ?

@roberth
Copy link
Member Author

roberth commented Oct 31, 2021

@symphorien I think we should, but it's not as simple as that because we'll have to restore the permissions, as distPhase needs to write to outputs again. I'd like to keep it simple for now.

@roberth
Copy link
Member Author

roberth commented Mar 24, 2022

/marvin opt-in
/status needs_reviewer

@marvin-mk2 marvin-mk2 bot added the marvin This PR was reviewed by Marvin, a discontinued bot: https://github.com/timokau/marvin-mk2 label Mar 24, 2022
@marvin-mk2
Copy link

marvin-mk2 bot commented Mar 24, 2022

Hi! I'm an experimental bot. My goal is to guide this PR through its stages, hopefully ending with a merge. You can read up on the usage here.

@asymmetric
Copy link
Contributor

@roberth how can i test this? I tried:

❯ nix build .#tests
error: flake output attribute 'legacyPackages.x86_64-linux.tests' is not a derivation

There are some ofborg eval failures, any idea why that is?

Sorry that I can't provide any more direct help at the moment.

@roberth
Copy link
Member Author

roberth commented Mar 31, 2022

nix build .#tests

Tests contains a nested attrset of derivations. Try with nix-build -A tests, which traverses those attrs and doesn't copy all of your local nixpkgs into the store.

ofborg eval failures

I don't think it's technically an eval failure, but rather that the eval status combines the other statuses. The lib-tests job did fail because it timed out rebuilding the world.

You could "just" build a package, rebuilding whatever needs to rebuild. A lot. If the build works, the pr works, but I don't know if this will impact many subtly broken tests. Maybe this pr should run separately on hydra first?

@marvin-mk2

This comment was marked as spam.

@symphorien
Copy link
Member

This needs doc for allowWriteInInstallCheck and I think this is too late for 22.05 given the feature freeze #167025

@symphorien
Copy link
Member

/status awaiting_changes

@timokau
Copy link
Member

timokau commented Apr 16, 2022

Hi!

The marvin-mk2 bot is now discontinued. I have removed the relevant tags from this PR. If you still need someone to look at it, one option would be to ask in this discourse thread.

I am posting this notice to all open PRs with the marvin tag. Please understand that following all of these discussions would take too much time, so I will unsubscribe from this PR unless I have already been involved in it before this message.

@timokau timokau removed marvin This PR was reviewed by Marvin, a discontinued bot: https://github.com/timokau/marvin-mk2 awaiting_changes labels Apr 16, 2022
@roberth roberth requested review from dasJ, infinisil, Mic92, zowoq and a team as code owners April 11, 2024 13:22
@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: GNOME GNOME desktop environment and its underlying platform 6.topic: haskell 6.topic: qt/kde 6.topic: kernel The Linux kernel 8.has: documentation 8.has: module (update) This PR changes an existing module in `nixos/` 6.topic: policy discussion 6.topic: vim 6.topic: ocaml 6.topic: xfce The Xfce Desktop Environment 6.topic: nodejs 6.topic: pantheon The Pantheon desktop environment 6.topic: cinnamon Desktop environment 6.topic: module system About "NixOS" module system internals 6.topic: systemd 6.topic: Lumina DE The Lumina Desktop Environment 6.topic: Enlightenment DE The Enlightenment Desktop Environment 6.topic: mate The MATE Desktop Environment 6.topic: vscode 6.topic: lib The Nixpkgs function library labels Apr 11, 2024
@roberth
Copy link
Member Author

roberth commented Apr 11, 2024

I did the thing.

If you're interested, this is continued in

@roberth roberth closed this Apr 11, 2024
@NixOS NixOS locked and limited conversation to collaborators Apr 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
0.kind: enhancement Add something new 2.status: merge conflict This PR has merge conflicts with the target branch 6.topic: cinnamon Desktop environment 6.topic: Enlightenment DE The Enlightenment Desktop Environment 6.topic: GNOME GNOME desktop environment and its underlying platform 6.topic: haskell 6.topic: hygiene 6.topic: kernel The Linux kernel 6.topic: lib The Nixpkgs function library 6.topic: Lumina DE The Lumina Desktop Environment 6.topic: mate The MATE Desktop Environment 6.topic: module system About "NixOS" module system internals 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: nodejs 6.topic: ocaml 6.topic: pantheon The Pantheon desktop environment 6.topic: policy discussion 6.topic: python 6.topic: qt/kde 6.topic: reproducible builds 6.topic: stdenv Standard environment 6.topic: systemd 6.topic: vim 6.topic: vscode 6.topic: xfce The Xfce Desktop Environment 8.has: documentation 8.has: module (update) This PR changes an existing module in `nixos/`
Projects
None yet
Development

Successfully merging this pull request may close these issues.