Convert run-checks script into a Rust binary

[Luni-4]

Pull Request Template

Checklist

• Confirm that run-checks.sh has been executed.

Related Issues/PRs

Provide links to relevant issues and dependent PRs.

Changes

This PR converts run-checks script into a Rust binary and replace it in CI. Part of #552

Testing

Run the script locally, please @antimora and @nathanielsimard double-check if it works correctly for you too

[antimora]

Thanks for translating the code to Rust. I tested locally and I have 3 comments:

1. I think we should a simple wrapper script in the root that would compile and execute for all target. This script should work for linux and window (universal script). So I came up quickly with the following. Please add ./run-checks.sh and chmod +x it.

#!/bin/bash
# This section will execute under Unix-like systems
if [ "$(uname)" != "Windows_NT" ]; then
  if [ ! -f ./scripts/run-checks ]; then
    rustc scripts/run-checks.rs --crate-type bin --out-dir scripts
  fi
  ./scripts/run-checks all
  exit
fi

# The following code is for Windows
: <<'WINDOWS_CODE'
@echo off
if not exist scripts\run-checks.exe (
  rustc scripts\run-checks.rs --crate-type bin --out-dir scripts
)
scripts\run-checks.exe all
exit
WINDOWS_CODE

This in theory should work on windows as well.

2. The print outputs are buffered. I think it's a better user experience if you let the output is displayed right away.
3. The output does not retain text colors. It was better to see text colors. It's quicker to find out what failed and passed.

[Luni-4]

Thanks for your review @antimora!

1. I think we should a simple wrapper script in the root that would compile and execute for all target. This script should work for linux and window (universal script). So I came up quickly with the following. Please add ./run-checks.sh and chmod +x it.

#!/bin/bash
# This section will execute under Unix-like systems
if [ "$(uname)" != "Windows_NT" ]; then
  if [ ! -f ./scripts/run-checks ]; then
    rustc scripts/run-checks.rs --crate-type bin --out-dir scripts
  fi
  ./scripts/run-checks all
  exit
fi

# The following code is for Windows
: <<'WINDOWS_CODE'
@echo off
if not exist scripts\run-checks.exe (
  rustc scripts\run-checks.rs --crate-type bin --out-dir scripts
)
scripts\run-checks.exe all
exit
WINDOWS_CODE

This in theory should work on windows as well.

I think this script should not be necessary because cargo and rustup can be installed on any of the platforms we want to support, so we can directly run run-checks binary. External dependencies, such as lavapipe, will be directly installed on GitHub Actions's virtual machines, while locally, a developer should already have that installed in order to build burn.

2. The print outputs are buffered. I think it's a better user experience if you let the output is displayed right away.

I've used println! to print the Command output on shell. So it's up to the println! macro dealing with all the information in a correct way. Although, it seems it's not possible to have an unbuffered output. We can only print the complete command output in one fell swoop. Is some of you aware of a possible way to do that without adding any dependency?

3. The output does not retain text colors. It was better to see text colors. It's quicker to find out what failed and passed.

Added! It seems it's not possible to do that for rustup since it does not implement a --color option. Opened an issue rust-lang/rustup#3433

[antimora]

About the first point I mentioned: on making the process of running checks more user-friendly. Since it's essential for every developer to run these checks with each PR, the current 3-step process (which includes reading the documentation on how to build and run the checks, building them, and then running them) can be a bit cumbersome. So, I'm proposing a one-step solution with ./run-checks.sh. This script will be a straightforward wrapper, designed to work on Linux, Mac, and Windows. It will simplify the workflow. What do you think?

[Luni-4]

I do not understand quite well your one-step solution, so I'm going to explain my yet-to-implement idea, perhaps it requires your cross-platform script and I can simply miss the point (or more points eh eh).

First of all, let's split the checks in two different environments.

Locally

You are using a specific OS, which could be Linux, MacOS, or Windows (for simplicity, just considering these three for now). You install all external dependencies which allow burn to be run in addition to rust toolchain. Now you can build run-checks.rs. When all of that is ready, you can run run-checks binary without any problem.

Continuous integration

You run all checks on virtual machines with a specific operating system, so you treat each virtual machine as if you were working locally. In this way you just need the run-checks binary to perform checks.

Let me know if there are some problems or flaws in this approach

[antimora]

The use case to which I am referring only relates to users manually executing the checks locally. All remaining cases, including CI, can remain unchanged.

To illustrate this use case, let me provide a before-and-after solution:

Currently:

1. User wants to submit a PR.
2. User clones the repo and makes changes.
3. Runs run-checks.sh to ensure the changes would be acceptable by CI.
4. Submits a PR, indicating that run-checks.sh passed.

With the new version:

1. User wants to submit a PR.
2. User clones the repo and makes changes.
3. User looks up README.md on how to run the checks.
4. Executes rustc scripts/run-checks.rs --crate-type bin --out-dir scripts.
5. Runs ./scripts/run-checks.
6. Submits a PR.

Admittedly, once the user compiles and knows what to do next time, it becomes easier. But I am worried about added friction. Also, with the new version, we need to account for an edge case that occurred to me: scripts/run-checks.rs is updated, but ./scripts/run-checks is outdated.

This compilation step is now required, and it's fine (because the script logic is cross-platform thanks to Rust). However, to make things easy for end users, we should have some sort of bootstrap script. This bootstrap script will remain minimal and not contain any logic.

In my above bootstrap script example, it was meant to be used for multiple OSes. But I think for Windows, one will still need a Linux subsystem. So to make sure the script can be executed for sure on Windows, Mac, and Linux, we will probably need two minimal bootstrap scripts:

run-checks.bat file:

rustc scripts\run-checks.rs --crate-type bin --out-dir scripts
scripts\run-checks.exe

run-checks.sh file:

rustc scripts\run-checks.rs --crate-type bin --out-dir scripts
scripts\run-checks

[Luni-4]

Thanks a lot @antimora for your detailed explanation and this piece of documentation! Now I can clearly see the problem and I agree with you.

I would only replace the bat script with a Powershell script which is now the default shell for Windows

[Luni-4]

@antimora

I removed the confirmation of running the scripts locally from the pull request template because our CI already performs all checks, so even if a contributor forgets to run the checks, the CI does that instead

[antimora]

I've used println! to print the Command output on shell. So it's up to the println! macro dealing with all the information in a correct way. Although, it seems it's not possible to have an unbuffered output. We can only print the complete command output in one fell swoop. Is some of you aware of a possible way to do that without adding any dependency?

Yes, there seems to be a way to print the outputs right away. So I checked out your PR locally and tested with the following changes. So instead of capturing the output from the command and printing again, you'll have to instruct to redirect the output the std out and std err out. Here is updated function (you'll have to change other parts accordingly, including rustup):

// Define and run a cargo command
fn run_cargo(command: &str, first_params: &[&str], second_params: &[&str], error: &str) {
    // Print cargo command
    println!(
        "\ncargo {} {} {}\n",
        command,
        first_params.join(" "),
        second_params.join(" ")
    );

    // Run cargo
    let mut child = Command::new("cargo")
        .arg(command)
        .args(first_params)
        .args(second_params)
        .stdout(Stdio::inherit()) // Send stdout directly to terminal
        .stderr(Stdio::inherit()) // Send stderr directly to terminal
        .spawn()
        .expect(error);

    // Wait for the command to finish
    let status = child.wait().expect("Failed to wait for child process");

    // Handle the exit status as you wish
    if !status.success() {
        std::process::exit(status.code().unwrap_or(1));
    }
}

I would recommend removing stdout_and_stderr_write function.

[antimora]

Thank you for improving the usability. I think we are almost done. Just these not too difficult issues should be fixed before merging. Please see my comments.

[Luni-4]

I've used println! to print the Command output on shell. So it's up to the println! macro dealing with all the information in a correct way. Although, it seems it's not possible to have an unbuffered output. We can only print the complete command output in one fell swoop. Is some of you aware of a possible way to do that without adding any dependency?

Yes, there seems to be a way to print the outputs right away. So I checked out your PR locally and tested with the following changes. So instead of capturing the output from the command and printing again, you'll have to instruct to redirect the output the std out and std err out. Here is updated function (you'll have to change other parts accordingly, including rustup):

// Define and run a cargo command
fn run_cargo(command: &str, first_params: &[&str], second_params: &[&str], error: &str) {
    // Print cargo command
    println!(
        "\ncargo {} {} {}\n",
        command,
        first_params.join(" "),
        second_params.join(" ")
    );

    // Run cargo
    let mut child = Command::new("cargo")
        .arg(command)
        .args(first_params)
        .args(second_params)
        .stdout(Stdio::inherit()) // Send stdout directly to terminal
        .stderr(Stdio::inherit()) // Send stderr directly to terminal
        .spawn()
        .expect(error);

    // Wait for the command to finish
    let status = child.wait().expect("Failed to wait for child process");

    // Handle the exit status as you wish
    if !status.success() {
        std::process::exit(status.code().unwrap_or(1));
    }
}

I would recommend removing stdout_and_stderr_write function.

Thank you! I had seen and tried that method before, but for some reasons it didn't work, but let me try again. Does that method print a colored output or it's just a pipe towards stdout and stderr?

[nathanielsimard]

Thanks a lot for improving the workflow, this will surely help with improving our CI and ease the pain of contributing to the project. 👏

[nathanielsimard]

@Luni-4 Once @antimora approves this PR, we can probably merge it! Thanks @antimora for reviewing and giving constructive feedback 🙏

[antimora]

Looks good. Thank you for improving and making it the script cross platform!

@nathanielsimard Ready to be merged.

[Luni-4]

Thanks to both of you for the detailed review and help! 😃