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

chore(ci): add rust toolchain to ci environments #9235

Merged
merged 8 commits into from
May 20, 2024

Conversation

brettlangdon
Copy link
Member

@brettlangdon brettlangdon commented May 11, 2024

Not currently being used, but will be needed in the future.

Testing

These are the necessary edits I've need on #9232 in order to get CI to be green for building the package in CircleCI and cibuildwheel.

Checklist

  • Change(s) are motivated and described in the PR description
  • Testing strategy is described if automated tests are not included in the PR
  • Risks are described (performance impact, potential for breakage, maintainability)
  • Change is maintainable (easy to change, telemetry, documentation)
  • Library release note guidelines are followed or label changelog/no-changelog is set
  • Documentation is included (in-code, generated user docs, public corp docs)
  • Backport labels are set (if applicable)
  • If this PR changes the public interface, I've notified @DataDog/apm-tees.

Reviewer Checklist

  • Title is accurate
  • All changes are related to the pull request's stated goal
  • Description motivates each change
  • Avoids breaking API changes
  • Testing strategy adequately addresses listed risks
  • Change is maintainable (easy to change, telemetry, documentation)
  • Release note makes sense to a user of the library
  • 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

@brettlangdon brettlangdon added the changelog/no-changelog A changelog entry is not required for this PR. label May 11, 2024
@brettlangdon brettlangdon requested a review from a team as a code owner May 11, 2024 11:59
@datadog-dd-trace-py-rkomorn
Copy link

datadog-dd-trace-py-rkomorn bot commented May 11, 2024

Datadog Report

Branch report: brettlangdon/ci.add.rust
Commit report: 4eb3545
Test service: dd-trace-py

✅ 0 Failed, 116951 Passed, 58974 Skipped, 3h 53m 9.51s Total duration (2h 57m 23.78s time saved)

@codecov-commenter
Copy link

codecov-commenter commented May 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 10.34%. Comparing base (3c03cd0) to head (4eb3545).
Report is 1 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #9235       +/-   ##
===========================================
- Coverage   76.16%   10.34%   -65.82%     
===========================================
  Files        1287     1257       -30     
  Lines      121888   120131     -1757     
===========================================
- Hits        92831    12425    -80406     
- Misses      29057   107706    +78649     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pr-commenter
Copy link

pr-commenter bot commented May 11, 2024

Benchmarks

Benchmark execution time: 2024-05-20 15:46:33

Comparing candidate commit ba93f51 in PR branch brettlangdon/ci.add.rust with baseline commit 3c03cd0 in branch main.

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

@brettlangdon brettlangdon enabled auto-merge (squash) May 11, 2024 14:01
@brettlangdon brettlangdon changed the title chore(ci): add rust toolchain to slotscheck and conftests jobs chore(ci): add rust toolchain to ci environments May 11, 2024
@brettlangdon
Copy link
Member Author

Still not getting builds passing 100%

https://github.com/DataDog/dd-trace-py/actions/runs/9045298537/job/24854896593#step:7:1971

rustup: installer for platform 'i686-unknown-linux-musl' not found, this may be unsupported

I have tried adding that target and it still fails.

This PR as-is is still progress.

@emmettbutler emmettbutler self-requested a review May 14, 2024 13:23
@emmettbutler emmettbutler marked this pull request as draft May 14, 2024 13:24
auto-merge was automatically disabled May 14, 2024 13:24

Pull request was converted to draft

@brettlangdon
Copy link
Member Author

I haven't figured out how to get the musl target to properly install/be available yet, so will hold until I figure that out.

@brettlangdon brettlangdon requested a review from a team May 14, 2024 22:47
@brettlangdon brettlangdon marked this pull request as ready for review May 14, 2024 22:48
@brettlangdon
Copy link
Member Author

I won't enable auto-merge for this, we first need to discuss if it is safe/advisable for us to drop musllinux-i686 builds.

We will have to do that whenever we add Rust since it doesn't support that as a target, but we need to try and determine if any of our users are actively using it.

This is considered a breaking change right? Since this would be dropping system support that we say we support.

@brettlangdon
Copy link
Member Author

Moving back to a draft, since there are some options we can possibly take to still support musllinux-i686

From @sanchda:

  • Random thoughts
    • i686 musl is a tier 2 target with std support, so the language should build
    • the fact that i686 linux unknown exists and is a thing makes me think that i686 support is mostly there (although certain things may be missing for certain libraries)
  • possible alternatives to dropping
    • cross-compile for an i686-musl target from x86_64-musl. Conceptually this should work at the compiler level
    • cross-build for an i686-musl target from an i686-gnu target. I used to do this for C and C++, but I’m not sure how hard it is to do it for rust.
    • build the required ecosystem for i686-musl from scratch, use it as a base image for Docker

@brettlangdon brettlangdon removed the request for review from majorgreys May 15, 2024 12:43
@romainkomorndatadog
Copy link
Collaborator

I know this is kind of off-topic, but I'd really want to make sure that whatever changes we make in CI are easily reproduced locally for dev scenarios. I don't know if that means modifying the testrunner image, or adding hatch/riot things, so that we can easily build and test in it, but it's important that the dev and CI workflows converge rather than diverge.

@brettlangdon
Copy link
Member Author

I know this is kind of off-topic, but I'd really want to make sure that whatever changes we make in CI are easily reproduced locally for dev scenarios. I don't know if that means modifying the testrunner image, or adding hatch/riot things, so that we can easily build and test in it, but it's important that the dev and CI workflows converge rather than diverge.

@romainkomorndatadog this is already done, there just happens to be a handful of different places where we need to add rust compiler :/

e.g. ./scripts/ddtest already has rust compiler.

if you want to build locally you'll need to install rust on your laptop, but assume that isn't a massive barrier (other than we'll want to make sure repo onboarding docs include that step).

@brettlangdon
Copy link
Member Author

We have figured out how to get builds working for musl linux i686. We have to use the custom image from #9319

@brettlangdon brettlangdon marked this pull request as ready for review May 20, 2024 16:02
@brettlangdon brettlangdon enabled auto-merge (squash) May 20, 2024 16:02
@brettlangdon brettlangdon merged commit 182bede into main May 20, 2024
151 of 152 checks passed
@brettlangdon brettlangdon deleted the brettlangdon/ci.add.rust branch May 20, 2024 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog/no-changelog A changelog entry is not required for this PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants