Skip to content

opam: fix opam sandboxing on nixos#363770

Merged
GaetanLepage merged 1 commit intoNixOS:masterfrom
eilvelia:opam
Jan 10, 2025
Merged

opam: fix opam sandboxing on nixos#363770
GaetanLepage merged 1 commit intoNixOS:masterfrom
eilvelia:opam

Conversation

@eilvelia
Copy link
Copy Markdown
Member

@eilvelia eilvelia commented Dec 10, 2024 •
edited
Loading

Before this commit, executing opam init would display that sandboxing fails with "bwrap: execvp sh: No such file or directory". makeWrapper with --set OPAM_USER_PATH_RO /run/current-system/sw/bin:/nix/ had been used here to fix that, however, OPAM_USER_PATH_RO has been removed since 2021: ocaml/opam@9b6370d (released as opam 2.2.0 in 2024)

This also fixes a funny bug which caused the changelog link to be broken because of with lib:

meta = with lib; {                                                   
  description = "Package manager for OCaml";                         
  homepage = "https://opam.ocaml.org/";                              
  changelog = "https://github.com/ocaml/opam/raw/${version}/CHANGES";
  maintainers = [ ];                                                 
  license = licenses.lgpl21Only;                                     
  platforms = platforms.all;                                         
};

It uses meta = with lib;; at the same time, the derivation's attrset is not rec, and ${version} ends to be lib.version.

cc @kit-ty-kate

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/)
  • 25.05 Release Notes (or backporting 24.11 and 25.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.

@github-actions github-actions bot added 6.topic: ocaml OCaml is a general-purpose, high-level, multi-paradigm programming language. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Dec 10, 2024
@eilvelia
Copy link
Copy Markdown
Member Author

eilvelia commented Dec 10, 2024

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 363770

x86_64-linux

✅ 3 packages built:
  • dune-release
  • opam
  • opam.installer

kit-ty-kate
kit-ty-kate approved these changes Dec 10, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@kit-ty-kate kit-ty-kate left a comment

Choose a reason for hiding this comment

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

i'm no nixos expert but from afar it looks reasonable

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one person. label Dec 10, 2024
@kit-ty-kate kit-ty-kate mentioned this pull request Dec 10, 2024
Merged
@kit-ty-kate
Copy link
Copy Markdown
Contributor

kit-ty-kate commented Dec 10, 2024

I've opened a PR upstream fixing this issue: ocaml/opam#6333
If you think this is the correct fix it will be part of the upcoming opam 2.4.

@nixos-discourse
Copy link
Copy Markdown

nixos-discourse commented Dec 29, 2024

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/5027

thiagokokada
thiagokokada reviewed Dec 29, 2024
View reviewed changes
@thiagokokada
Copy link
Copy Markdown
Contributor

thiagokokada commented Dec 29, 2024

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 363770

x86_64-linux

✅ 3 packages built:
  • dune-release
  • opam
  • opam.installer

@wegank wegank removed the 12.approvals: 1 This PR was reviewed and approved by one person. label Dec 30, 2024
@eilvelia
Copy link
Copy Markdown
Member Author

eilvelia commented Dec 30, 2024

(The ofborg failure does not look related to this.)

GaetanLepage
GaetanLepage reviewed Jan 9, 2025
View reviewed changes
@eilvelia
opam: fix opam sandboxing on nixos, cleanup
5a51e70 
To make opam sandboxing (via bwrap) work on nixos, the following had been used
here:
  --set OPAM_USER_PATH_RO /run/current-system/sw/bin:/nix/
However, OPAM_USER_PATH_RO has been removed in opam 2.2.0, requiring a
new workaround: ocaml/opam@9b6370d
(Before this commit, executing `opam init` would display that sandboxing fails
with "bwrap: execvp sh: No such file or directory".)

- Removes outdated workarounds for ocp-build and argv0, cleans postInstall
- Fixes link to the changelog which was broken because of "with lib;"
@GaetanLepage
Copy link
Copy Markdown
Contributor

GaetanLepage commented Jan 10, 2025

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 363770

x86_64-linux

✅ 3 packages built:
  • dune-release
  • opam
  • opam.installer

aarch64-linux

✅ 3 packages built:
  • dune-release
  • opam
  • opam.installer

x86_64-darwin

✅ 3 packages built:
  • dune-release
  • opam
  • opam.installer

aarch64-darwin

✅ 3 packages built:
  • dune-release
  • opam
  • opam.installer

GaetanLepage
GaetanLepage approved these changes Jan 10, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@GaetanLepage GaetanLepage left a comment

Choose a reason for hiding this comment

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

LGTM

@GaetanLepage GaetanLepage merged commit ec3665c into NixOS:master Jan 10, 2025
@nixpkgs-ci
Copy link
Copy Markdown
Contributor

nixpkgs-ci bot commented Jan 10, 2025

Successfully created backport PR for release-24.11:

@nixpkgs-ci nixpkgs-ci bot mentioned this pull request Jan 10, 2025
Merged
1 task
@eilvelia eilvelia deleted the opam branch January 10, 2025 10:22
@kit-ty-kate kit-ty-kate mentioned this pull request Mar 1, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thiagokokada thiagokokada thiagokokada left review comments

@GaetanLepage GaetanLepage GaetanLepage approved these changes

+1 more reviewer

@kit-ty-kate kit-ty-kate kit-ty-kate approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

6.topic: ocaml OCaml is a general-purpose, high-level, multi-paradigm programming language. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@eilvelia @kit-ty-kate @nixos-discourse @thiagokokada @GaetanLepage @vbgl @wegank