Skip to content
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

Eliminate 150 Vale Warnings #1276

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Evaan2001
Copy link

@Evaan2001 Evaan2001 commented Aug 22, 2024

Description

This PR only edits documentation material. The goal was to eliminate a significant chunk of vale warnings. I resolved 150 warnings from 13 files, constituting about 35% of total warnings.

I had a question about one particular warning. Whenever "execute" or "execution" was present in the text, vale would throw
Be careful with 'Execution', it’s profane in some cases. alex.ProfanityUnlikely . In the beginning, I replaced "execution" with "processing" or "operation". But then I started coming across proper nouns such as Remote Execution Protocol and Local Remote Execution (LRE). Unsure about removing "execution" from these, I purposely ignored these warnings. Let me know if y'all would like me to revert my earlier swapping of execution with process/operation or leave things be as they are currently.

Fixes # 1150

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

I went to the root directory and ran cargo test --all, which conducted 316 tests and my branch passed all of them. Since I was solely editing documentation, I wasn't expecting any tests to fail ...


This change is Reviewable

Copy link
Member

@allada allada left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, there was recently a big change on the documentation code. Would you mind rebasing?

Reviewable status: 0 of 2 LGTMs obtained, and 0 of 13 files reviewed, and pending CI: pre-commit-checks

@Evaan2001 Evaan2001 force-pushed the vale_upgrades_1 branch 2 times, most recently from cda7297 to 0e03113 Compare August 23, 2024 05:44
@Evaan2001
Copy link
Author

Evaan2001 commented Aug 23, 2024

@allada, I rebased and resolved the conflicts (there were 3 conflicting links and I accepted the new fixed links).

I'm surprised that some of the CI tests are failing. I only edited md and mdx files, which shouldn't break any code. I ran cargo test --all and passed all the tests. I also ran pre-commit run -a and all of the checks passed ("forbid-binary-files" got skipped as there were "no files to check").

I saw some error messages mention "Unsupported OS version"; I'm using MacOS 14.6.1 Sonoma on Apple silicon. Thoughts?

Copy link
Member

@aaronmondal aaronmondal left a comment

Choose a reason for hiding this comment

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

Wow, nice work!

Regarding the execution term, it's a but suboptimal, but I think we need to keep it since the RBE is the remote build execution protocol.

Reviewed 8 of 13 files at r1, 5 of 5 files at r2, all commit messages.
Reviewable status: 0 of 2 LGTMs obtained, and all files reviewed, and pending CI: docker-compose-compiles-nativelink (22.04), integration-tests (22.04), and 21 discussions need to be resolved


-- commits line 4 at r2:
nit: Add this to link the commit to the vale issue:

Towards #1086

README.md line 19 at r2 (raw file):

