Skip to content

Init grist core at 1.5.1#376176

Draft
Scandiravian wants to merge 4 commits intoNixOS:masterfrom
Scandiravian:init-grist-core
Draft

Init grist core at 1.5.1#376176
Scandiravian wants to merge 4 commits intoNixOS:masterfrom
Scandiravian:init-grist-core

Conversation

@Scandiravian
Copy link
Contributor

@Scandiravian Scandiravian commented Jan 23, 2025

This is based on the work done in #305019 and #322633. I've added some additional changes to the service and changed the buildPhase to bring the output size down with a few hundred MiB.

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: python Python is a high-level, general-purpose programming language. 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` labels Jan 23, 2025
@Scandiravian
Copy link
Contributor Author

@bendlas and @soyouzpanda this is based on a lot of your work, so let me know if this is an issue for you.

I've added myself as a maintainer to the package and module; if you would like to be added as maintainers, please let me know as well.

@NixOSInfra NixOSInfra added the 12.first-time contribution This PR is the author's first one; please be gentle! label Jan 23, 2025
@Scandiravian Scandiravian force-pushed the init-grist-core branch 3 times, most recently from f0923d1 to 1cff416 Compare January 23, 2025 17:31
@soyouzpanda

This comment was marked as spam.

@Scandiravian
Copy link
Contributor Author

image

@soyouzpanda I'm not sure I follow - could you explain what you mean by this? 😕

@soyouzpanda
Copy link
Contributor

image

@soyouzpanda I'm not sure I follow - could you explain what you mean by this? 😕

I do not want my work to be used in an open source project that collaborates with weapon makers and fascists, that's all.

@Scandiravian
Copy link
Contributor Author

Scandiravian commented Jan 23, 2025

I do not want my work to be used in an open source project that collaborates with weapon makers and fascists, that's all.

@soyouzpanda I hear what you're saying. I don't want to get into a big discussion, but suffice to say that I understand your point of view and where you're coming from.

I can remove the commits that I cherry-picked from your PR and rewrite those parts myself. I can't guarantee that it won't be somewhat similar to the work you made, since there's a limited number of ways to configure things in Nix, but it will remove your association from the history. Would that be acceptable to you?

@bendlas
Copy link
Contributor

bendlas commented Jan 25, 2025

Sorry, I don't really know what's going on and I also don't really feel like playing catch-up, so let me just write down what I'm taking from this and where I'm at:

I'm assuming that Scandiravian or their project has a known association with Anduril and the MIC, either way they don't seem to deny it. I am feeling blindsided by them not being up-front about it, because at this point, their controversial status within the community cannot be considered suprising. For this reason, I'll disengage from this conversation and rescind my earlier offer of helping shepherd their work. Please let me know if I've got anything wrong in my assessment.

thanks

@Scandiravian
Copy link
Contributor Author

Scandiravian commented Jan 25, 2025

Sorry, I don't really know what's going on and I also don't really feel like playing catch-up, so let me just write down what I'm taking from this and where I'm at:

I'm assuming that Scandiravian or their project has a known association with Anduril and the MIC, either way they don't seem to deny it. I am feeling blindsided by them not being up-front about it, because at this point, their controversial status within the community cannot be considered suprising. For this reason, I'll disengage from this conversation and rescind my earlier offer of helping shepherd their work. Please let me know if I've got anything wrong in my assessment.

thanks

@bendlas I completely understand your position, but to clear things up, I don't have any affiliation with Anduril, nor any other company related to the MIC, in any capacity. I never have and I never will, as it would be irreconcilable with my personal values.

The organisation I'm working for is a public institution that works to improve treatment for patients across the EU.

I think @soyouzpanda is uncomfortable contributing to nixpkgs as a whole, not due to anything related to me. I want to respect their position and accommodate it in a way that works for them, which is why I chose to keep the focus on how to resolve the issues they have with their work being contributed to nixpkgs.

@bendlas
Copy link
Contributor

bendlas commented Jan 25, 2025

@Scandiravian thank you very much for that clarification! In this case, I'd like to ask your forgiveness for the misunderstanding and to re-offer my help. I'll have a closer look at this PR, next week.

@bendlas bendlas self-requested a review January 25, 2025 15:35
@bendlas
Copy link
Contributor

bendlas commented Jan 25, 2025

As for soyouzpanda's contributions: It's probably best to remove their commits entirely, in order to respect their protest.

I agree that it wouldn't be reasonable for them to expect zero overlap in solution space, and I feel if that was their goal they might have deleted their PR - lets just be as clean-room as possible, given that we've already looked at their commits.

cc @NixOS/moderation, just to make sure we're getting this right

@Scandiravian
Copy link
Contributor Author

@Scandiravian thank you very much for that clarification! In this case, I'd like to ask your forgiveness for the misunderstanding and to re-offer my help. I'll have a closer look at this PR, next week.

There's nothing to forgive. This is a sensitive topic and I understand there are strong feelings involved.

Your help would be greatly appreciated!

I made some changes to the systemd unit that I forgot to push before finishing work yesterday. I got to a point where the module works with sandboxing disabled, but there's still some issues when it's turned on. I'll push my work when I get to the office on Monday.

As for soyouzpanda's contributions: It's probably best to remove their commits entirely, in order to respect their protest.

I agree that it wouldn't be reasonable for them to expect zero overlap in solution space, and I feel if that was their goal they might have deleted their PR - lets just be as clean-room as possible, given that we've already looked at their commits.

cc @NixOS/moderation, just to make sure we're getting this right

That sounds reasonable; until there's input from soyouzpanda on a solution that would work for them, I think it's the best we can do given the circumstances. I'll sort out the history on Monday.

@h7x4 h7x4 added 8.has: module (new) This PR adds a module in `nixos/` 8.has: tests This PR has tests labels Jan 26, 2025
@Scandiravian Scandiravian force-pushed the init-grist-core branch 6 times, most recently from d52fb68 to 75e3ea5 Compare January 27, 2025 12:12
@Scandiravian
Copy link
Contributor Author

Scandiravian commented Jan 27, 2025

I've updated the history and pushed my local changes. The module should work as long as enableSandboxing = false. When it's enabled grist throws an error about python3 not being available when creating a blank document through the web-interface. I've checked that the binary is in the correct path in the systemd unit, so I'm not sure why the error happens.

It's something that could be fixed upstream by rewriting sandbox/run.py to not be so dependent on hardcoded paths, but I'm not sure how viable that is as it looks like a big undertaking.

I'll spend some more time on this issue later this week (probably Wednesday).

I'm also confused about the failing CI check regarding the docs - If someone understands why this is failing, please let me know 😅

@Scandiravian Scandiravian force-pushed the init-grist-core branch 2 times, most recently from 465cb53 to b87dd96 Compare January 27, 2025 12:57
needed for grist-core

Co-authored-by: phaer <hello@phaer.org>
@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Apr 25, 2025
@Scandiravian Scandiravian force-pushed the init-grist-core branch 4 times, most recently from 1d8707c to 7c28b30 Compare April 25, 2025 11:06
Co-authored-by: phaer <hello@phaer.org>
@github-actions github-actions 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 Apr 25, 2025
@Scandiravian Scandiravian force-pushed the init-grist-core branch 3 times, most recently from 8c784fc to c073f71 Compare April 25, 2025 11:40
Scandiravian and others added 2 commits April 25, 2025 13:44
Co-authored-by: phaer <hello@phaer.org>
Basic smoketest for the gVisor sandboxing.

Signed-off-by: Raito Bezarius <masterancpp@gmail.com>
@Scandiravian Scandiravian changed the title Init grist core at 1.3.2 Init grist core at 1.5.1 Apr 25, 2025
@Scandiravian
Copy link
Contributor Author

Scandiravian commented Apr 25, 2025

I've updated the PR with the changes from @phaer. Unfortunately there's still an issue when sandbox is enabled due to these lines in Grist: https://github.com/gristlabs/grist-core/blob/8d2ce5ed0d013ce1b08a90fa497a42f33cbd452a/sandbox/gvisor/run.py#L177-L191

This will result in the error failed to load /usr/bin/python3: no such file or directory, when using Grist through the web-interface

I'm not sure if there is a smart way to fake that path within the systemd unit. If there isn't, it'll probably require a rewrite of the script upstream.

@phaer
Copy link
Member

phaer commented Apr 25, 2025

Unfortunately there's still an issue when sandbox is enabled due to these lines in Grist: https://github.com/gristlabs/grist-core/blob/8d2ce5ed0d013ce1b08a90fa497a42f33cbd452a/sandbox/gvisor/run.py#L177-L191

This will result in the error failed to load /usr/bin/python3: no such file or directory, when using Grist through the web-interface

To be clear, this is grist core only checking /usr/bin/python3 and /usr/local/bin/python3? Shouldn't this be handled by:

          BindReadOnlyPaths = [
            "${cfg.package.pythonEnv}/bin:/usr/bin"
            "${cfg.package.pythonEnv}/lib:/usr/lib"
          ];

?

@Scandiravian
Copy link
Contributor Author

To be clear, this is grist core only checking /usr/bin/python3 and /usr/local/bin/python3? Shouldn't this be handled by:

          BindReadOnlyPaths = [
            "${cfg.package.pythonEnv}/bin:/usr/bin"
            "${cfg.package.pythonEnv}/lib:/usr/lib"
          ];

?

I think the error might be from gvisor and then propagated through grist-core. I'll look into it, when I'm back at the office tomorrow

@Scandiravian
Copy link
Contributor Author

I had a look at the issue today and yesterday

I changed the run.py script to mount the entire Nix store in the sandbox, which seems to have resolved the failed to load /usr/bin/python3: no such file or directory - the reason for this working is still unclear to me

Unfortunately the script now stalls at this call: https://github.com/gristlabs/grist-core/blob/51dc0d7949d483458c9eb8fd7b6d8feefa4aeaf6/sandbox/gvisor/run.py#L271. I'll try to look into this next issue tomorrow

My version of the script is here https://github.com/Scandiravian/grist-core/blob/nix-debug/sandbox/gvisor/run.py. It can be run by changing the source for grist-core to:

src = fetchFromGitHub {
  owner = "Scandiravian";
  repo = "grist-core";
  rev = "8023ac462fdf780055b198b7b2efa7f0797149f6";
  hash = "sha256-JAWPuVI3rf8EcRWND2JdeLjQnAV1mKEDU0HHB+JOFTg=";
};

My plan going forward is to fix the stalling issue, then figure out the root cause of both of them to determine whether it's possible to fix the issues in this PR or if it needs to be done upstream

Copy link
Member

Choose a reason for hiding this comment

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

Drop the formatting changes here.

@phaer
Copy link
Member

phaer commented Apr 29, 2025

My plan going forward is to fix the stalling issue, then figure out the root cause of both of them to determine whether it's possible to fix the issues in this PR or if it needs to be done upstream

Thank you for your investigations! I only had a quick glance at the source yet and might be misreading, but could it be that it failed to run python in the first place, because it needs to resolve a symlink pointing from the python binary to the nix store but the systemd sandbox didn't have that accessible?

It seems to resolve symlinks (realpath) while processing oci mount paths in the linked run.py, so a similar thing might happen one sandbox below, so to speak, for trying to run a binary in the nix store inside a "container"?

@Scandiravian
Copy link
Contributor Author

Scandiravian commented May 3, 2025

Thank you for your investigations! I only had a quick glance at the source yet and might be misreading, but could it be that it failed to run python in the first place, because it needs to resolve a symlink pointing from the python binary to the nix store but the systemd sandbox didn't have that accessible?

It seems to resolve symlinks (realpath) while processing oci mount paths in the linked run.py, so a similar thing might happen one sandbox below, so to speak, for trying to run a binary in the nix store inside a "container"?

I think the python binaries in /usr/bin should be the actual executables, since the store-path of pythonEnv is bound to that path, but there might be some other issues related to Grist's own python library

I got a bit further with the investigation. The gvisor python process starts as it should, but the interaction between the JS code and the python code which is somehow broken

Commands sent from the JS backend to the python process in the sandbox all time out. I'll try to figure out a way to debug this next week

@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Jun 9, 2025
@nixpkgs-ci nixpkgs-ci bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Oct 30, 2025
@embedding-shapes
Copy link

Anything one could do here to help out to get this across the finish line?

@nixpkgs-ci nixpkgs-ci bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Dec 8, 2025
@nixpkgs-ci nixpkgs-ci bot removed the 12.first-time contribution This PR is the author's first one; please be gentle! label Mar 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2.status: merge conflict This PR has merge conflicts with the target branch 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: python Python is a high-level, general-purpose programming language. 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` 8.has: module (new) This PR adds a module in `nixos/` 8.has: module (update) This PR changes an existing module in `nixos/` 8.has: tests This PR has tests 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

Development

Successfully merging this pull request may close these issues.