Avoid the use of the rb_eval_cmd_kw function if it is not available

[jeremyevans]

Fixes #75

[rhenium]

ruby/ruby#15404 intended to deprecate it, not actually remove it. We should fix it there, and then have_func will detect it as available again.

Both rb_eval_cmd_kw() and rb_funcallv_kw() seem to be from Ruby 2.7. Can we just use the rb_funcallv_kw() path always?

[jeremyevans]

ruby/ruby#15404 intended to deprecate it, not actually remove it. We should fix it there, and then have_func will detect it as available again.

I agree. I'm not sure why the deprecation code does not work as expected. However, even if it is only deprecated and not actually removed, we shouldn't be using it in the tk gem, just as we would avoid other deprecated methods.

Both rb_eval_cmd_kw() and rb_funcallv_kw() seem to be from Ruby 2.7. Can we just use the rb_funcallv_kw() path always?

I don't think we can always use rb_funcallv_kw(), because that does not evaluate a String. I do not know whether TkUtil.eval_cmd is designed to evaluate a String, but it is public API, and does currently support it:

$ ruby -r tk -e "p TkUtil.eval_cmd('1+1')"
2

[rhenium]

Sorry, I simplified it too much. I meant that the new code path added in this PR, using rb_eval_string_protect() and rb_funcallv_kw() depending on the argument type, could probably replace the usage of rb_eval_cmd_kw() for all Ruby versions >= 2.7.

I submitted a PR to ruby/ruby to restore the declaration with a proper deprecation attribute: ruby/ruby#15435

[jeremyevans]

While we could continue to use rb_eval_cmd_kw(), we should use something else if it is deprecated. We should only use a deprecated method if that is the only way to handle things. If gems supported by Ruby core ignore deprecations, what message does that send to other gem authors?

If the approach in this PR is too ugly, and there is no way to improve it and keep backwards compatibility, then I think we should not deprecate rb_eval_cmd_kw(). Hopefully @nobu can provide his thoughts on this.

[rhenium]

While we could continue to use rb_eval_cmd_kw(), we should use something else if it is deprecated.

Agreed. AFAIK mkmf doesn't provide a straightforward way to check if a function is deprecated or not, so I think it's simpler to remove the rb_eval_cmd_kw() usage entirely, regardless of the Ruby version.

IMHO, the functionality provided by rb_eval_cmd*() feels somewhat out of place, so deprecating it makes sense to me.

[jeremyevans]

While we could continue to use rb_eval_cmd_kw(), we should use something else if it is deprecated.

Agreed. AFAIK mkmf doesn't provide a straightforward way to check if a function is deprecated or not, so I think it's simpler to remove the rb_eval_cmd_kw() usage entirely, regardless of the Ruby version.

Once your Ruby PR is merged, the new code in this PR would not be used. It would only start being used once Ruby actually drops the method. I think that's acceptable. That allows us to keep full backwards compatibility for as long as possible. While I think this PR is backwards compatible, I'm not 100% sure it is in all respects.

If Ruby adds support for checking for deprecated functions, I would be in favor of using it and switching to this new code if deprecation is detected.

IMHO, the functionality provided by rb_eval_cmd*() feels somewhat out of place, so deprecating it makes sense to me.

I agree. I don't know the historical reason behind having a single function that either evaluates a string or calls a proc, instead of having two separate functions.

[rhenium]

rb_eval_cmd_kw() in https://github.com/ruby/ruby/blob/941e70ab38d01d067b7bbbcdf8553893a9ca8b49/vm_eval.c#L2148-L2179 doesn't suppress exceptions. The rb_protect() around the eval and funcall should be unnecessary.

[jeremyevans]

Thank you. I misunderstood the purpose of the tag code in rb_eval_cmd_call_kw when trying to recreate the behavior. I'll update the PR.

[rhenium]

I also think that's acceptable. My suggestion is if we really want to avoid all deprecation warnings.

[jeremyevans]

Ruby 4.0 shipped with the fix so the rb_eval_cmd_kw function is deprecated but not removed. So tk still builds with it, without this change. So there currently isn't a rush to merge this.