[![License](https://img.shields.io/badge/License-Apache_2.0-blue.svg)](https://opensource.org/licenses/Apache-2.0)

## What's NativeLink

nit: This looks a bit off. Maybe change it to ## About NativeLink


README.md line 21 at r2 (raw file):

## What's NativeLink

NativeLink is an efficient, high-performance build cache and remote operation system that accelerates software compilation and testing while reducing infrastructure costs. It optimizes build processes for projects of all sizes by intelligently caching build artifacts and distributing tasks across several machines.

I think it's fine to use the term execution here, since RBE stands for Remote Build Execution and NativeLink is essentially an implementation of that protocol.

To allow the term you can add [Ee]xecution to this file: https://github.com/TraceMachina/nativelink/blob/main/.github/styles/config/vocabularies/TraceMachina/accept.txt

Code quote:

operation

README.md line 35 at r2 (raw file):

   - Significantly reduces build times, especially for incremental changes

2. **Efficient Remote Execution**:

As above.


README.md line 45 at r2 (raw file):

## 🚀 Quickstart

To start, you can deploy NativeLink as a Docker image (as shown below) or by using a cloud-hosted solution provided by the NativeLink team - [NativeLink Cloud](https://app.nativelink.com). It's **FREE** for individuals, open-source projects, and cloud production environments, with support for unlimited team members.

Suggestion:

the

README.md line 45 at r2 (raw file):

## 🚀 Quickstart

To start, you can deploy NativeLink as a Docker image (as shown below) or by using a cloud-hosted solution provided by the NativeLink team - [NativeLink Cloud](https://app.nativelink.com). It's **FREE** for individuals, open-source projects, and cloud production environments, with support for unlimited team members.

Suggestion:

container

SECURITY.md line 35 at r2 (raw file):

Nix derivation hash tag the images. The latest pushed image
corresponds to the `main` branch. GitHub action producing an image signs that
image. Note that the [OCI workflow](https://github.com/TraceMachina/nativelink/actions/workflows/image.yaml) might take some time to publish the latest image.

This change looks wrong. (It's also a bit risky to change since it's part of the security related information. Feel free to omit it from this PR)


deployment-examples/chromium/README.md line 33 at r2 (raw file):

> the stack with `pulumi stack` and `pulumi destroy`.

Next, start some standard deployments. This step includes building and preparing the remote containers for use in the cluster.:

nit: line break at 80 characters


docs/src/content/docs/deployment-examples/on-prem-overview.mdx line 18 at r2 (raw file):

Each example leverages some latest, cutting-edge Kubernetes
configurations, but with a simplified architecture that prioritizes

Suggestion:

Each example leverages Kubernetes with a simplified architecture that prioritizes

docs/src/content/docs/faq/remote-execution.mdx line 7 at r2 (raw file):

---

Remote processing is a powerful feature that allows you to distribute build

Suggestion:

execution

docs/src/content/docs/faq/remote-execution.mdx line 14 at r2 (raw file):

- **Faster build and test operation**: By distributing actions across different nodes,
you can carry out builds and tests in parallel, reducing the time required.

Suggestion:

run

docs/src/content/docs/faq/remote-execution.mdx line 15 at r2 (raw file):

- **Faster build and test operation**: By distributing actions across different nodes,
you can carry out builds and tests in parallel, reducing the time required.
- **Consistent processing environment**: Remote processing ensures that all members of

Suggestion:

execution

docs/src/content/docs/faq/remote-execution.mdx line 17 at r2 (raw file):

- **Consistent processing environment**: Remote processing ensures that all members of
a development team are working in the same environment, reducing the
"it works on this machine" problem.

This is a figure of speech, so it should remain it works on my machine.

Code quote:

it works on this machine

docs/src/content/docs/faq/remote-execution.mdx line 22 at r2 (raw file):

For more information on how to get started with remote execution in NativeLink,
please refer to [NativeLink On-Prem Guide](https://docs.nativelink.com/introduction/on-prem).

Suggestion:

please refer to the [NativeLink On-Prem Guide](https://docs.nativelink.com/introduction/on-prem).

docs/src/content/docs/faq/rust.mdx line 12 at r2 (raw file):

all the languages that are fast and non garbage collected, Rust stands out as the
sole language that provides the necessary guarantees for writing asynchronous code
for several distributed systems that communicate over GRPC.

docs/src/content/docs/introduction/on-prem.mdx line 13 at r2 (raw file):

or specific infrastructure setups.

To provide this functionality, NativeLink is seamlessly

Suggestion:

flexibility

docs/src/content/docs/introduction/on-prem.mdx line 17 at r2 (raw file):

hard to ensure that the process of setting up NativeLink on
your own servers is as straightforward as possible and
provides comprehensive documentation to guide you through the process.

Let's remove this, it's unnecessary.

Suggestion:

deployable in an on-premises environment.

docs/src/content/docs/introduction/on-prem.mdx line 20 at r2 (raw file):

## Making your First Deployment
To get started with running NativeLink on-premises, it's recommended to take a

Suggestion:

consider taking

local-remote-execution/README.md line 5 at r2 (raw file):

NativeLink's Local Remote Execution is a framework to build, distribute, and
iterate on custom toolchain setups that are transparent, fully hermetic,
and reproducible across machines of the same system architecture, while a saving a lot of time.

local-remote-execution/README.md line 17 at r2 (raw file):

> [!NOTE]
> At the moment LRE works just on `x86_64-linux`.

Suggestion:

doesn't work on systems other than `x86_64-linux`.

local-remote-execution/README.md line 343 at r2 (raw file):

## 📐 Architecture

The original C++ and Java toolchain containers are virtually never instantiated.

Suggestion:

never

@Evaan2001
Copy link
Author

@aaronmondal sorry for my late reply, and thanks for going through all of my changes. I'll incorporate the changes you recommended shortly. Alongside [Ee]xecution, I'll also add [Ee]xecute to the list of accepted words and I'll revert my previous execution -> operation/processing changes.

Quick question about "works on my machine". I'm aware it's a common figure of speech. I changed it as Vale would warn against using "my". One way to bypass this could be to add this phrase to the list of accepted words; I'll have to test if phrases (multiple words = multiple spaces) get parsed as expected. Alternatively, we can ignore this warning for now. What do you think would be the most sustainable approach?

Towards TraceMachina#1086. This commit only edits documentation material. The goal was to eliminate a significant
chunk of vale warnings. I was able to resolve 150 warnings, which is about 35%.
@Evaan2001
Copy link
Author

@aaronmondal your suggestions have been incorporated. Have a look!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants