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

introduce --parser runtime flag #9167

Merged
merged 3 commits into from Dec 15, 2023
Merged

Conversation

HParker
Copy link
Contributor

@HParker HParker commented Dec 8, 2023

Introduce --parser runtime flag

ruby --parser=prism

also update the description:

$ ruby --parser=prism --version
ruby 3.3.0dev (2023-12-08T04:47:14Z add-parser-runtime.. 0616384c9f) +PRISM [x86_64-darwin23]

Bug #20044

@HParker HParker force-pushed the add-parser-runtime-flag branch 2 times, most recently from ee9457d to 4d8147a Compare December 8, 2023 05:18
@HParker HParker marked this pull request as ready for review December 8, 2023 06:26
ractor_core.h Outdated
@@ -186,6 +186,7 @@ struct rb_ractor_struct {
VALUE r_stderr;
VALUE verbose;
VALUE debug;
bool prism;
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe it should be global, not ractor local.

@HParker HParker force-pushed the add-parser-runtime-flag branch 3 times, most recently from 4db67a4 to e12345c Compare December 13, 2023 17:46
@HParker
Copy link
Contributor Author

HParker commented Dec 13, 2023

I have been thinking about this change and if it should go in before or after 3.3.0.

I want to have it in 3.3.0, but I worry that it might make a bad first impression for Prism. I worry that someone will turn on prism via this option and be disappointed and decide Prism is not good when they run into issues. If we get this in shortly after the 3.3.0 release and have all the time between then and 3.3.1 to test, we might be able to make a better first impression.

I am open to whatever the team thinks is best, but right now I am nervous about including it in 3.3.0.

@jeremyevans
Copy link
Contributor

My opinion is that we should not add features to tiny releases, only to minor ones. So if --parser=prism is not ready for 3.3.0, it should wait until 3.4.0.

@HParker
Copy link
Contributor Author

HParker commented Dec 13, 2023

In that case, I think we should probably add this in 3.3.0. My hope is that there is a way to use prism optionally as the parser before 3.4.0. I would like to at least be testing with prism in CI regularly in 2024.

@kddnewton
Copy link
Contributor

My opinion is that we should not add features to tiny releases, only to minor ones. So if --parser=prism is not ready for 3.3.0, it should wait until 3.4.0.

I agree, I don't think we should add features in tiny releases. Given that we now compile all nodes and are just fixing bugs, I think we need to get this merged for 3.3.0 so that we're not stuck waiting for it for another year. The function of this flag is not for people to run it in production, the function is for us to gather feedback about what may or may not be breaking with people's code that we don't have access to. We need this flag for testing and I wouldn't want to add it in a tiny release.

@kddnewton
Copy link
Contributor

@ko1 can you rereview? It looks like @HParker addressed your concern. I just want to make sure you are happy with this before I merge it. Given timezones, I'll wait for your reply tomorrow.

@kddnewton kddnewton closed this Dec 14, 2023
@kddnewton kddnewton reopened this Dec 14, 2023
@kddnewton
Copy link
Contributor

Sorry I just clicked the wrong button....

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.

Looks great!

ruby.c Outdated
else if (is_option_with_arg("parser", Qfalse, Qtrue)) {
if (strcmp("prism", s) == 0) {
(*rb_ruby_prism_ptr()) = true;
rb_warn("The Prism compiler is currently experimental and compatibility with parse.y is not yet complete. Please report an issues you find on the prism issue tracker.");
Copy link
Member

Choose a reason for hiding this comment

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

an issues, should be without an

Copy link
Contributor

Choose a reason for hiding this comment

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

I assume that's meant to be any

Copy link
Member

Choose a reason for hiding this comment

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

Yeah any issue would also work

Introduce runtime flag for specifying the parser,

```
ruby --parser=prism
```

also update the description:

```
$ ruby --parser=prism --version
ruby 3.3.0dev (2023-12-08T04:47:14Z add-parser-runtime.. 0616384) +PRISM [x86_64-darwin23]
```

[Bug #20044]
ruby.c Outdated Show resolved Hide resolved
Co-authored-by: Ufuk Kayserilioglu <ufuk@paralaus.com>
@@ -287,6 +287,19 @@ def test_rjit_version
end
end

def test_parser_flag
warning = /The Prism compiler is currently experimental and compatibility with parse.y is not yet complete. Please report an issues you find on the prism issue tracker./
Copy link
Contributor

Choose a reason for hiding this comment

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

This message needs to be updated as well.

@kddnewton kddnewton enabled auto-merge (rebase) December 15, 2023 18:19
@kddnewton kddnewton merged commit b418e5a into ruby:master Dec 15, 2023
96 of 97 checks passed
@ko1
Copy link
Contributor

ko1 commented Dec 20, 2023

@ko1 can you rereview? It looks like @HParker addressed your concern. I just want to make sure you are happy with this before I merge it. Given timezones, I'll wait for your reply tomorrow.

Sorry I missed the comment.
It's okay but rb_ruby_ can be located on ruby.c (natural for me).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
6 participants