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

Fix CROSS_COMPILING guard and RUBYOPT=-v spec #1203

Merged
merged 1 commit into from
Oct 14, 2024
Merged

Conversation

larskanis
Copy link
Contributor

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 76c1fef or 5320f88 .

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
@eregon
Copy link
Member

eregon commented Oct 13, 2024

Would it work if we remove +PRISM on neither side?

My impression is RUBY_DESCRIPTION is incorrectly set by CRuby (maybe in that fake.rb file?) when running specs from the build-but-not-installed files.

In this specific case, whether cross compiling or not Prism should be used by default anyway, isn't it?

@eregon eregon requested a review from kddnewton October 13, 2024 15:45
@larskanis
Copy link
Contributor Author

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.

Copy link
Contributor

@kddnewton kddnewton left a 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 but prism subprocess because of RB_DEFAULT_PARSER_PRISM
  • prism main process because of --parser=prism but parse.y subprocess because of RB_DEFAULT_PARSER_PRISM
  • parse.y main process because of --parser=prism or no options and parse.y subprocess because of RB_DEFAULT_PARSER_PRISM
  • prism main process because of --parser=prism or no options and prism subprocess because of RB_DEFAULT_PARSER_PRISM

@eregon
Copy link
Member

eregon commented Oct 14, 2024

mspec should ensure --parser=parse.y is correctly passed to all subprocesses, and RB_DEFAULT_PARSER_PRISM should be the same for all subprocesses due to being in ENV (although some specs clear ENV to test that itself so that doesn't hold there), so in theory it should work except these unsetenv_others: true specs.
Anyway, this change is fine, let's merge it.

@eregon eregon merged commit 41c5991 into ruby:master Oct 14, 2024
14 checks passed
@larskanis larskanis deleted the patch-1 branch October 14, 2024 18:47
@larskanis
Copy link
Contributor Author

Thank you!

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.

3 participants