-
-
Notifications
You must be signed in to change notification settings - Fork 390
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
Fix CROSS_COMPILING guard and RUBYOPT=-v spec #1203
Conversation
The spec about `RUBYOPT=-v` fails since the point in time when the parser was switched to PRISM. But this wasn't noticed since the guard around it switched the two specs off, when executed from the ruby/ruby repository by `make test-spec`. This is because the constant `CROSS_COMPILING` is set by the `$(arch)-fake.rb` to `RUBY_PLATFORM` and mspec is executed with `-r $(arch)-fake.rb` on the command line of `make test-spec`. It is defined here: https://github.com/ruby/ruby/blob/98fce00cab460be81cb1f5e4cf8c0d66e006a35b/template/fake.rb.in#L40 This patch changes the guard to use `RbConfig` variable `CROSS_COMPILING` which is either "no" for a native build or "yes" when the ruby has been built per cross compiler. When the guard is fixed or when mspec is executed out of tree, the specs fail like so: https://github.com/oneclick/rubyinstaller2/actions/runs/11308579444/job/31451561785#step:32:78 To fix the failing specs the `PRISM` flag is removed from both sides before equality check. This is a leftover from the incomplete commit ruby@76c1fef
Would it work if we remove My impression is In this specific case, whether cross compiling or not Prism should be used by default anyway, isn't it? |
The options of RUBY_DESCRIPTION are passed from the calling ruby to the fake.rb file like so: options = remove_const(:RUBY_DESCRIPTION)[/( \+[^\[\]\+]+)*(?= \[\S+\]\z)/]
RUBY_DESCRIPTION = "ruby 3.4.0dev (2024-10-13T04:55:41Z master 98fce00cab)#{options} [x86_64-linux]".freeze ... so that it differs when cross compiling. But that case is excluded by the guard anyway. So IMHO a native build should match literally and should not need to exclude the "+PRISM" part. But maybe I miss something. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is correct. Unfortunately the Ruby test suite is fickle — it can be run with a 2x2 combination of prism/parse.y running in main process versus subprocess because it doesn't always pass on the options.
So it's possible to be:
parse.y
main process because of--parser=parse.y
butprism
subprocess because ofRB_DEFAULT_PARSER_PRISM
prism
main process because of--parser=prism
butparse.y
subprocess because ofRB_DEFAULT_PARSER_PRISM
parse.y
main process because of--parser=prism
or no options andparse.y
subprocess because ofRB_DEFAULT_PARSER_PRISM
prism
main process because of--parser=prism
or no options andprism
subprocess because ofRB_DEFAULT_PARSER_PRISM
mspec should ensure |
Thank you! |
The spec about
RUBYOPT=-v
fails since the point in time when the parser was switched to PRISM. But this wasn't noticed since the guard around it switched the two specs off, when executed from the ruby/ruby repository bymake test-spec
.This is because the constant
CROSS_COMPILING
is set by the$(arch)-fake.rb
toRUBY_PLATFORM
and mspec is executed with-r $(arch)-fake.rb
on the command line ofmake test-spec
. It is defined here: https://github.com/ruby/ruby/blob/98fce00cab460be81cb1f5e4cf8c0d66e006a35b/template/fake.rb.in#L40This patch changes the guard to use
RbConfig
variableCROSS_COMPILING
which is either "no" for a native build or "yes" when the ruby has been built per cross compiler.When the guard is fixed or when mspec is executed out of tree, the specs fail like so: https://github.com/oneclick/rubyinstaller2/actions/runs/11308579444/job/31451561785#step:32:78
To fix the failing specs the
PRISM
flag is removed from both sides before equality check. This is a leftover from the incomplete commit 76c1fef or 5320f88 .