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

Add 3.4-asan builds, try 2 #15

Merged
merged 5 commits into from
Feb 18, 2025
Merged

Conversation

ivoanjo
Copy link
Contributor

@ivoanjo ivoanjo commented Feb 17, 2025

What does this PR do?

This PR is a second attempt at adding 3.4-asan builds (first attempt was #13); this version is now atop #14 .

It introduces a new "3.4-asan" build, based on the existing asan builds, but just pointed at the ruby_3_4 branch.

In #13, we were building the latest tagged 3.4 release, which I expect would be more stable than just using ruby_3_4 (and thus better for my downstream purposes of "having a build that doesn't fail for non-asan-related reasons").

Switching between both options is as simple as:

diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml
index 7eb72a8..d7608d9 100644
--- a/.github/workflows/build.yml
+++ b/.github/workflows/build.yml
@@ -29,6 +29,8 @@ jobs:
       with:
         repository: ruby/ruby
         path: ruby
+        fetch-tags: true
         fetch-depth: 0
     - name: Set latest_commit
       id: latest_commit
       working-directory: ruby
@@ -37,7 +39,8 @@ jobs:
       id: latest_commit_3_4_asan
       working-directory: ruby
       run: |
-        git checkout ruby_3_4
+        LATEST_TAG=$(git tag --list | grep -E "v3_4_[0-9]+$" | sort -V | tail -n1)
+        git checkout "$LATEST_TAG"
         echo "commit=$(git rev-parse HEAD)" >> $GITHUB_OUTPUT
     - name: Check if latest commit already built
       uses: actions/github-script@v7

I personally prefer building from the tag, but happy to use the branch option if that's preferrable.

Motivation:

The intention of "3.4-stable" is to provide the latest up-to-date stable Ruby, so that we can reliably use it as a breaking CI step.

As discussed in ruby/setup-ruby#682, the current ruby-asan builds are a bit of a "sharp edge" when used in CI because they may break due to changes that are completely unrelated to asan.

Building asan rubies is a bit awkward still, as e.g. ruby-build and other version managers don't have support for it, and it requires very modern versions of specific system tools (e.g. clang).

Additional Notes:

In particular, I decided to not touch the logic that determines weather there's a more recent commit to build or not. This does mean that if ruby master sees no commits, but there's changes in the 3.4 branch, this won't be picked up immediately; and it also means that if there's a new master commit and no change to the 3.4 branch we still rebuild 3.4-asan.

My thinking is that given that #14 added caching already, this approach keeps things simple.

Let me know if you're not convinced, and I can change that.

How to test the change?

I've built this in the downstream fork, and manually downloaded the resulting Ruby and it seems to be in good shape and with asan working fine.

**What does this PR do?**

This PR is a second attempt at adding 3.4-asan builds (first attempt was
ruby#13); this version
is now atop ruby#14 .

It introduces a new "3.4-asan" build, based on the existing asan builds,
but just pointed at the `ruby_3_4` branch.

In ruby#13, we were building
the latest tagged 3.4 release, which I expect would be more stable
than just using `ruby_3_4` (and thus better for my downstream
purposes of "having a build that doesn't fail for non-asan-related
reasons").

Switching between both options is as simple as:

```diff
diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml
index 7eb72a8..d7608d9 100644
--- a/.github/workflows/build.yml
+++ b/.github/workflows/build.yml
@@ -29,6 +29,8 @@ jobs:
       with:
         repository: ruby/ruby
         path: ruby
+        fetch-tags: true
         fetch-depth: 0
     - name: Set latest_commit
       id: latest_commit
       working-directory: ruby
@@ -37,7 +39,8 @@ jobs:
       id: latest_commit_3_4_asan
       working-directory: ruby
       run: |
-        git checkout ruby_3_4
+        LATEST_TAG=$(git tag --list | grep -E "v3_4_[0-9]+$" | sort -V | tail -n1)
+        git checkout "$LATEST_TAG"
         echo "commit=$(git rev-parse HEAD)" >> $GITHUB_OUTPUT
     - name: Check if latest commit already built
       uses: actions/github-script@v7
```

I personally prefer building from the tag, but happy to use the
branch option if that's preferrable.

**Motivation:**

The intention of "3.4-stable" is to provide the latest up-to-date
stable Ruby, so that we can reliably use it as a breaking CI step.

As discussed in ruby/setup-ruby#682, the
current ruby-asan builds are a bit of a "sharp edge" when used in CI
because they may break due to changes that are completely unrelated to
asan.

Building asan rubies is a bit awkward still, as e.g. ruby-build and
other version managers don't have support for it, and it requires
very modern versions of specific system tools (e.g. clang).

**Additional Notes:**

In particular, I decided to not touch the logic that determines weather
there's a more recent commit to build or not. This does mean that if
ruby master sees no commits, but there's changes in the 3.4 branch,
this won't be picked up immediately; and it also means that if there's
a new master commit and no change to the 3.4 branch we still rebuild
3.4-asan.

My thinking is that given that ruby#14 added caching already, this approach
keeps things simple.

Let me know if you're not convinced, and I can change that.

**How to test the change?**

I've built this in the downstream fork, and manually downloaded the resulting Ruby and it seems to be in good shape and with asan working fine.

* Successful run: https://github.com/DataDog/ruby-dev-builder/actions/runs/13371638547
* Resulting builds: https://github.com/DataDog/ruby-dev-builder/releases/tag/v20250217.134317
@ivoanjo ivoanjo mentioned this pull request Feb 17, 2025
- name: Set latest_commit
id: latest_commit
working-directory: ruby
run: echo "commit=$(git rev-parse HEAD)" >> $GITHUB_OUTPUT

- name: Set latest commit (3.4-asan)
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it should be named 3.4-asan or asan-3.4.
On one hand for https://github.com/ruby/setup-ruby?tab=readme-ov-file#supported-version-syntax if we consider asan an "engine" it should be asan-3.4.
OTOH it's a special case anyway and I think we should hardcode it like other special head builds (e.g. no version matching).
IOW its engine would be asan-3.4 or 3.4-asan, no "version" from setup-ruby POV.

It's also "3.4 + ASAN" so 3.4-asan is kinda intuitive.
Let's stick with that for now.

@eregon
Copy link
Member

eregon commented Feb 17, 2025

Switching between both options is as simple as:

OK, that convinces me, let's do the latest tag thing then.

Copy link
Member

@eregon eregon left a comment

Choose a reason for hiding this comment

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

This looks good.
It's a bit wasteful to rebuild 3.4 every day even though it won't change, but I guess compared to the many other builds it's not that bad.

It'd be interesting to see if there is an easy/simple-enough way to reuse the previous 3.4-asan build when there is no new tag, that'd be perfect compute-time wise. That can be another PR.

@ivoanjo
Copy link
Contributor Author

ivoanjo commented Feb 18, 2025

It'd be interesting to see if there is an easy/simple-enough way to reuse the previous 3.4-asan build when there is no new tag, that'd be perfect compute-time wise. That can be another PR.

It seems doable, at the cost of a bit more branching/complexity...

Something along the lines of: record the tag built in the release notes, then update the code to parse it as well. Then adjust the slow matrix and reuse-slow to also trigger on this new condition on top of the manual one. May need a few extra adjustments to account for when adding new os, as there won't be a reusable build then.

@eregon eregon merged commit c6feac2 into ruby:master Feb 18, 2025
@eregon
Copy link
Member

eregon commented Feb 18, 2025

That sounds worth it to me, could you give it a try?

(FWIW I noticed in https://github.com/DataDog/ruby-dev-builder/actions/runs/13386925511 only debug and asan builds are failing but head are passing, that might be a motivation to do head builds more often so they are still done if there is a debug/asan-specific failure. Though the head builds might be broken too and the debug one might reveal it better. Anyway, just noticed but nothing to do here now, I'll just report debug is broken to CRuby)

@ivoanjo
Copy link
Contributor Author

ivoanjo commented Feb 18, 2025

That sounds worth it to me, could you give it a try?

Ack, I'll take a stab at it :)

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

Successfully merging this pull request may close these issues.

2 participants