Skip to content

Persist OPENSSL_PREFIX across builds - #4738

Merged
sarthakaggarwal97 merged 1 commit into
valkey-io:unstablefrom
sarthakaggarwal97:fix-openssl-prefix-persistence
Sep 18, 2026
Merged

sarthakaggarwal97 merged 1 commit into
valkey-io:unstablefrom
sarthakaggarwal97:fix-openssl-prefix-persistence

Conversation

@sarthakaggarwal97

Copy link
Copy Markdown
Contributor

Pointed out by @zuiderkwast in PR #3798.

Valkey remembers that TLS was enabled between builds, but not which OpenSSL installation was selected. After building with a custom OPENSSL_PREFIX, running a plain make later may discover a different system OpenSSL.

This changes the build flags, triggers an unexpected full rebuild, and links Valkey against an unintended OpenSSL installation.

Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>
@coderabbitai

coderabbitai Bot commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: f94bf83b-6f5a-44d8-a744-cfa8ed21382c

📥 Commits

Reviewing files that changed from the base of the PR and between 7b87fee and 5833a92.

📒 Files selected for processing (1)
  • src/Makefile

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

The persist-settings target now writes OPENSSL_PREFIX to .make-settings with the other build settings.

Changes

Build settings persistence

Layer / File(s) Summary
Persist OpenSSL prefix
src/Makefile
The persist-settings target records OPENSSL_PREFIX=$(OPENSSL_PREFIX) in .make-settings.

Priority: ⬇️ Low

Estimated code review effort: 1 (Trivial) | ~2 minutes

Suggested reviewers: yangbodong22011

Merge Risk: ⚪ Minimal · up to 5833a

The build setting is persisted and reused as intended, with no actionable merge-blocking risk remaining.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: persisting OPENSSL_PREFIX across builds.
Description check ✅ Passed The description explains the build behavior, the problem with losing OPENSSL_PREFIX, and the effect of the change. It directly relates to the changeset.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@sarthakaggarwal97 sarthakaggarwal97 added the run-extra-tests Run extra tests on this PR (Runs all tests from daily except valgrind and RESP) label Sep 18, 2026
@sarthakaggarwal97
sarthakaggarwal97 marked this pull request as ready for review September 18, 2026 21:10

@zuiderkwast zuiderkwast left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you!

@zuiderkwast zuiderkwast added the no-release-notes Don't mention in release notes label Sep 18, 2026
@sarthakaggarwal97 sarthakaggarwal97 added run-extra-tests Run extra tests on this PR (Runs all tests from daily except valgrind and RESP) and removed run-extra-tests Run extra tests on this PR (Runs all tests from daily except valgrind and RESP) labels Sep 18, 2026
@codecov

codecov Bot commented Sep 18, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.60%. Comparing base (7b87fee) to head (5833a92).

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #4738      +/-   ##
============================================
- Coverage     80.67%   80.60%   -0.08%     
============================================
  Files           192      192              
  Lines        100856   100856              
============================================
- Hits          81369    81298      -71     
- Misses        19487    19558      +71     

see 22 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@sarthakaggarwal97 sarthakaggarwal97 added run-extra-tests Run extra tests on this PR (Runs all tests from daily except valgrind and RESP) and removed run-extra-tests Run extra tests on this PR (Runs all tests from daily except valgrind and RESP) labels Sep 18, 2026
@sarthakaggarwal97

Copy link
Copy Markdown
Contributor Author

Daily Run - https://github.com/valkey-io/valkey/actions/runs/35395372072

I think the label even caused by no-release-notes label made the daily run hide and not show up in the PR CI

@sarthakaggarwal97

Copy link
Copy Markdown
Contributor Author

The daily run looks okay! Merging it! thanks @zuiderkwast

