dd-trace-py icon indicating copy to clipboard operation
dd-trace-py copied to clipboard

chore(setup): early check for valid Rust toolchain

Open sanchda opened this issue 1 year ago • 5 comments
trafficstars

Adds an early check for a valid Rust toolchain. Since Rust isn't installed everywhere we might get built, this provides a clearer and more immediate signal that system dependencies must be resolved.

Uh, if there's some fancy, new-fangled fixture for this in buildtools or whatever, I'm ignorant. I tried to read the docs, but didn't see anything that did what I thought needed to be done here.

Here's a local failure Screenshot 2024-07-03 at 9 28 51 AM

And you can see it dumps out faster in CI if there's a problem. Screenshot 2024-07-03 at 9 36 02 AM

Screenshot 2024-07-03 at 9 36 07 AM

Checklist

  • [x] The PR description includes an overview of the change
  • [x] The PR description articulates the motivation for the change
  • [x] The change includes tests OR the PR description describes a testing strategy
  • [x] The PR description notes risks associated with the change, if any
  • [x] Newly-added code is easy to change
  • [x] The change follows the library release note guidelines
  • [x] The change includes or references documentation updates if necessary
  • [x] Backport labels are set (if applicable)

Reviewer Checklist

  • [ ] Title is accurate
  • [ ] All changes are related to the pull request's stated goal
  • [ ] Avoids breaking API changes
  • [ ] Testing strategy adequately addresses listed risks
  • [ ] Newly-added code is easy to change
  • [ ] Release note makes sense to a user of the library
  • [ ] If necessary, author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment
  • [ ] Backport labels are set in a manner that is consistent with the release branch maintenance policy

sanchda avatar Jul 03 '24 14:07 sanchda

  • #9709 Graphite 👈
  • main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @sanchda and the rest of your teammates on Graphite Graphite

sanchda avatar Jul 03 '24 14:07 sanchda

Might be nice if we could optionally build without extensions (because I like incidents).

romainkomorn-exdatadog avatar Jul 03 '24 15:07 romainkomorn-exdatadog

Datadog Report

Branch report: 07-03-chore_setup_early_check_for_valid_rust_toolchain Commit report: 3170952 Test service: dd-trace-py

:white_check_mark: 0 Failed, 175508 Passed, 1216 Skipped, 10h 0m 33.25s Total duration (29m 9.23s time saved)

Might be nice if we could optionally build without extensions (because I like incidents).

I think we need to do a few things before we can achieve that goal. Notably, we'd need to distinguish between extensions which are critical for core functionality (unfortunately, this includes the Rust extension which is the focus of this PR) and those that represent "optional" features (like profiling and appsec). At the same time, we'd need some controls in place to ensure that everything is built and shipped in our final wheel (we've missed this before, when certain extensions were optional).

We just need a lot more maturity around extensions before we can realistically get there. Right now, the default posture is to fail if "almost" anything that isn't necessary for ddtrace-as-a-product-platform-plus-all-the-products fails to build.

Maybe this is something we can talk about internally? We have more and more upcoming work dealing with moving functionality into native extensions, so it's definitely meaningful to get ahead of these issues.

sanchda avatar Jul 03 '24 15:07 sanchda

Benchmarks

Benchmark execution time: 2024-07-04 13:47:57

Comparing candidate commit 3170952bc74eb69556070bdb59db998d911f89ac in PR branch 07-03-chore_setup_early_check_for_valid_rust_toolchain with baseline commit f85625adceb18107014c3229b834a1df7e912395 in branch main.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 221 metrics, 9 unstable metrics.

pr-commenter[bot] avatar Jul 03 '24 16:07 pr-commenter[bot]

/merge

sanchda avatar Jul 04 '24 14:07 sanchda

:steam_locomotive: MergeQueue: waiting for PR to be ready

This merge request is not mergeable yet, because of pending checks/missing approvals. It will be added to the queue as soon as checks pass and/or get approvals. Note: if you pushed new commits since the last approval, you may need additional approval. You can remove it from the waiting list with /remove command.

Use /merge -c to cancel this operation!

dd-devflow[bot] avatar Jul 04 '24 14:07 dd-devflow[bot]

:steam_locomotive: MergeQueue: queue is disabled

Added to the queue but the mergequeue is not enabled for now.

Use /merge -c to cancel this operation!

dd-devflow[bot] avatar Jul 04 '24 15:07 dd-devflow[bot]

:steam_locomotive: MergeQueue: This merge request was already merged

This pull request was merged directly.

dd-devflow[bot] avatar Jul 04 '24 17:07 dd-devflow[bot]