sapling: init at 0.1.20221118-210929-cfbb68aa

[pbar1]

Description of changes

Add Sapling (ie, sl) to Nixpkgs.

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, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• 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/)
• 22.11 Release Notes (or backporting 22.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
  • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
• Fits CONTRIBUTING.md.

[ihatethefrench]

Did some personal testing here's everything that I've tested that works:

• Initialising a repository (sl init <path>)
• Cloning from Git (sl clone --git <url>)
• Smart log (sl sl)
• Interactive smart log (sl web)
  • This one needed nodejs in PATH so I temporarily installed it using nix profile install nixpkgs#nodejs
• Pulling and pushing (sl pull/sl push)

Safe to say, I believe this works 😛

[pbar1]

Here's my attempt at building Sapling from source: https://gist.github.com/pbar1/dfdcce442306e70f7bddabe3403276c8

Using the package definition in this gist, I was almost able to get the sl binary built using buildRustPackage - ran into these build failures:

error: failed to run custom build command for `bzip2-sys v0.1.11+1.0.8`
error: failed to run custom build command for `libz-sys v1.1.8`
error: failed to run custom build command for `lz4-sys v1.9.4`
error: failed to run custom build command for `libnghttp2-sys v0.1.7+1.45.0`
error: failed to run custom build command for `zstd-sys v2.0.1+zstd.1.5.2`

Adding each of those as dependencies to buildInputs (they weren't in there before) didn't fix things. Here's the actual error message - they were all Permission denied errors:

  thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Os { code: 13, kind: PermissionDenied, message: "Permission denied" }', /build/cargo-vendor-dir/zstd-sys-2.0.1+zstd.1.5.2/build.rs:184:58

Didn't pursue it any further after seeing that error; these builds take quite a long time.

Also had to generate a Cargo.lock file and copy it into the derivation, as upstream does not commit this file. buildRustPackage also seems to assume Cargo workspaces are in the source root and thus looks for Cargo.toml there when comparing it to the vendor-generated one; I hacked that by copying it to the root, but this could also just be user error on my part not knowing how to use buildRustPackage properly.

I left a TODO comment in this PR's package definition to the effect of: building Sapling is somewhat hard. Solving for building sl would be the first and largest step, and I feel we're almost there. The next step would be deducing how to replicate the other dependencies in site-packages via either a build or copy. For example, there's a shared object conch_parser.so that I haven't taken stock of yet.

Finally, looks like node is an optional dependency that's used to launch the interactive smartlog web view - not sure what to do with that, regarding baking it in or leaving it out and optional.

It's unfortunate that there's only a deb for x86_64-linux right now, but I think this repackaged deb is a good enough start so people can begin using sl, and then we can push the source build over the finish line.

[pbar1]

Alright - I got it to build from source! It was similar to the tokenizer example. Since a new version was also released since the last commit, I also updated the version and pushed a new commit, rather than squashing, for posterity of the original work done with the deb.

There's a fair amount of pinning going on here. I started making an update.sh script to automate all of the ceremony of pulling out the pythonhosted.org dependencies, but eventually lost steam. This would be pretty much necessary for a sustainable package update workflow.

[nixos-discourse]

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/1448

[SuperSandro2000]

Adding each of those as dependencies to buildInputs (they weren't in there before) didn't fix things. Here's the actual error message - they were all Permission denied errors:

Did you add pkg-config to nativeBuildInputs?

[SuperSandro2000]

There is no crate by chance so we can avoid vendoring the cargo.lock?

[SuperSandro2000]

How hard would it be to rip this all out and use our pythonPackages?

In the current state this is unmaintainable and we can't merge it like this.

[pbar1]

Agreed, wanted to automate this part with a script; it's hard to tell what they're doing with these deps, but the setup.py logic depends on the actual archive instead of the source within. I mostly mimicked the example for tokenizer in doing this, to get it working. Are there any alternatives to generating these (and the associated items in the let block`)?

[pbar1]

My current plan is to generate a deps.nix file using an update script that can be imported by this file, to abstract away all the crud currently here. Is there a way to calculate the sha256 of a derivation that would be generated from fetchurl beforehand, rather than attempting with a fake hash and then correcting it?

[pbar1]

Ok, I've done the above and made a usable script for automating asset fetching, plus discovered nix-prefetch-url. Will integrate the latter into said script and push tomorrow or so, as it's gotten late here.

[SuperSandro2000]

Why do we need the exact versions at all? Can't we just use any version?

[asdf8dfafjk]

I suspect there is a needless amount of debate which distracts from the end goal. I know Nix is a build paltform and everyone feels good when they know builds better than the other guy but end of the day a project needs working binaries, and sapling binary works.

Perhaps, just perhaps, building in the most ideal way isn't the priority for fb team right now? Maybe 6 months down the line they'd be using the latest and best, maybe ship a default.nix and flake.nix?

I know I sound very bitter but I see this attitude repeatedly where PR creators are punished for their attempts. OP has a working binary, great, lets go with it. I am all for maintainability but hey, there are no spaces at the end of newline so that should be good enough!

[thoughtpolice]

I suspect

Your suspicions are wrong.

Perhaps, just perhaps, building in the most ideal way isn't the priority for fb team right now?

Where did someone say it should be their priority? I'm getting pretty tired of people who aren't maintainers, or do not contribute actively to maintaining nixpkgs, constantly shoving words in the mouths of developers, all because they're in a rush to complain we aren't doing free work for them fast enough. Sapling was released a week ago. We're lucky someone even stepped up to try this early given my initial glance at the complexity of the package. And you still complaining that we still haven't merged this fast enough, still?

The Sapling team is not obligated to make changes we need; in fact if they said "No" to everything we wanted here, that would still be informative for our purposes so we know what to expect. This is good! This is literally completely self-evident to anyone who has had to work between teams before! Literally this happens all the time in open source. These are relevant things to maintainers who are responsible for delivering the software to Nix users.

Again: The current expression works. It basically looks fine, too, assuming we're on board with all this. I never said I wasn't. I think we should have communication with them to make it easy as possible to maintain this in the future, especially wrt concerns like Cargo.lock. In my experience communicating with upstreams, I think this will in turn give the Sapling developers a little insight into what we and others will need. None of this is in conflict.

I know I sound very bitter but I see this attitude repeatedly where PR creators are punished for their attempts.

Let me get this straight: you aren't writing the patches, you aren't maintaining the package, you aren't writing the software. The only thing you seem to be doing is posting in a text box about how we aren't doing this to your personal liking and at the speed which personally makes you happy — yet you're the one who is bitter and whose feelings need to be assuaged? Sorry, but that sounds like a "You" problem entirely.

[asdf8dfafjk]

I've definitely contributed and stopped only because of this attitude of Devs like you to casually veer into intellectual self pleasure and losing sight of the goal

Theres a reason a human looks at these PRs, because humans are supposed to factor in situations.

[thoughtpolice]

Good, because luckily we have hundreds of amazing and talented contributors who aren't afraid to roll up their sleeves and do what it takes to be part of a community, and part of that means doing dirty work, sometimes. It's never too late to know this feeling, if you want to really participate. But until then, don't bother or offer any more "help" or "suggestions" like this.

[SuperSandro2000]

Part of the problem is that we're going to need continuous integration and testing with upstream's builds in order to make sure this works,

Yes, like for any other program in nixpkgs which is not using upstreams vendored dependencies (which no program should) and where upstream does not have really tight version ranges which has its own problems.

I know Nix is a build paltform

yes and nixpkgs is a package collection that is comparable to the ones used by apt and rpm and using such hacks would be completely unacceptable for the main repository.

If you need something that "just works" then upstreaming to nixpkgs might not be the option you are searching for for packages like sapling that have such a horrible build process.

I know I sound very bitter but I see this attitude repeatedly where PR creators are punished for their attempts. OP has a working binary, great, lets go with it. I am all for maintainability but hey, there are no spaces at the end of newline so that should be good enough!

nixpkgs has a higher standard then things working. Otherwise managing security patches would be impossible. If this is the best solution we have right now then I'd rather not have sapling.

I just skimmed through @thoughtpolice posts but this PR should have not be merged in this state and I am voting for reverting it rather than having this compromise.

[pbar1]

Adding each of those as dependencies to buildInputs (they weren't in there before) didn't fix things. Here's the actual error message - they were all Permission denied errors:

Did you add pkg-config to nativeBuildInputs?

I did have pkg-config in nativeBuildInputs at that time. That comment though was in the context of a prior iteration where I was just building the rust binary, whereas this latest build that succeeds is the full packge. Upstream builds the tool with Python, even though its entrypoint is Rust.

[pbar1]

I've finished gen-deps and pushed. This file is run whenever you'd want to update the package version - it fetches the latest GitHub stable release (and calculates its hash), performs nix-prefetch-url on the imperative assets in sapling's setup.py, and finally renders deps.nix which is imported and used in the derivation.

[ihatethefrench]

Did some personal testing here's everything that I've tested that works:

* Initialising a repository (`sl init <path>`)

* Cloning from Git (`sl clone --git <url>`)

* Smart log (`sl sl`)

* Interactive smart log (`sl web`)
  
  * This one needed `nodejs` in PATH so I temporarily installed it using `nix profile install nixpkgs#nodejs`

* Pulling and pushing (`sl pull`/`sl push`)

Safe to say, I believe this works 😛

Following up with this, the latest commit also works as expected, with one notable exception being sl web/sl isl which returns the following error:

abort: No such file or directory: /nix/store/kjlzi41rbd36gq78h0balmwqk8fmd0gj-python3.10-sapling-0.1.20221118-210929-cfbb68aa/lib/python3.10/site-packages/edenscm/commands/../../edenscm-isl/run-isl

Everything else works as expected!

P.S.: Great job on the package! Excited to see these final kinks worked out and Sapling officially available in Nixpkgs :)

[thoughtpolice]

Basically looks good to me, assuming we're all on board with the Rust and Python stuff going on here. I could make one or two minor nits but it isn't worth it honestly.

It would also be nice to support Darwin but I suspect that will just add even more complexity, so we'll just save it for when we need it and someone adds it.

[thoughtpolice]

abort: No such file or directory: /nix/store/kjlzi41rbd36gq78h0balmwqk8fmd0gj-python3.10-sapling-0.1.20221118-210929-cfbb68aa/lib/python3.10/site-packages/edenscm/commands/../../edenscm-isl/run-isl

Ugh, actually this is rough and worth fixing IMO. The web interface is literally the best part of the whole thing, it's incredible. Maybe I can patch this in myself...

If it's ridiculously complex to add to the package I guess we can hold off. But if it's a little complex, it might be worth it now. The web interface really is that good.

[ihatethefrench]

abort: No such file or directory: /nix/store/kjlzi41rbd36gq78h0balmwqk8fmd0gj-python3.10-sapling-0.1.20221118-210929-cfbb68aa/lib/python3.10/site-packages/edenscm/commands/../../edenscm-isl/run-isl

Ugh, actually this is rough and worth fixing IMO. The web interface is literally the best part of the whole thing, it's incredible. Maybe I can patch this in myself...

If it's ridiculously complex to add to the package I guess we can hold off. But if it's a little complex, it might be worth it now. The web interface really is that good.

Hard agree! It's fantastic, I've grown to love it myself!

I'm personally worried that if this is merged without it working people might think it's a bug and not intended - especially when the web interface is such a large selling point for Sapling

I understand however that this is already a very complex derivation - and my best wishes go to everyone who's already put time and effort into packaging it 😁

[thoughtpolice]

I'm personally worried that if this is merged without it working people might think it's a bug and not intended - especially when the web interface is such a large selling point for Sapling

I agree that it may seem like an unintentional bug. FWIW, if we can't fix sl web directly right now, it's probably worth just writing a wrapper script that intercepts the web argument to sl and then throws an error that is more informative, i.e. maybe something like

$ sl web
ERROR: The Sapling Web UI is currently not supported by the Nix package.
Please see the following URL for more information: https://github.com/nixos/nixpkgs/issue/12345678910

We can just file a bug separately for that last part.

[quark-zju]

Hi, author of the Python dependencies in Sapling's setup.py here. I can provide some context about the current implementation:

• Internal build environment does not have pip, or Internal network access to pypi.org. The logic needs to work both internally and externally.
• On Windows, we want to zip all (pure) dependencies to pythonXY.zip. We found that importing from many small files is slower than importing from a single zip on Windows. This might require some "low-level" access to the dependencies.
• There are generated dependencies. For example, generate edenfs Thrift types in Python. This is not used in the current release of Sapling though.

I'm not an expert in the Python dependency management world and haven't fully explored choices like poetry and others. If there is a format to declare the dependencies (requirements.txt?) more formally, I think we can support that.

Note we also have native dependencies. I wonder if something like a check-in default.nix in Sapling with buildInputs specifying nix dependencies would be helpful. If this sounds good then PRs are welcome. While I don't feel confident writing the initial .nix, I think we can probably maintain it and use a GitHub workflow keeping it build (if the build is not fragile).

[pbar1]

Basically looks good to me, assuming we're all on board with the Rust and Python stuff going on here. I could make one or two minor nits but it isn't worth it honestly.

It would also be nice to support Darwin but I suspect that will just add even more complexity, so we'll just save it for when we need it and someone adds it.

@thoughtpolice Nit away! I'd love the feedback, part of the learning process and I'd like this package to be as maintainable as possible.

abort: No such file or directory: /nix/store/kjlzi41rbd36gq78h0balmwqk8fmd0gj-python3.10-sapling-0.1.20221118-210929-cfbb68aa/lib/python3.10/site-packages/edenscm/commands/../../edenscm-isl/run-isl

Ugh, actually this is rough and worth fixing IMO. The web interface is literally the best part of the whole thing, it's incredible. Maybe I can patch this in myself...

If it's ridiculously complex to add to the package I guess we can hold off. But if it's a little complex, it might be worth it now. The web interface really is that good.

This should be fixed for sure before merging - it was working in the initial iteration of this package (when I used autoPatchelfHook on the deb). The error doesn't look too arcane, hopefully just a hardcoded path (🤞).

re: Darwin support - yeah, I also really want to get this working as there is an official macOS build (Homebrew) which would be nice to have parity with. I believe the issue (and fix) is the same as is referenced in this PR comment: PyO3/setuptools-rust#106 (comment). If someone knows how to inject these build flags into the derivation, that may help get the macOS build off the ground.

[pbar1]

Hi, author of the Python dependencies in Sapling's setup.py here. I can provide some context about the current implementation:

• Internal build environment does not have pip, or Internal network access to pypi.org. The logic needs to work both internally and externally.
• On Windows, we want to zip all (pure) dependencies to pythonXY.zip. We found that importing from many small files is slower than importing from a single zip on Windows. This might require some "low-level" access to the dependencies.
• There are generated dependencies. For example, generate edenfs Thrift types in Python. This is not used in the current release of Sapling though.

I'm not an expert in the Python dependency management world and haven't fully explored choices like poetry and others. If there is a format to declare the dependencies (requirements.txt?) more formally, I think we can support that.

Note we also have native dependencies. I wonder if something like a check-in default.nix in Sapling with buildInputs specifying nix dependencies would be helpful. If this sounds good then PRs are welcome. While I don't feel confident writing the initial .nix, I think we can probably maintain it and use a GitHub workflow keeping it build (if the build is not fragile).

@quark-zju Thanks for the context! So the zip files built by setup.py are not needed unless it's the Windows build? Snooping in the deb they are there, but if they're not needed for the Unix builds we can filter them out in this package derivation.

Regarding your last comment about checking in something to Sapling upstream - this could be a Nix Flake (which is like a module/crate/etc in the Nix ecosystem).

[quark-zju]

@pbar1 The dependencies are the same for all platforms. The zip used on Windows is an optimization since reading too many small files are slower on Windows' filesystem. On other platforms, we use site-packages/ or just provide the dependencies as plain files for the dependencies. That said, they are mainly just IPython dependencies for the debugshell command, and toml for EdenFS on Windows support. So if they are missing most commands might still run. The documentation site has a few references to the debugshell command though, since it could be useful for developers to explore internals.

[lf-]

I've looked at Darwin.

Darwin build log

python3.10-sapling>    Compiling conch-parser v0.1.1 (/private/tmp/nix-build-python3.10-sapling-0.1.2022
1118-210929-cfbb68aa.drv-0/source/eden/scm/lib/third-party/conch-parser)
python3.10-sapling>    Compiling conch_parser v0.1.0 (/private/tmp/nix-build-python3.10-sapling-0.1.2022
1118-210929-cfbb68aa.drv-0/source/eden/scm/edenscmnative/conch_parser)
python3.10-sapling> error: linking with `/nix/store/v069wkyb6y6qw2bhpzag5kka19j7clgg-clang-wrapper-11.1.
0/bin/cc` failed: exit status: 1
python3.10-sapling>   |
python3.10-sapling>   = note: "/nix/store/v069wkyb6y6qw2bhpzag5kka19j7clgg-clang-wrapper-11.1.0/bin/cc" 
"-Wl,-exported_symbols_list,/private/tmp/nix-build-python3.10-sapling-0.1.20221118-210929-cfbb68aa.drv-0
/rustcvU4ZCh/list" "-arch" "arm64" "/private/tmp/nix-build-python3.10-sapling-0.1.20221118-210929-cfbb68
aa.drv-0/rustcvU4ZCh/symbols.o" "/private/tmp/nix-build-python3.10-sapling-0.1.20221118-210929-cfbb68aa.
drv-0/source/eden/scm/build/cargo-target/release/deps/conch_parser.conch_parser.39884371-cgu.0.rcgu.o" "
/private/tmp/nix-build-python3.10-sapling-0.1.20221118-210929-cfbb68aa.drv-0/source/eden/scm/build/cargo
-target/release/deps/conch_parser.conch_parser.39884371-cgu.1.rcgu.o" "/private/tmp/nix-build-python3.10
-sapling-0.1.20221118-210929-cfbb68aa.drv-0/source/eden/scm/build/cargo-target/release/deps/conch_parser
.conch_parser.39884371-cgu.10.rcgu.o" "/private/tmp/nix-build-python3.10-sapling-0.1.20221118-210929-cfb
b68aa.drv-0/source/eden/scm/build/cargo-target/release/deps/conch_parser.conch_parser.39884371-cgu.11.rc
gu.o" "/private/tmp/nix-build-python3.10-sapling-0.1.20221118-210929-cfbb68aa.drv-0/source/eden/scm/buil
d/cargo-target/release/deps/conch_parser.conch_parser.39884371-cgu.12.rcgu.o" "/private/tmp/nix-build-py
thon3.10-sapling-0.1.20221118-210929-cfbb68aa.drv-0/source/eden/scm/build/cargo-target/release/deps/conc
h_parser.conch_parser.39884371-cgu.13.rcgu.o" "/private/tmp/nix-build-python3.10-sapling-0.1.20221118-21
0929-cfbb68aa.drv-0/source/eden/scm/build/cargo-target/release/deps/conch_parser.conch_parser.39884371-c
gu.14.rcgu.o" "/private/tmp/nix-build-python3.10-sapling-0.1.20221118-210929-cfbb68aa.drv-0/source/eden/
scm/build/cargo-target/release/deps/conch_parser.conch_parser.39884371-cgu.15.rcgu.o" "/private/tmp/nix-
build-python3.10-sapling-0.1.20221118-210929-cfbb68aa.drv-0/source/eden/scm/build/cargo-target/release/d
eps/conch_parser.conch_parser.39884371-cgu.2.rcgu.o" "/private/tmp/nix-build-python3.10-sapling-0.1.2022
1118-210929-cfbb68aa.drv-0/source/eden/scm/build/cargo-target/release/deps/conch_parser.conch_parser.398
84371-cgu.3.rcgu.o" "/private/tmp/nix-build-python3.10-sapling-0.1.20221118-210929-cfbb68aa.drv-0/source
/eden/scm/build/cargo-target/release/deps/conch_parser.conch_parser.39884371-cgu.4.rcgu.o" "/private/tmp
/nix-build-python3.10-sapling-0.1.20221118-210929-cfbb68aa.drv-0/source/eden/scm/build/cargo-target/rele
ase/deps/conch_parser.conch_parser.39884371-cgu.5.rcgu.o" "/private/tmp/nix-build-python3.10-sapling-0.1
.20221118-210929-cfbb68aa.drv-0/source/eden/scm/build/cargo-target/release/deps/conch_parser.conch_parse
r.39884371-cgu.6.rcgu.o" "/private/tmp/nix-build-python3.10-sapling-0.1.20221118-210929-cfbb68aa.drv-0/s
ource/eden/scm/build/cargo-target/release/deps/conch_parser.conch_parser.39884371-cgu.7.rcgu.o" "/privat
e/tmp/nix-build-python3.10-sapling-0.1.20221118-210929-cfbb68aa.drv-0/source/eden/scm/build/cargo-target
/release/deps/conch_parser.conch_parser.39884371-cgu.8.rcgu.o" "/private/tmp/nix-build-python3.10-saplin
g-0.1.20221118-210929-cfbb68aa.drv-0/source/eden/scm/build/cargo-target/release/deps/conch_parser.conch_
parser.39884371-cgu.9.rcgu.o" "/private/tmp/nix-build-python3.10-sapling-0.1.20221118-210929-cfbb68aa.dr
v-0/source/eden/scm/build/cargo-target/release/deps/conch_parser.48is4v7njdgj2p3p.rcgu.o" "-L" "/private
/tmp/nix-build-python3.10-sapling-0.1.20221118-210929-cfbb68aa.drv-0/source/eden/scm/build/cargo-target/
release/deps" "-L" "/nix/store/gqjsjmpqmk10in55zg1jwlghdybcjf6b-rustc-1.64.0/lib/rustlib/aarch64-apple-d
arwin/lib" "/private/tmp/nix-build-python3.10-sapling-0.1.20221118-210929-cfbb68aa.drv-0/source/eden/scm
/build/cargo-target/release/deps/libcpython-b5f8dd225d1ddb56.rlib" "/private/tmp/nix-build-python3.10-sa
pling-0.1.20221118-210929-cfbb68aa.drv-0/source/eden/scm/build/cargo-target/release/deps/libnum_traits-7
8e42db7b4011770.rlib" "/private/tmp/nix-build-python3.10-sapling-0.1.20221118-210929-cfbb68aa.drv-0/sour
ce/eden/scm/build/cargo-target/release/deps/libpython3_sys-857a5a88994ca0d9.rlib" "/private/tmp/nix-buil
d-python3.10-sapling-0.1.20221118-210929-cfbb68aa.drv-0/source/eden/scm/build/cargo-target/release/deps/
liblibc-0df7e67c607172a4.rlib" "/private/tmp/nix-build-python3.10-sapling-0.1.20221118-210929-cfbb68aa.d
rv-0/source/eden/scm/build/cargo-target/release/deps/libconch_parser-51a58589b7002c8b.rlib" "/private/tm
p/nix-build-python3.10-sapling-0.1.20221118-210929-cfbb68aa.drv-0/source/eden/scm/build/cargo-target/rel
ease/deps/libvoid-490d0af61468c83d.rlib" "/private/tmp/nix-build-python3.10-sapling-0.1.20221118-210929-
cfbb68aa.drv-0/source/eden/scm/build/cargo-target/release/deps/libserde-940f75e74ae05b41.rlib" "/nix/sto
re/gqjsjmpqmk10in55zg1jwlghdybcjf6b-rustc-1.64.0/lib/rustlib/aarch64-apple-darwin/lib/libstd-08068d401c2
5d211.rlib" "/nix/store/gqjsjmpqmk10in55zg1jwlghdybcjf6b-rustc-1.64.0/lib/rustlib/aarch64-apple-darwin/l
ib/libpanic_unwind-84be07e68b9d00f4.rlib" "/nix/store/gqjsjmpqmk10in55zg1jwlghdybcjf6b-rustc-1.64.0/lib/
rustlib/aarch64-apple-darwin/lib/libobject-4804c562fc61a807.rlib" "/nix/store/gqjsjmpqmk10in55zg1jwlghdy
bcjf6b-rustc-1.64.0/lib/rustlib/aarch64-apple-darwin/lib/libmemchr-279301e6a5d99fea.rlib" "/nix/store/gq
jsjmpqmk10in55zg1jwlghdybcjf6b-rustc-1.64.0/lib/rustlib/aarch64-apple-darwin/lib/libaddr2line-fbfb344f09
805b90.rlib" "/nix/store/gqjsjmpqmk10in55zg1jwlghdybcjf6b-rustc-1.64.0/lib/rustlib/aarch64-apple-darwin/
lib/libgimli-616191dd1d974165.rlib" "/nix/store/gqjsjmpqmk10in55zg1jwlghdybcjf6b-rustc-1.64.0/lib/rustli
b/aarch64-apple-darwin/lib/librustc_demangle-4687ef078813d96d.rlib" "/nix/store/gqjsjmpqmk10in55zg1jwlgh
dybcjf6b-rustc-1.64.0/lib/rustlib/aarch64-apple-darwin/lib/libstd_detect-1386b7e41dd1fd86.rlib" "/nix/st
ore/gqjsjmpqmk10in55zg1jwlghdybcjf6b-rustc-1.64.0/lib/rustlib/aarch64-apple-darwin/lib/libhashbrown-26b1
aa4af9d5f07a.rlib" "/nix/store/gqjsjmpqmk10in55zg1jwlghdybcjf6b-rustc-1.64.0/lib/rustlib/aarch64-apple-d
arwin/lib/libminiz_oxide-8a7d5d906c81e365.rlib" "/nix/store/gqjsjmpqmk10in55zg1jwlghdybcjf6b-rustc-1.64.
0/lib/rustlib/aarch64-apple-darwin/lib/libadler-327750ded6594992.rlib" "/nix/store/gqjsjmpqmk10in55zg1jw
lghdybcjf6b-rustc-1.64.0/lib/rustlib/aarch64-apple-darwin/lib/librustc_std_workspace_alloc-b801be618a24f
1bf.rlib" "/nix/store/gqjsjmpqmk10in55zg1jwlghdybcjf6b-rustc-1.64.0/lib/rustlib/aarch64-apple-darwin/lib
/libunwind-fa75ed5f72174942.rlib" "/nix/store/gqjsjmpqmk10in55zg1jwlghdybcjf6b-rustc-1.64.0/lib/rustlib/
aarch64-apple-darwin/lib/libcfg_if-adf0011e71898575.rlib" "/nix/store/gqjsjmpqmk10in55zg1jwlghdybcjf6b-r
ustc-1.64.0/lib/rustlib/aarch64-apple-darwin/lib/liblibc-3d5bcaabca1cfe44.rlib" "/nix/store/gqjsjmpqmk10
in55zg1jwlghdybcjf6b-rustc-1.64.0/lib/rustlib/aarch64-apple-darwin/lib/liballoc-cd287cf1e41c0ec4.rlib" "
/nix/store/gqjsjmpqmk10in55zg1jwlghdybcjf6b-rustc-1.64.0/lib/rustlib/aarch64-apple-darwin/lib/librustc_s
td_workspace_core-d99e291378cbf0b8.rlib" "/nix/store/gqjsjmpqmk10in55zg1jwlghdybcjf6b-rustc-1.64.0/lib/r
ustlib/aarch64-apple-darwin/lib/libcore-fcdbad0d5b23b740.rlib" "/nix/store/gqjsjmpqmk10in55zg1jwlghdybcj
f6b-rustc-1.64.0/lib/rustlib/aarch64-apple-darwin/lib/libcompiler_builtins-9ba449e7b506e8b4.rlib" "-lico
nv" "-lSystem" "-lresolv" "-lc" "-lm" "-liconv" "-L" "/nix/store/gqjsjmpqmk10in55zg1jwlghdybcjf6b-rustc-
1.64.0/lib/rustlib/aarch64-apple-darwin/lib" "-o" "/private/tmp/nix-build-python3.10-sapling-0.1.2022111
8-210929-cfbb68aa.drv-0/source/eden/scm/build/cargo-target/release/deps/libconch_parser.dylib" "-Wl,-dea
d_strip" "-dynamiclib" "-Wl,-dylib" "-nodefaultlibs"
python3.10-sapling>   = note: Undefined symbols for architecture arm64:
python3.10-sapling>             "_PyTuple_SetItem", referenced from:
python3.10-sapling>                 cpython::objects::tuple::PyTuple::new::hb986eaf3710beb4e in libcpyth
on-b5f8dd225d1ddb56.rlib(cpython-b5f8dd225d1ddb56.cpython.ae93d97e-cgu.12.rcgu.o)
python3.10-sapling>             "_PyExc_TypeError", referenced from:
python3.10-sapling>                 cpython::err::PyErr::new::hd304cecd34745e9a in libcpython-b5f8dd225d
1ddb56.rlib(cpython-b5f8dd225d1ddb56.cpython.ae93d97e-cgu.2.rcgu.o)
python3.10-sapling>                 _$LT$cpython..err..PyErr$u20$as$u20$core..convert..From$LT$cpython..
python..PythonObjectDowncastError$GT$$GT$::from::hbd370ed32e977912 in libcpython-b5f8dd225d1ddb56.rlib(c
python-b5f8dd225d1ddb56.cpython.ae93d97e-cgu.2.rcgu.o)
python3.10-sapling>             "_PyEval_InitThreads", referenced from:
python3.10-sapling>                 cpython::py_module_initializer_impl::h013fdfaba2040c3b in libcpython
-b5f8dd225d1ddb56.rlib(cpython-b5f8dd225d1ddb56.cpython.ae93d97e-cgu.11.rcgu.o)
python3.10-sapling>                 std::sync::once::Once::call_once::_$u7b$$u7b$closure$u7d$$u7d$::h1ac
17a28dd67804b (.llvm.13657123622603539223) in libcpython-b5f8dd225d1ddb56.rlib(cpython-b5f8dd225d1ddb56.
cpython.ae93d97e-cgu.6.rcgu.o)
python3.10-sapling>                 core::ops::function::FnOnce::call_once$u7b$$u7b$vtable.shim$u7d$$u7d
$::h7d70fec1cc8a9a67 (.llvm.13657123622603539223) in libcpython-b5f8dd225d1ddb56.rlib(cpython-b5f8dd225d
1ddb56.cpython.ae93d97e-cgu.6.rcgu.o)
python3.10-sapling>             "_PyModule_Create2", referenced from:
python3.10-sapling>                 cpython::py_module_initializer_impl::h013fdfaba2040c3b in libcpython
-b5f8dd225d1ddb56.rlib(cpython-b5f8dd225d1ddb56.cpython.ae93d97e-cgu.11.rcgu.o)
python3.10-sapling>             "_PyModule_Type", referenced from:
python3.10-sapling>                 cpython::py_module_initializer_impl::h013fdfaba2040c3b in libcpython
-b5f8dd225d1ddb56.rlib(cpython-b5f8dd225d1ddb56.cpython.ae93d97e-cgu.11.rcgu.o)
python3.10-sapling>             "_PyType_IsSubtype", referenced from:
python3.10-sapling>                 cpython::py_module_initializer_impl::h013fdfaba2040c3b in libcpython
-b5f8dd225d1ddb56.rlib(cpython-b5f8dd225d1ddb56.cpython.ae93d97e-cgu.11.rcgu.o)
python3.10-sapling>             "_PyGILState_Ensure", referenced from:
python3.10-sapling>                 core::ptr::drop_in_place$LT$cpython..err..PyErr$GT$::h64851cdcb78893
d6 in libcpython-b5f8dd225d1ddb56.rlib(cpython-b5f8dd225d1ddb56.cpython.ae93d97e-cgu.8.rcgu.o)
python3.10-sapling>                 core::ptr::drop_in_place$LT$cpython..objects..list..PyList$GT$::h94c
70227e3b821a0 (.llvm.9899936352201448833) in libcpython-b5f8dd225d1ddb56.rlib(cpython-b5f8dd225d1ddb56.c
python.ae93d97e-cgu.8.rcgu.o)
python3.10-sapling>                 core::ptr::drop_in_place$LT$core..option..Option$LT$cpython..objects
..object..PyObject$GT$$GT$::he4c03d23d08bf731 in libcpython-b5f8dd225d1ddb56.rlib(cpython-b5f8dd225d1ddb
56.cpython.ae93d97e-cgu.8.rcgu.o)
python3.10-sapling>                 cpython::objectprotocol::_$LT$impl$u20$core..fmt..Debug$u20$for$u20$
cpython..objects..object..PyObject$GT$::fmt::h73b90813b5c02d9f in libcpython-b5f8dd225d1ddb56.rlib(cpyth
on-b5f8dd225d1ddb56.cpython.ae93d97e-cgu.8.rcgu.o)
python3.10-sapling>                 _$LT$cpython..objects..object..PyObject$u20$as$u20$core..ops..drop..
Drop$GT$::drop::h1d074b796de211ca in libcpython-b5f8dd225d1ddb56.rlib(cpython-b5f8dd225d1ddb56.cpython.a

Indeed, it is probably that comment you found in Cryptography. Notably, it is not being given the link flag to link to cpython, which is obviously why it's unresolved, due to being built in "extension module" mode (https://github.com/dgrunwald/rust-cpython/blob/master/python3-sys/build.rs#L347). But if it's expected to be built with those symbols expected to be unresolved, that now makes sense to me.

I would support merging this as-is and fix up macOS setup later unless it's obviously easy.

[pbar1]

I've looked at Darwin.

Darwin build log
Indeed, it is probably that comment you found in Cryptography. Notably, it is not being given the link flag to link to cpython, which is obviously why it's unresolved, due to being built in "extension module" mode (https://github.com/dgrunwald/rust-cpython/blob/master/python3-sys/build.rs#L347). But if it's expected to be built with those symbols expected to be unresolved, that now makes sense to me.

I would support merging this as-is and fix up macOS setup later unless it's obviously easy.

It would seem that's expected (unresolved) on macOS targets - believe I saw them in the setuptools_rust or PyO3 docs mentioned, but it was somewhat buried. The Sapling source does pass the right config in its file eden/scm/distutils_rust/__init.py__, but I haven't been able to replicate it via passing RUSTFLAGS to the build yet. Thanks for looking into it @lf-! I agree that releasing then filing a bug to fix the macOS build is the way to go.

[pbar1]

Ok - with this latest amend I've gotten sl web working. In doing so I had to refactor the derivation into a couple of intermediates that are then joined:

• ("sapling-isl") Extract the buildPythonPackage (till this point, the only derivation in the PR)
• ("sapling-main") Add another derivation to build the ISL server via yarn --offline along with fetchYarnDeps

The results of the above are then copied into the final complete derivation.

[pbar1]

@pbar1 The dependencies are the same for all platforms. The zip used on Windows is an optimization since reading too many small files are slower on Windows' filesystem. On other platforms, we use site-packages/ or just provide the dependencies as plain files for the dependencies. That said, they are mainly just IPython dependencies for the debugshell command, and toml for EdenFS on Windows support. So if they are missing most commands might still run. The documentation site has a few references to the debugshell command though, since it could be useful for developers to explore internals.

@quark-zju Good to know - that means we can filter out the zips in the Nix builds if they're only needed on Windows. As for the pyassets mainly being used for IPython - do you think any of them will be used not for IPython in the future? In any case, I'm in favor of keeping the functionality as is in this pr (we've got the generated python deps, debugshell should work).

[thoughtpolice]

This is looking great and the sl web fix is wonderful. I agree we should handle macOS in a separate issue as well unless it's trivial, it hopefully can be handled without making things unwieldly.

I'm got things to attend to at this exact second, but unless anyone else has objections, I plan on taking this branch for a short spin and merging it later tonight.

[thoughtpolice]

I ran several tests and ported several patches from my own repositories, rebased and used sl web, etc — everything looks good! Hats off and thanks to @pbar1 for all the work. Now other downstream stuff can start if we want.

[lf-]

I've root caused the Darwin issues. They're related to the cargo config that nixpkgs puts in overriding the cargo config that sapling adds, making its rust flags get ignored. Going to write a PR.

Update: PR written but it doesn't work at runtime so it's marked draft: #202754

[SuperSandro2000]

Basically looks good to me, assuming we're all on board with the Rust and Python stuff going on here. I could make one or two minor nits but it isn't worth it honestly.

As someone who has its sleeves really deep into nixpkgs python ecosystem the merged solution here is not acceptable from the view of python packages. If something is broken from the python side because of the non standard vendoring here then you are most likely out of luck and need to solve it yourself.

I'm not an expert in the Python dependency management world and haven't fully explored choices like poetry and others. If there is a format to declare the dependencies (requirements.txt?) more formally, I think we can support that.

nixpkgs supports setup.py/setuptools, requirements.txt, pyproject, flit, hatchling and probably some more. Anything would be better for us than directly downloading things from pypi and vendoring them.

@pbar1 The dependencies are the same for all platforms. The zip used on Windows is an optimization since reading too many small files are slower on Windows' filesystem. On other platforms, we use site-packages/ or just provide the dependencies as plain files for the dependencies. That said, they are mainly just IPython dependencies for the debugshell command, and toml for EdenFS on Windows support. So if they are missing most commands might still run. The documentation site has a few references to the debugshell command though, since it could be useful for developers to explore internals.

We do not have support for python on windows out of the box or target that, so nothing to worry about yet. Most of python build systems support making dependencies platform specific.

---

If you need this package a week after its release and are fine with using such workarounds, then having the nix file out of tree would be a better solution than having major compromises. I am not happy that this got merged at this stage.

[thoughtpolice]

As someone who has its sleeves really deep into nixpkgs python ecosystem the merged solution here is not acceptable from the view of python packages. If something is broken from the python side because of the non standard vendoring here then you are most likely out of luck and need to solve it yourself.

OK, I might have been wrong here, but — my understanding here was that generally it's ok if Python code is vendored from upstream to some extent, but it can't be exposed as any sort of package for downstream Python package consumers; i.e. you shouldn't be able to import sapling as a propagatedBuildInput into a python package (since it would then propagate its vendored packages into downstream consumers, which could cause conflicts.) You can't have two versions of one package, in other words.

But Sapling is not a python package, really, it's an application written in python which depends on a fixed set of Python dependencies, which we do not have, but nothing more than that. The problem I had with the previous version was hardcoding tons of stuff that would need to be tediously updated. The fact that they pick some specific dependencies is otherwise not a big deal, IMO.

Another way of putting it is like this — if they instead simply checked all the "python dependencies" into a directory under vendor/ in their subtree, we wouldn't even be having this discussion I suspect because the download code path wouldn't have needed to be written and wouldn't have cropped up in the code review path. But these aren't really very different (auto-downloading the deps from a schema vs just letting upstream check them in directly is, in the end, a distinction without a difference, unless I'm missing something huge, which is possible.)

However, in a twist of fate, it is a python package in the sense it also contains a sapling egg and it's exposed under $out/lib/python3.10/site-packages/..., so in theory it could be imported from a downstream consumer. This is problematic (e.g. a tool might want to call sl but also use python310.foobar, while sl itself needs some other version of foobar), but I'm not sure it's inherent; we could modify sapling or the derivation to otherwise handle that. (But consider that again this isn't exposed through pythonPackages, only through all-packages.nix because again, it can't work with just any Python; Python is simply an implementation detail, and in fact Python 3.10 is required right now, I think.)

I might not understand all the specific criteria the maintainers of the Python infrastructure have, though. Presumably it's somewhere in-between "duplicate every dependency willy nilly" — which would be bad — vs "literally never duplicate anything ever in history even if it's inside something that isn't really Python package" — which, I mean, if we can't leverage one of the actual major benefits of Nix existing, we might as well just give up.

We might want to discuss this in another issue or a revert PR if you're willing to file it, since this one is getting pretty long in the tooth.

[thoughtpolice]

Actually, disregard that last comment; I guess my eyes glazed over this part:

directly downloading things from pypi and vendoring them.

If this is the crux of the matter, something like a requirements.txt in there to use as a basis, we can probably sort something out in pretty short order, I'd guess.

[SuperSandro2000]

OK, I might have been wrong here, but — my understanding here was that generally it's ok if Python code is vendored from upstream to some extent, but it can't be exposed as any sort of package for downstream Python package consumers; i.e. you shouldn't be able to import sapling as a propagatedBuildInput into a python package (since it would then propagate its vendored packages into downstream consumers, which could cause conflicts.) You can't have two versions of one package, in other words.

This would definitely cause issues due to how bad pip handles this and is not allowed to be done.

Another way of putting it is like this — if they instead simply checked all the "python dependencies" into a directory under vendor/ in their subtree, we wouldn't even be having this discussion I suspect

If someone would have checked the upstream repo and found this probably, too because as a Distro we don't favor vendoring

Presumably it's somewhere in-between

Add cases like "upstream version pinning cannot be trusted because it pins 2 year old cryptography versions because they require rust to build now" which is not an issue for us.