-
Notifications
You must be signed in to change notification settings - Fork 130
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Reorder README for Simplicity #563
Reorder README for Simplicity #563
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 3 files at r1, all commit messages.
Reviewable status: 0 of 1 LGTMs obtained, and pending CI: docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04)
nativelink-config/examples/basic_cas.jsonc
line 1 at r1 (raw file):
{
Is this a jsonc
a copy error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 3 files at r1.
Reviewable status: 0 of 1 LGTMs obtained, and pending CI: docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04)
README.md
line 15 at r1 (raw file):
## Download Native Link ### 🦀 Cargo
Suggestion:
Installing with Cargo
README.md
line 65 at r1 (raw file):
and `worker`. ## ❄️ Installing with Nix (Optional)
All our installation methods are optional. In fact, the Nix installation is the most robust one as it's the most thoroughly tested one. We're already suggesting Cargo at the top.
README.md
line 86 at r1 (raw file):
📦 Using the Docker image
We're not distributing images using the legacy Docker spec but OCI images per https://github.com/opencontainers/image-spec . Docker is used nowhere in the pipeline that creates these images.
If we need to clearify things for users unfamiliar with containers we could add something like this to the pull commands:
docker run \
-v basic_cas.json:/config.json \
ghcr.io/tracemachina/nativelink:wm6l3qi72hv9phn44853ir1dxs3q7k65
nativelink-config/examples/basic_cas.json
line 6 at r1 (raw file):
"memory": { "eviction_policy": { // 100mb.
We should just have one config file. Either this one or the .json5
ones. In that case we should make all files JSON5 though and do this change in a separate commit.
nativelink-config/examples/basic_cas.jsonc
line 1 at r1 (raw file):
{
If we need to clarify that we're not using "real" json, we need to use json5
here as were not following JSON-C but JSON5: https://json5.org/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 3 files at r1, all commit messages.
Reviewable status: 0 of 1 LGTMs obtained, and pending CI: docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 3 files at r1, all commit messages.
Reviewable status: 0 of 1 LGTMs obtained
README.md
line 18 at r1 (raw file):
```bash cargo install --git https://github.com/TraceMachina/nativelink --tag v0.1.0
Do we want to recommend a command that grabs the latest tag so we don't have to constantly update this?
README.md
line 29 at r1 (raw file):
```bash wget https://github.com/TraceMachina/nativelink/blob/main/nativelink-config/examples/basic_cas.json
Would recommend replacing with: curl -O https://raw.githubusercontent.com/TraceMachina/nativelink/main/nativelink-config/examples/basic_cas.json
as it comes pre-installed on many linux distros and on macos, as well as still working on windows post-install.
README.md
line 35 at r1 (raw file):
```bash cas --config=basic_cas.json
On macOS this only works if written as cas basic_cas.json
README.md
line 86 at r1 (raw file):
Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…
We're not distributing images using the legacy Docker spec but OCI images per https://github.com/opencontainers/image-spec . Docker is used nowhere in the pipeline that creates these images.
If we need to clearify things for users unfamiliar with containers we could add something like this to the pull commands:
docker run \ -v basic_cas.json:/config.json \ ghcr.io/tracemachina/nativelink:wm6l3qi72hv9phn44853ir1dxs3q7k65
I'll chime in again that I had no idea what an OCI image was but would understand what it was doing if docker was in the title even if inaccurate. Maybe split the difference and call it a container image which both Docker as the legacy spec and OCI kinda fall unde?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 LGTMs obtained
README.md
line 35 at r1 (raw file):
Previously, blakehatch (Blake Hatch) wrote…
On macOS this only works if written as
cas basic_cas.json
Also only working in this form when running in an Ubuntu docker container, on both this and MacOS the suggested command throws:
error: unexpected argument '--config' found
tip: to pass '--config' as a value, use '-- --config'```
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 LGTMs obtained
README.md
line 86 at r1 (raw file):
Previously, blakehatch (Blake Hatch) wrote…
I'll chime in again that I had no idea what an OCI image was but would understand what it was doing if docker was in the title even if inaccurate. Maybe split the difference and call it a container image which both Docker as the legacy spec and OCI kinda fall unde?
I think putting in a one liner like that for starting the server would be great. Some of the feedback today has been around simplicity of getting started, needs to be simplified to minimum number of commands that works across most contexts. Nix as the starting entry point to use turned out to be intimidating and not a 5 minute task to setup/use. On bootstrapping Nix for a fresh machine is non-trivial because of the side effects it needs todo (add users/groups, mount points, etc..), then first build of nativelink needs to download 4GB of code/artifacts before reaching the cargo compile phase.
A one liner docker command would be great here, could be misleading if first thing seen, client environment could be different from docker host server. Running cargo directly on host and bazel client on same host would yield higher success on first touch point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 LGTMs obtained
README.md
line 86 at r1 (raw file):
I'll chime in again that I had no idea what an OCI image was but would understand what it was doing if docker was in the title even if inaccurate. Maybe split the difference and call it a container image which both Docker as the legacy spec and OCI kinda fall unde?
Yes, something like Using the container image
would be correct.
9cc1ba5
to
b028401
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 LGTMs obtained
README.md
line 15 at r1 (raw file):
## Download Native Link ### 🦀 Cargo
Done.
README.md
line 29 at r1 (raw file):
Previously, blakehatch (Blake Hatch) wrote…
Would recommend replacing with:
curl -O https://raw.githubusercontent.com/TraceMachina/nativelink/main/nativelink-config/examples/basic_cas.json
as it comes pre-installed on many linux distros and on macos, as well as still working on windows post-install.
Done.
README.md
line 35 at r1 (raw file):
Previously, blakehatch (Blake Hatch) wrote…
Also only working in this form when running in an Ubuntu docker container, on both this and MacOS the suggested command throws:
error: unexpected argument '--config' found tip: to pass '--config' as a value, use '-- --config'```
Done.
README.md
line 65 at r1 (raw file):
Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…
All our installation methods are optional. In fact, the Nix installation is the most robust one as it's the most thoroughly tested one. We're already suggesting Cargo at the top.
Nix remains esoteric. I agree that could be where most users end up but it is not where they are today, simply based on early feedback.
nativelink-config/examples/basic_cas.json
line 6 at r1 (raw file):
Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…
We should just have one config file. Either this one or the
.json5
ones. In that case we should make all files JSON5 though and do this change in a separate commit.
Done.
nativelink-config/examples/basic_cas.jsonc
line 1 at r1 (raw file):
Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…
If we need to clarify that we're not using "real" json, we need to use
json5
here as were not following JSON-C but JSON5: https://json5.org/
Done.
nativelink-config/examples/basic_cas.jsonc
line 1 at r1 (raw file):
Previously, adam-singer (Adam Singer) wrote…
Is this a
jsonc
a copy error?
Done.
cb2b155
to
7ce87a2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 of 1 LGTMs obtained, and pending CI: Analyze (javascript-typescript), Analyze (python), Bazel Dev / ubuntu-22.04, Cargo Dev / ubuntu-22.04, Local / ubuntu-22.04, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), pre-commit-checks, publish-image, ubuntu-20.04, ubuntu-20.04 / stable
7ce87a2
to
718d5ba
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r4, 2 of 2 files at r5, all commit messages.
Reviewable status: 2 of 1 LGTMs obtained, and pending CI: Bazel Dev / ubuntu-22.04, Cargo Dev / ubuntu-22.04, Local / ubuntu-22.04, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), publish-image, ubuntu-20.04 / stable, ubuntu-22.04 / stable, windows-2022 / stable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 2 of 1 LGTMs obtained, and pending CI: Bazel Dev / ubuntu-22.04, Cargo Dev / ubuntu-22.04, Local / ubuntu-22.04, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), publish-image, ubuntu-20.04 / stable, ubuntu-22.04 / stable, windows-2022 / stable
README.md
line 65 at r1 (raw file):
Previously, MarcusSorealheis (Marcus Eagan) wrote…
Nix remains esoteric. I agree that could be where most users end up but it is not where they are today, simply based on early feedback.
It may be esoteric to users outside of the build system space but so are Bazel and Buck2. Virtually anything outside of CMake could be flagged as esoteric.
The Nix setup is the preferred setup for production deployments as it's the only one that is hermetic and reproducible. It's also the only setup that is compatible with all features of nativelink. Whether users like it or not, this
README.md
line 91 at r1 (raw file):
for pull commands. Images are tagged by nix derivation hash. The most recently pushed image
We can't remove this as it's the documentation for our tagging pattern and our signing process.
We could add a line as whe one in the discussion above to make it easier for users to get started.
README.md
line 96 at r1 (raw file):
might take a few minutes to publish the latest image. ```sh
We can't remove this. This signature verification process is a security hard requirement.
README.md
line 1 at r3 (raw file):
⚠️ This software is very early and still in an alpha state with many quirks. If you use our ambitious project and please share feedback.
This seems unnecessary. Things are running stable for use in production.
The currently unstable features are marked as experimental and don't have to be stable.
README.md
line 40 at r3 (raw file):
```bash cas -basic_cas.json
What does the -
do? Shouldn't it just be cas basic_cas.json
or cas ./basic_cas.json
?
README.md
line 91 at r3 (raw file):
📦 Working with Docker/Containers
One of our core values is being vendor neutral. Containers are vendor neutral and so are OCI images (which means the same thing). Docker is a non-vendor neutral vendor lock-in. The term "container" should be clear enough to users.
README.md
line 93 at r3 (raw file):
## 📦 Working with Docker/Containers The canonical container workflow is still being developed and should be released soon. For diehard Docker users, check out [deployment-examples/docker-compose](./deployment-examples/docker-compose/README.md).
We shouldn't recommend the docker-compose setup here. It's insecure, requires root permissions and can actually blow up our users systems since it's set up in a way that mounts directories into the local filesystem in a way that writes artifacts with root permissions.
#506 fixes these issues for CI by using the k8s setup instead. The docker compose setup will have to be reworked before we can start recommending it to users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 2 of 1 LGTMs obtained, and pending CI: Bazel Dev / ubuntu-22.04, Cargo Dev / ubuntu-22.04, Local / ubuntu-22.04, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), publish-image, ubuntu-20.04 / stable, ubuntu-22.04 / stable, windows-2022 / stable
README.md
line 65 at r1 (raw file):
Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…
It may be esoteric to users outside of the build system space but so are Bazel and Buck2. Virtually anything outside of CMake could be flagged as esoteric.
The Nix setup is the preferred setup for production deployments as it's the only one that is hermetic and reproducible. It's also the only setup that is compatible with all features of nativelink. Whether users like it or not, this
is a fully supported setup and shouldn't be marked as optional.
@aaronmondal It's no longer marked as optional but the change isn't reflected in Reviewable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fyi @MarcusSorealheis I'm working on a fix for the images. I think with that we can make the part around containers easier to digest while staying in line with best practices.
Reviewed all commit messages.
Reviewable status: 2 of 1 LGTMs obtained
@aaronmondal should your work on the containers hold up this README? We can open a new one when ready. We have people getting frustrated with the README currently. |
the fix for containers will take a day to evaluate with the entire ecosystem based on how people are using them. We also need to do that in a separate commit and revist the documentation then. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super sorry to block this but we can't publish it in this state. It's breaking our security guidelines which require us to publish signed images and be explicit and transparent about the signature process.
If it's a big issue we could just do the reordering. But we can't add the proposed changes regarding the container workflows.
We also can't remove the entire container section as that would require us to first remove all images that have been published until now (which is like 50 images).
Reviewable status: 2 of 1 LGTMs obtained
@aaronmondal My comment was that it was not ready. We didn't publish any workflow. I think removing everything is the safest. We can add it back to the README when it is ready. |
718d5ba
to
757e15c
Compare
@aaronmondal the concern is no longer valid as I have removed containers entirely from the README. We can add that back in later. So there are no outstanding issues. |
It's no longer relevant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 2 of 1 LGTMs obtained, and pending CI: Analyze (javascript-typescript), Analyze (python), Bazel Dev / ubuntu-22.04, Local / ubuntu-22.04, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), integration-tests (20.04), pre-commit-checks, publish-image, ubuntu-20.04, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable
README.md
line 93 at r3 (raw file):
Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…
We shouldn't recommend the docker-compose setup here. It's insecure, requires root permissions and can actually blow up our users systems since it's set up in a way that mounts directories into the local filesystem in a way that writes artifacts with root permissions.
#506 fixes these issues for CI by using the k8s setup instead. The docker compose setup will have to be reworked before we can start recommending it to users.
Can we just make mark this with Coming Soon
and link a ticket, but say we have a legacy Docker Compose
setup as a reference?
README.md
line 31 at r5 (raw file):
for more details and examples. To grab the example in your current working directory, run:
nit: This sentence does not make sense. I think you are saying to download
the an example into your cwd.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MarcusSorealheis We cannot remove the container section. We're already publishing binary packages (in the form of containers). As long as we do that we have to document the process with wich they're created and how to verify them. We can move it further down if that's more user friendly but we cannot remove it.
Reviewable status: 2 of 1 LGTMs obtained, and pending CI: Analyze (javascript-typescript), Analyze (python), Bazel Dev / ubuntu-22.04, Local / ubuntu-22.04, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), integration-tests (20.04), pre-commit-checks, publish-image, ubuntu-20.04, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable
just for the record, I'm extremely pro-Nix, and I want us to absolute prioritize making Nix a first class citizen. In case that is ever unclear, we will always support it. Rust is our primary tool today, and we are still working on Nix support. I will take months and many customers to get the workflow right. |
we are discussing the README. Nothing else. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aaronmondal , would you be ok if we made the images private and remove the reference for now?
Reviewable status: 2 of 1 LGTMs obtained, and pending CI: Analyze (javascript-typescript), Analyze (python), Bazel Dev / ubuntu-22.04, Local / ubuntu-22.04, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), integration-tests (20.04), pre-commit-checks, publish-image, ubuntu-20.04, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dismissed @adam-singer from a discussion.
Reviewable status: 2 of 1 LGTMs obtained
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@allada That would be ok. However, it would break all users that depend on this and would make it quite inconvenient to iterate on these pipelines. It would also stall development on our internal k8s infra.
I'd strongly prefer not to do this and instead to just move the section down and/or mark it as experimental.
Reviewable status: 2 of 1 LGTMs obtained
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
*or just wait for literally 20 min until we have tags up and can just cut a new release. Note that these workflows are not actually experimental. They're stable and currently documented correctly.
Reviewable status: 2 of 1 LGTMs obtained
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about simply hiding it from the main page?
Reviewable status: 2 of 1 LGTMs obtained
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@allada You mean keeping them public but just making them invisible? That would be completely fine. We'd still have to keep the docs around though since these are published binaries. But hiding them on the main page could be an improvement for UX if they're that much of an issue.
Reviewable status: 2 of 1 LGTMs obtained
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MarcusSorealheis WDYT about moving the container section 1:1 into the SECURITY.md
? That would be fine with me, wouln't break existing users and would still keep things documented and simplify the readme
Reviewable status: 2 of 1 LGTMs obtained
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 2 of 1 LGTMs obtained
README.md
line 93 at r3 (raw file):
Previously, allada (Nathan (Blaise) Bruer) wrote…
Can we just make mark this with
Coming Soon
and link a ticket, but say we have a legacyDocker Compose
setup as a reference?
Let's move the entire section to SECURITY.md
. Then we can remove it from this readme.
757e15c
to
7733fa3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed all commit messages.
Dismissed @blakehatch from 2 discussions.
Reviewable status: 2 of 1 LGTMs obtained, and pending CI: Analyze (javascript-typescript), Analyze (python), Bazel Dev / ubuntu-22.04, Cargo Dev / ubuntu-22.04, Local / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), pre-commit-checks, publish-image, ubuntu-20.04, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable
7733fa3
to
5215faa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r6, 1 of 1 files at r8, all commit messages.
Reviewable status: 2 of 1 LGTMs obtained, and pending CI: Analyze (javascript-typescript), Local / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), ubuntu-20.04 / stable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 2 of 1 LGTMs obtained, and pending CI: Analyze (javascript-typescript), Local / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), ubuntu-20.04 / stable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Reviewed all commit messages.
Reviewable status: 3 of 1 LGTMs obtained, and pending CI: Remote / large-ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 3 of 1 LGTMs obtained, and pending CI: Remote / large-ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04)
Description
Reorders the README so getting started does not require Nix.
Fixes (#517)
Type of change
Documentation.
Please delete options that are not relevant.
How Has This Been Tested?
Please also list any relevant details for your test configuration
Checklist
bazel test //...
passes locallygit amend
see some docsThis change is![Reviewable](https://camo.githubusercontent.com/1541c4039185914e83657d3683ec25920c672c6c5c7ab4240ee7bff601adec0b/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)