Skip to content

lowdown: fix aarch64-darwin by removing deprecated sandbox#125004

Merged
domenkozar merged 8 commits intoNixOS:masterfrom
hlolli:lowdown-darwin
May 31, 2021
Merged

lowdown: fix aarch64-darwin by removing deprecated sandbox#125004
domenkozar merged 8 commits intoNixOS:masterfrom
hlolli:lowdown-darwin

Conversation

@hlolli
Copy link
Member

@hlolli hlolli commented May 30, 2021 •
edited
Loading

Motivation for this change

Currently latest nix doesn't build on aarch64 due to failure from lowdown.
The current regression test provided from lowdown falsly pass.
So I wrote one very basic install-check.

Some interesting discussions about the (bad and deprecated) sandbox.h
https://developer.apple.com/forums/thread/661939

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs 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/)
  • Added a release notes entry if the change is major or breaking
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot added the 6.topic: darwin Running or building packages on Darwin label May 30, 2021
@hlolli hlolli force-pushed the lowdown-darwin branch 2 times, most recently from f651e83 to 3653379 Compare May 30, 2021 23:16
@ofborg ofborg bot requested a review from sternenseemann May 30, 2021 23:26
@ofborg ofborg bot added 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 May 30, 2021
sternenseemann
sternenseemann requested changes May 30, 2021
View reviewed changes
@ofborg ofborg bot requested a review from sternenseemann May 31, 2021 00:28
@r-rmcgibbo
Copy link

r-rmcgibbo commented May 31, 2021 •
edited
Loading

Result of nixpkgs-review pr 125004 at b62962b run on aarch64-linux 1

6 packages built successfully:
  • hydra-unstable
  • lowdown
  • nix-direnv
  • nix-update
  • nixUnstable
  • nixpkgs-review

Result of nixpkgs-review pr 125004 at b62962b run on x86_64-linux 1

6 packages built successfully:
  • hydra-unstable
  • lowdown
  • nix-direnv
  • nix-update
  • nixUnstable
  • nixpkgs-review

@hlolli hlolli mentioned this pull request May 31, 2021
Closed
veprbl
veprbl reviewed May 31, 2021
View reviewed changes
veprbl
veprbl reviewed May 31, 2021
View reviewed changes
@veprbl
Copy link
Member

veprbl commented May 31, 2021

@GrahamcOfBorg build lowdown

@veprbl veprbl changed the title lowdown: fix aarch64-darwin remove deprecated sandbox lowdown: fix aarch64-darwin by removing deprecated sandbox May 31, 2021
@hlolli
Copy link
Member Author

hlolli commented May 31, 2021

@veprbl you're fast! but I added there as well $out/bin to the postInstall check, it doesn't find the binary without some help it seems.

@veprbl
Copy link
Member

veprbl commented May 31, 2021

@GrahamcOfBorg build lowdown

@veprbl
Copy link
Member

veprbl commented May 31, 2021

Result of nixpkgs-review pr 125004 run on x86_64-darwin 1

5 packages built:
  • lowdown
  • nix-direnv
  • nix-review (nixpkgs-review)
  • nix-update
  • nixFlakes (nixUnstable)

@domenkozar domenkozar merged commit deaa70e into NixOS:master May 31, 2021
@github-actions
Copy link
Contributor

github-actions bot commented May 31, 2021

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

Please cherry-pick the changes locally:

git fetch origin release-21.05
git worktree add -d .worktree/backport-125004-to-release-21.05 origin/release-21.05
cd .worktree/backport-125004-to-release-21.05
git checkout -b backport-125004-to-release-21.05
ancref=$(git merge-base 6094bec3c3cb811800f6f71a8951328487b6df8c ded7f8f89567b63096a41d90177fb7a4e702c7a5)
git cherry-pick -x $ancref..ded7f8f89567b63096a41d90177fb7a4e702c7a5

@andir
Copy link
Member

andir commented May 31, 2021

This PR doesn't follow our contribution guidelines for commit messages. Please try a bit harder to follow it. If senior contributors don't do that we have zero chance to enforce it as they aren't leading with good examples.

@veprbl
Copy link
Member

veprbl commented May 31, 2021

Ouch, this was intended to be squashed on merge...

@hlolli
Copy link
Member Author

hlolli commented May 31, 2021

This PR doesn't follow our contribution guidelines for commit messages. Please try a bit harder to follow it. If senior contributors don't do that we have zero chance to enforce it as they aren't leading with good examples.

that's very fair point!

I'm less senior than having merge rights, but I'm in the habit of squasing and renaming the commit messages.
But I see now this was probably aimed at @veprbl altough I can do better too.

@emilazy emilazy mentioned this pull request Oct 6, 2024
Merged
13 tasks
emilazy added a commit to emilazy/nixpkgs that referenced this pull request Oct 7, 2024
@emilazy
lowdown: add flag to disable the Darwin sandbox
dc32d18 
This is a program written in a memory‐unsafe language that processes
potentially‐untrusted user input. We shouldn’t disable upstream’s
sandboxing mechanisms for all downstream consumers without good
reason.

Although the sandbox API is officially marked as deprecated, it is
used as the basis for the supported App Sandbox and it is extremely
unlikely to ever be removed as it is used extensively throughout
the OS for service hardening and by third parties like the Chrome
sandbox. Nix itself uses it to sandbox builds, and its lack of support
for nesting is why this caused problems in the first place. Instead,
introduce a `lowdown-unsandboxed` package that can be used in the
`nativeBuildInputs` of Nix builds, while keeping the sandboxed
version of the program for general use. The name might not be ideal,
as it remains identical to `lowdown` on non‐Darwin platforms,
but I couldn’t think of a better one.

See: NixOS#125004
Closes: NixOS#346933
wrbbz pushed a commit to wrbbz/nixpkgs that referenced this pull request Oct 9, 2024
@emilazy
lowdown: add flag to disable the Darwin sandbox
a9be53d 
This is a program written in a memory‐unsafe language that processes
potentially‐untrusted user input. We shouldn’t disable upstream’s
sandboxing mechanisms for all downstream consumers without good
reason.

Although the sandbox API is officially marked as deprecated, it is
used as the basis for the supported App Sandbox and it is extremely
unlikely to ever be removed as it is used extensively throughout
the OS for service hardening and by third parties like the Chrome
sandbox. Nix itself uses it to sandbox builds, and its lack of support
for nesting is why this caused problems in the first place. Instead,
introduce a `lowdown-unsandboxed` package that can be used in the
`nativeBuildInputs` of Nix builds, while keeping the sandboxed
version of the program for general use. The name might not be ideal,
as it remains identical to `lowdown` on non‐Darwin platforms,
but I couldn’t think of a better one.

See: NixOS#125004
Closes: NixOS#346933
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@veprbl veprbl veprbl approved these changes

@sternenseemann sternenseemann Awaiting requested review from sternenseemann

Assignees

No one assigned

Labels

6.topic: darwin Running or building packages on Darwin 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.

6 participants

@hlolli @r-rmcgibbo @veprbl @andir @sternenseemann @domenkozar