Merged
Conversation
Member
Author
|
@quark-zju: this might be interesting to you. If you're not familiar with Nix, you can build this package by checking out my branch and running |
13 tasks
Member
Author
Maybe this is interesting? I did |
ofborg
bot
added
8.has: package (new)
Member
Author
|
I have unified then diffed the file lists from the official distribution and the one from nixpkgs and it doesn't look like we're missing files or anything like that. More investigation required. |
Contributor
Hmm, running these commands using the Darwin build always emits a watchman message? AFAIK watchman is an optional dependency, at least on Linux. Is it possible that it's required on macOS? |
Member
Author
|
Maybe? I think it's a red herring then, it seems wrong that it should make it not clone a repo. |
Member
Author
|
it would be worthwhile to try the official binary distribution in that case i think, and see if the debug output says more stuff. maybe another day, I'm done debugging this for the minute. |
Contributor
|
Yeah and I checked the Homebrew package spec, no mention of Watchman as a dependency either |
Member
|
Note that sapling also starts a background daemon when it's first invoked that continues to exist under your login session; this is true of both macOS and Linux (and probably Windows.) I suspect there's some potential interaction here between the two that might be causing the fault. See also: facebook/sapling#261 |
Member
|
Also, with my latest commit as of 8d7badd, this branch is slightly out of date, sorry about that. JFYI, you'll need to conditionally make |
Member
Author
|
ah YES, I saw that bug now. I bet that's related. |
Member
Author
|
I have tried |
Member
Author
|
Confirmed still happening on tip of |
Member
Author
|
Here's the debug logs (finally figured out how to get those with Debug logs |
Member
Author
|
I found that the daemon was implicated, but in a more interesting way: it was masking a segfault. Thanks to upstream help, I found out about |
Member
Author
|
I found out that this is a Python 3.10 related bug, so I downgraded this package to use Python 3.8 instead. It now works on macOS. |
Contributor
@lf- Curious, what was the source of the incompatibility with 3.10 on macOS? |
Member
Author
Issue linked above: it's a silent segmentation fault. a fix has been sent for review internally apparently but I'm not sure when it will come out. |
Member
|
Sorry for the delay, I've been under the weather a bit the past day and a half. This does look good aside from the one minor |
There were two factors here: our cargo hook was messing up the cargo config, which broke the build, and also an upstream bug where Sapling didn't work on Python 3.10. The upstream issue was filed as facebook/sapling#279 We can get rid of the python 3.8 override as soon as this patch gets into a released version.
Member
Author
|
I've made the requested change and added a reminder comment to delete the python 3.8 override on the next update. |
Contributor
|
I've been testing this commit on One tangential piece of feedback in the meantime (which IMO it's totally fine to address via a future PR if you'd rather keep this one narrowly scoped): I've noticed in casually poking around that there are some |
pbar1
approved these changes
Dec 1, 2022
Contributor
That's something I'd wrestled with when making the original derivation; these are optional dependencies and not strictly required for core |
Contributor
|
That makes sense at an abstract level, and again, I see no need to block on it here, but fwiw, I do see two wrinkles at a more concrete level, one related to UX and one related to the semantics of the notional For the first: IMO, plenty of people who try to use this are going to expect For the second: while this obviously may evolve in the future, for today, issues like facebook/sapling#177 suggest to me that the sapling implementation is presently actually quite tightly coupled to the GitHub graphql API. As a result... do you have a sense of how likely the notional |
Member
Author
|
i think we're talking about a difference analogous to that between git and gitFull. for what it's worth, the github coupled functionality is largely just stuff on the periphery such as the submit/pr functionality. the actual fetching of repos and interaction generally just shells out to git. |
Contributor
|
|
thoughtpolice
approved these changes
Dec 1, 2022
Member
|
We could do it the other way around too: sapling by default could have a |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of changes
Fixed compilation of Sapling on Darwin. One small problem is that it doesn't actually work at all: when I run
sl clone https://github.com/nixos/nixpkgsit completes instantly (it shouldn't be that fast) and has no working tree or history. However, I think that this is a useful starting point for people to debug further, so I'm posting this as-is.Fixes #202703
Here's what it does:
Things done
sandbox = trueset innix.conf? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)nixos/doc/manual/md-to-db.shto update generated release notes