@sarthakaggarwal97
sarthakaggarwal97 merged commit 11387e5 into valkey-io:unstable Sep 18, 2026
117 of 118 checks passed
@github-project-automation github-project-automation Bot moved this to To be backported in Valkey 8.1 Sep 18, 2026
@github-project-automation github-project-automation Bot moved this to To be backported in Valkey 7.2 Sep 18, 2026
@github-project-automation github-project-automation Bot moved this to To be backported in Valkey 9.0 Sep 18, 2026
@github-project-automation github-project-automation Bot moved this from Todo to Merged in Valkey 9.2 Sep 18, 2026
@github-project-automation github-project-automation Bot moved this to To be backported in Valkey 8.0 Sep 18, 2026
@github-project-automation github-project-automation Bot moved this to To be backported in Valkey 9.1 Sep 18, 2026
valkeyrie-ops Bot pushed a commit that referenced this pull request Sep 19, 2026
Pointed out by @zuiderkwast in [PR
#3798](#3798 (comment)).

Valkey remembers that TLS was enabled between builds, but not which
OpenSSL installation was selected. After building with a custom
`OPENSSL_PREFIX`, running a plain `make` later may discover a different
system OpenSSL.

This changes the build flags, triggers an unexpected full rebuild, and
links Valkey against an unintended OpenSSL installation.

Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>
Backport-Source-PR: 4738
valkeyrie-ops Bot pushed a commit that referenced this pull request Sep 19, 2026
Pointed out by @zuiderkwast in [PR
#3798](#3798 (comment)).

Valkey remembers that TLS was enabled between builds, but not which
OpenSSL installation was selected. After building with a custom
`OPENSSL_PREFIX`, running a plain `make` later may discover a different
system OpenSSL.

This changes the build flags, triggers an unexpected full rebuild, and
links Valkey against an unintended OpenSSL installation.

Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>
Backport-Source-PR: 4738
valkeyrie-ops Bot pushed a commit that referenced this pull request Sep 19, 2026
Pointed out by @zuiderkwast in [PR

Valkey remembers that TLS was enabled between builds, but not which
OpenSSL installation was selected. After building with a custom
`OPENSSL_PREFIX`, running a plain `make` later may discover a different
system OpenSSL.

This changes the build flags, triggers an unexpected full rebuild, and
links Valkey against an unintended OpenSSL installation.

Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>
Backport-Source-PR: 4738
valkeyrie-ops Bot pushed a commit that referenced this pull request Sep 19, 2026
Pointed out by @zuiderkwast in [PR
#3798](#3798 (comment)).

Valkey remembers that TLS was enabled between builds, but not which
OpenSSL installation was selected. After building with a custom
`OPENSSL_PREFIX`, running a plain `make` later may discover a different
system OpenSSL.

This changes the build flags, triggers an unexpected full rebuild, and
links Valkey against an unintended OpenSSL installation.

Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>
Backport-Source-PR: 4738
valkeyrie-ops Bot pushed a commit that referenced this pull request Sep 19, 2026
Pointed out by @zuiderkwast in [PR
#3798](#3798 (comment)).

Valkey remembers that TLS was enabled between builds, but not which
OpenSSL installation was selected. After building with a custom
`OPENSSL_PREFIX`, running a plain `make` later may discover a different
system OpenSSL.

This changes the build flags, triggers an unexpected full rebuild, and
links Valkey against an unintended OpenSSL installation.

Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>
Backport-Source-PR: 4738
@zuiderkwast

Copy link
Copy Markdown
Contributor

@sarthakaggarwal97 Notice: This PR isn't in "To be backported" for 9.2 and valkyrie-ops didn't backport it to the 9.2 branch.

@sarthakaggarwal97 sarthakaggarwal97 moved this from Merged to To be backported in Valkey 9.2 Sep 19, 2026
@sarthakaggarwal97

Copy link
Copy Markdown
Contributor Author

Yeah, that's weird. Need to fix project rules for 9.2

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

Labels

no-release-notes Don't mention in release notes

Projects

Status: To be backported
Status: To be backported
Status: To be backported
Status: To be backported
Status: To be backported
Status: To be backported

Development

Successfully merging this pull request may close these issues.

2 participants