Building Buck on Illumos #120
Comments
|
All these sound like things we should clean up. Moving to fs4. Fixes to protobuf stuff. We'll aim to reduce the delta you are maintaining, and hopefully eventually get to success. |
|
<3. I'm gonna send a PR with the protobuff bits right now, as it's very small and self-contained. |
|
I've got an internal diff moving to fs4. There is your protoc which I'm importing. For jemalloc I've got a diff to use that on linux/mac, rather than generic unix, which seems like the only flavours of unix it will actually work. I'm pushing those through the review/CI process internally and then they'll be in the repo (tomorrow if I get a diff review tonight, otherwise likely Monday). Then we'll be on to new and exciting errors :) |
|
I notice you upgraded rustyline and removed psutil - are both of those required too? |
|
ah yes, the dangers of reconstructing a story after the fact, I forgot about those too :) So, psutil has a bunch of errors if you try to compile it, and is only being used in-tree for one tiny thing, which on some platforms returns empty results already anyway, so my idea was to try and remove it for now, just to get things building. Rustyline also has vague illumos issues (I know I've run into them and hit a corner case, but I can't off the top of my head remember what that is) and I was upgrading it in-tree because it also failed to build for some reason; personally I tend to use reedline because I love nu shell, and it does seem to work well on illumos, but I'm not even sure whatever that edge case was is relevant here, so I wasn't suggesting removing it. I also didn't have a chance yet to dig into how integrated it is, but I assume it's fairly important, as readline often is. |
|
FWIW the psutil dep is probably only needed for straight-up Linux and macOS and can be gated to those two platforms only. Prior to the OSS release it was easiest to gate to Unix, but sounds like we should be a bit more precise with the conditional imports now. I can update this later this weekend. |
|
A PR to fix compilation of fs4 on illumos is here: al8n/fs4-rs#3 |
|
All my diffs are approved and running through CI - expect them to land in 2-5 hours. |
Summary: The intention of using environment variables is to override these paths. With the code as written, this will unconditionally panic if protoc_bin_vendored doesn't know where the protobuff compiler/includes are, even if we're using an environment variable to set these paths. With this change, we only unwrap when actually attempting to use the path from `protoc_bin_vendored`. Found as part of #120. Pull Request resolved: #121 Reviewed By: cjhopman Differential Revision: D44799077 Pulled By: ndmitchell fbshipit-source-id: 56db51961166df0870cb4ad07a220abfc21c5008
Summary: As requested in #120, let's gate psutil inclusion to only linux and macOS (where this code should only return valid results.) Reviewed By: ndmitchell, stepancheg Differential Revision: D44815147 fbshipit-source-id: 99d9fce17bafb5683a3e946e63ed72f7ad59e524
|
44b28fd should remove the psutil dep for non-linux/mac. |
|
Catching up on this, very cool to see all this activity! This error is way nicer now too :) buuuuuut!
success! Well, with this patch anyway: diff --git a/Cargo.toml b/Cargo.toml
index 22e2f2d70c..7bbc9117be 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -128,7 +128,7 @@ faccess = "0.2.3"
fancy-regex = "0.10.0"
flate2 = "1.0.22"
fnv = "1.0.7"
-fs2 = "0.4.3"
+fs4 = { git = "https://github.com/sunfishcode/fs4-rs", branch = "sunfishcode/illumos", features = ["sync"] }
futures = { version = "0.3.24", features = ["async-await", "compat"] }
futures-intrusive = "0.4"
glob = "0.3.0"
diff --git a/app/buck2_client_ctx/Cargo.toml b/app/buck2_client_ctx/Cargo.toml
index 7edd24b340..25bef4ba3c 100644
--- a/app/buck2_client_ctx/Cargo.toml
+++ b/app/buck2_client_ctx/Cargo.toml
@@ -16,7 +16,7 @@ chrono = { workspace = true }
crossterm = { workspace = true }
derivative = { workspace = true }
derive_more = { workspace = true }
-fs2 = { workspace = true }
+fs4 = { workspace = true }
futures = { workspace = true }
hex = { workspace = true }
httparse = { workspace = true }
diff --git a/app/buck2_client_ctx/src/build_count.rs b/app/buck2_client_ctx/src/build_count.rs
index 0c3bf71804..e18eda173f 100644
--- a/app/buck2_client_ctx/src/build_count.rs
+++ b/app/buck2_client_ctx/src/build_count.rs
@@ -15,7 +15,7 @@ use anyhow::Context;
use buck2_common::client_utils;
use buck2_core::fs::paths::abs_norm_path::AbsNormPathBuf;
use buck2_core::fs::paths::file_name::FileName;
-use fs2::FileExt;
+use fs4::FileExt;
use serde::Deserialize;
use serde::Serialize;
use tokio::io::AsyncReadExt;
diff --git a/app/buck2_client_ctx/src/daemon/client/mod.rs b/app/buck2_client_ctx/src/daemon/client/mod.rs
index 72b4201efe..21b64c9a18 100644
--- a/app/buck2_client_ctx/src/daemon/client/mod.rs
+++ b/app/buck2_client_ctx/src/daemon/client/mod.rs
@@ -19,7 +19,7 @@ use buck2_cli_proto::*;
use buck2_common::daemon_dir::DaemonDir;
use buck2_core::fs::fs_util;
use buck2_core::fs::paths::abs_norm_path::AbsNormPathBuf;
-use fs2::FileExt;
+use fs4::FileExt;
use futures::future::BoxFuture;
use futures::pin_mut;
use futures::stream;
diff --git a/starlark-rust/starlark/Cargo.toml b/starlark-rust/starlark/Cargo.toml
index 4f4906933c..6f063d7b7a 100644
--- a/starlark-rust/starlark/Cargo.toml
+++ b/starlark-rust/starlark/Cargo.toml
@@ -41,7 +41,7 @@ walkdir = "2.3"
serde = { version = "1.0", features = ["derive"] }
logos = "0.12"
serde_json = "1.0"
-rustyline = "7.1"
+rustyline = "9.1"
maplit = "1.0.2"
lsp-server = "0.5"
lsp-types = "0.93.0"and then $ cargo update -p rustix
$ cargo update -p nix
$ cargo update -p rustyline(these operations not reflected in the diff because of Once @sunfishcode 's PR lands and a new release is cut, I can send in a PR to actually update this. |
|
Fantastic! For info, the upgrade to fs4 had to get reverted because it pulls in an old rustix that has problems on Windows. If the fs4 diffs you have up get merged, then we should be good to do it again. If they don't get merged, I guess someone needs to fork to fs6... I'll work on an upgrade of rustyline. I see 11.0 is the latest, so will upgrade to that. You can ignore the future incompatibilities things - we'll take care of them when they become real issues, and they show up for everyone. |
|
With dd685b5 we've upgraded to rustyline 11.0. Unfortunately we can't upgrade to fs4 until al8n/fs4-rs#2 lands. |
|
fs4 0.6.4 is now released, which has the fixes for building on illumos. |
This fixes the build on illumos. Fixes facebook#120
Hey there! I'm excited about Buck, and want to give it a try generally. I think it could solve some problems that I have at work. But we use illumos for various things, which means sometimes stuff gets weird. I personally use Windows, so I'm not an expert on illumos either, so I am ultimately trying to get a program I don't know running on a platform I don't know well; fun times :)
So, here's where I'm at at the time of putting this together:
steveklabnik@f5a35c8
Let's talk about how I got there.
So, the first thing that goes wrong is:
Okay, illumos is a weird unix, but I thought nix supported it. Let's check in on nix: https://crates.io/crates/nix
Oh, the latest release is 0.26, not 0.19. Let's see if we can update it:
Oh my.
Turns out 0.19 is being brought in by the dependency on fs2. I haven't heard of that package before.
https://github.com/danburkert/fs2-rs last commit in 2018. Uh oh. Here's a comment from 3 weeks ago on a PR: danburkert/fs2-rs#42 (comment)
Okay! Let's sub in fs4! If we do that...
Not just that, but we're also now trying to build jemallocator, which says
I asked one of my co-workers about this, and he said
I saw things in upstream jemalloc that suggest it might, but the jemallocator crate says three linux targets and one mac target, not even windows! (I hear you've got windows builds though; I haven't tried it on my local computer, that will come soon enough though)
But, it seems like jemalloc's statistics API isn't actually required to get a build off. So maybe we could conditionally include it, not on illumos. Problem: it doesn't seem like that configuration exists for workspaces, so we have to re-duplicate it into each cargo.toml. Gross but I just want this to build first, we can figure out something cleaner later...
... but that also doesn't seem to work, it's still trying to build jemallocator anyway. Either I'm making a big mistake or it just ignores more complex condtitionals.
I'm also getting errors about protobuffs failing to build. turns out that the pre-built stuff isn't supported upstream. https://github.com/stepancheg/rust-protoc-bin-vendored/blob/master/protoc-bin-vendored/src/lib.rs#L61-L74
Well, that should be okay, I should be able to set
BUCK2_BUILD_PROTOC_INCLUDEandBUCK2_BUILD_PROTOCto override this. But that doesn't work either. I realized that this is because the code as written unconditionally calls that method above inprotoc-bin-vendored, and then unwraps it, so it always panics. The diff above includes a small change which I believe shouldn't make that happen anymore.So, finally, building this gives:
I am not sure how much time I'll be able to devote to figuring out this build, but will keep poking at it, and tracking how things seem to go.
The text was updated successfully, but these errors were encountered: