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

Optionally push benchmark results to bencher.dev #6

Merged
merged 1 commit into from
Nov 7, 2023
Merged

Conversation

aochagavia
Copy link
Collaborator

Supersedes #4

Thanks @epompeii for the initial implementation!

@cpu do we need to adjust the flake file regarding the new git dependency? I tested nix develop followed by cargo check and it worked, so maybe no changes are necessary.

}

/// Sends the instruction counts to bencher.dev for visualization
pub async fn send_icounts_to_bencher(
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe make this a method for BencherDev?

@cpu
Copy link
Member

cpu commented Nov 7, 2023

@cpu do we need to adjust the flake file regarding the new git dependency? I tested nix develop followed by cargo check and it worked, so maybe no changes are necessary.

Yup! Because of the git patch the Cargo.lock isn't sufficient to specify the expected hashes for Nix. You'll see errors if you use nix build .# to build this branch's Rust application with Nix. When you use nix develop and then cargo the Nix parts are supplying the tooling but you're building with vanilla cargo instead of the package derivation.

We can fix the vendoring hashes for the git dependency, but I run into problems building bencher in the isolated Nix build environment that need more investigation.

Flake update for the cargoLock outputHashes:
39c39,49
<             cargoLock = { lockFile = "${crateDir}/Cargo.lock"; };
---
>             cargoLock = {
>               lockFile = "${crateDir}/Cargo.lock";
>               outputHashes = {
>                 "bencher_client-0.3.15" =
>                   "sha256-VWCQvcszAkny0VANH0o4FoIhe+6w0MgBJveWum44wS4=";
>                 "progenitor-0.4.0" =
>                   "sha256-XbZ0ioirI4sYYGyyhstNXEs2v3azlT5z8YE6Fyl3uB4=";
>                 "typify-0.0.14" =
>                   "sha256-PYX6I0qBTrCeRK6zMFxqwaRUvIrx9md7kGz+nRo/jIo=";
>               };
>             };
bencher_client build err:
   Compiling bencher_client v0.3.15 (https://github.com/bencherdev/bencher?rev=d0532976#d0532976)
error: failed to run custom build command for `bencher_client v0.3.15 (https://github.com/bencherdev/bencher?rev=d0532976#d0532976)`

Caused by:
  process didn't exit successfully: `/build/ci-bench-runner/target/release/build/bencher_client-6ab86101c605e1d5/build-script-build` (exit status: 101)
  --- stdout
  cargo:rerun-if-changed=../../services/console/src/content/api/swagger.json

  --- stderr
  thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Os { code: 2, kind: NotFound, message: "No such file or directory" }', /build/cargo-vendor-dir/bencher_client-0.3.1>
  note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

I think given the way this repo is being used we should remove the package derivation and just use Nix to manage the dev. environment. The package isn't very useful right now since the Ansible deployment isn't going to use it (it builds from src on the host). It probably isn't worth debugging this too deeply - we can merge as is and I'll remove the package from the flake.

Copy link
Member

@cpu cpu left a comment

Choose a reason for hiding this comment

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

Looks good!

@@ -67,12 +67,14 @@ Before starting, you will need:
* A GitHub application configured according to the [GitHub app](../readme.md#github-app) section of
the main README. The application **must** be installed on your fork of the rustls repo, with the required
permissions.
* If testing code changes to the `ci-bench-runner` app or Ansbile playbooks, a fork of this repository that
* (Optional) A Bencher.dev project and testbed, configured according to the
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* (Optional) A Bencher.dev project and testbed, configured according to the
* (Optional) A [Bencher.dev](https://bencher.dev/) project and testbed, configured according to the

@cpu
Copy link
Member

cpu commented Nov 7, 2023

I think given the way this repo is being used we should remove the package derivation and just use Nix to manage the dev. environment.

Here's a small patch for that: #7 No docs updates required, we only described the dev env in README.

@aochagavia aochagavia merged commit 41c634a into main Nov 7, 2023
7 of 8 checks passed
@aochagavia aochagavia deleted the bencher branch November 7, 2023 14:39
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