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

Allow TCL 9.0 for tests #1673

Draft
wants to merge 4 commits into
base: unstable
Choose a base branch
from
Draft

Conversation

zuiderkwast
Copy link
Contributor

Add 9.0 to list of allowed TCL versions for running the tests.

8.7 is removed. There is no such TCL version. I guess 8.7 was added in the past as an attempt to be future proof.

Signed-off-by: Viktor Söderqvist <[email protected]>
@zuiderkwast zuiderkwast added the run-extra-tests Run extra tests on this PR (Runs all tests from daily except valgrind and RESP) label Feb 5, 2025
@zuiderkwast
Copy link
Contributor Author

Fedora now comes with TCL 9.0 and we started getting failures like this: https://github.com/valkey-io/valkey/actions/runs/13147311114/job/36688160501

@zuiderkwast zuiderkwast added the test-failure An issue indicating a test failure label Feb 5, 2025
@zuiderkwast zuiderkwast marked this pull request as draft February 5, 2025 22:22
Signed-off-by: Viktor Söderqvist <[email protected]>
@hwware
Copy link
Member

hwware commented Feb 5, 2025

Which version is used in our online CI?

@zuiderkwast
Copy link
Contributor Author

Which version is used in our online CI?

Most of them have 8.6. Macos has 8.5. Fedora rawhide has now only 9.0. I guess we need to support all of them.

@zuiderkwast zuiderkwast marked this pull request as ready for review February 5, 2025 23:23
@zuiderkwast zuiderkwast requested review from hwware and enjoy-binbin and removed request for hwware February 5, 2025 23:25
@zuiderkwast
Copy link
Contributor Author

TCL 9.0 works now. The test-fedorarawhide-jemalloc job passed.

The remaining failures are other problems. One of them is solved in #1675.

@zuiderkwast
Copy link
Contributor Author

😱 Still fails with TLS:

version conflict for package "tcl": have 9.0.0, need 8.4

Need to solve this too.

Copy link

codecov bot commented Feb 5, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.06%. Comparing base (8d8ce19) to head (fa95058).

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #1673      +/-   ##
============================================
+ Coverage     71.05%   71.06%   +0.01%     
============================================
  Files           121      121              
  Lines         65243    65243              
============================================
+ Hits          46357    46365       +8     
+ Misses        18886    18878       -8     

see 12 files with indirect coverage changes

@zuiderkwast zuiderkwast marked this pull request as draft February 6, 2025 00:09
@zuiderkwast
Copy link
Contributor Author

zuiderkwast commented Feb 6, 2025

😓 The TCL TLS package doesn't seem to support TCL 9.0 yet. See here: https://core.tcl-lang.org/tcltls/wiki/Documentation

There's a beta version of this package 2.0 which will support TCL 9.0 according to this page: https://core.tcl-lang.org/tcltls/file?name=doc/tls.html&ci=trunk

Currently, fedora rawhide includes tcltls 1.7 which is incompatible with tcl 9.0 and effectively broken. I guess they will fix it soon.

Alternatives to this PR:

  • Download and build TCL 8.6, without using fedora's package repos.
  • Remove Fedora rawhide from our daily scope. It is the current development verions, like our unstable branch. Maybe we shouldn't actually run it in daily?

@enjoy-binbin
Copy link
Member

Remove Fedora rawhide from our daily scope. It is the current development verions, like our unstable branch. Maybe we shouldn't actually run it in daily?

i think we can go with this option.

@@ -294,7 +292,7 @@ proc parse_options {} {
incr j
set ::host ${val}
} elseif {$opt eq {--tls} || $opt eq {--tls-module}} {
package require tls 1.6
Copy link
Member

Choose a reason for hiding this comment

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

I think we should not touch tls lib

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tls 1.6 requires TCL 8.x and it rejects TCL 9.x.

If we want to use TCL 9.0, we need to use tls 2.0, which is only a beta version now. It's not yet released.

If we want to allow different TCL versions, we also need different TLS lib versions, so I think we can remove the hard-coded version requirements.

@madolson
Copy link
Member

madolson commented Feb 7, 2025

Remove Fedora rawhide from our daily scope. It is the current development verions, like our unstable branch. Maybe we shouldn't actually run it in daily?

I think having it here is good as it notifies us about upcoming issues, but I agree for the moment we shouldn't be blocked on it.

@zuiderkwast
Copy link
Contributor Author

@jonathanspw is looking at solving this in Fedora, probably add TCL 8 back.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run-extra-tests Run extra tests on this PR (Runs all tests from daily except valgrind and RESP) test-failure An issue indicating a test failure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants