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

[PRISM] allow enabling Prism via flag or env var #9115

Merged
merged 1 commit into from Dec 5, 2023

Conversation

HParker
Copy link
Contributor

@HParker HParker commented Dec 4, 2023

Enable Prism using either --prism

ruby --prism test.rb

or via env var

RUBY_PRISM=1 ruby test.rb

This uses Prism to load the first file and files loaded with require. This does not use Prism for eval, but that can be done in a followup.

@HParker HParker force-pushed the add-prism-flang-end-envvar branch 4 times, most recently from 47fd1eb to 2e763ce Compare December 4, 2023 20:43
Enable Prism using either --prism

    ruby --prism test.rb

or via env var

    RUBY_PRISM=1 ruby test.rb
Copy link
Contributor

@jemmaissroff jemmaissroff left a comment

Choose a reason for hiding this comment

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

Looks good, just one q about if we can reduce dependencies

@@ -13,6 +13,7 @@
#include "internal/gc.h"
#include "shape.h"
#include "vm_core.h"
#include "prism/prism.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way we can do this without adding this dependency?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think, as long as rb_iseq_new_main_prism takes a pm_string_t and pm_options_t, we need those types available.

Maybe we can hide the types behind a void * or a VALUE? Would that be better?

I looked into if we could move this to prism_compile.c instead of defining this in iseq.c, but prism_compile.c doesn't know about iseq_alloc.

Also, it looks like iseq.c already depends on prism/prism.h via prism_compile.h.

Is the idea in reducing dependencies to decouple things, or is the worry this will make compile times longer since changes in prism will require a recompile on all things that use iseq.h?

Copy link
Contributor

Choose a reason for hiding this comment

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

The smell is actually a bit of common.mk. Historically we've been trying to keep the dependencies to a minimum to make it easier to pull Prism out if the Ruby core team ever decides to do something like that. But I hink in this case, you're right, it's not really worth the void * casting.

@jemmaissroff jemmaissroff merged commit 9b76c7f into ruby:master Dec 5, 2023
98 checks passed
@HParker HParker deleted the add-prism-flang-end-envvar branch December 5, 2023 19:11
@mame
Copy link
Member

mame commented Dec 6, 2023

@jemmaissroff @HParker Hey, this is a user-facing feature. Please file a ticket on bugs.ruby-lang.org and get matz's approval to merge it. Please revert it for now. cc/ @kddnewton

@hsbt
Copy link
Member

hsbt commented Dec 6, 2023

Command line option and Environmental variables need to discuss naming and get approval from Matz.

@HParker
Copy link
Contributor Author

HParker commented Dec 6, 2023

Thank you @mame and @hsbt. I opened #9133 to revert this change so the conversation about the feature flag can happen. Please feel free to merge it.

I apologize for not following the correct process.

@mame
Copy link
Member

mame commented Dec 6, 2023

@HParker Thank you! Sorry for the hassle. I think we should discuss the following before merging it, for example.

  • What to do with the command-line option when prism becomes the default parser in the future.
    • Just remove it, or leave it as a "do nothing" option?
  • What are the appropriate names.
    • In Ruby, not absolutely but the names of features for professional use tend to be long and verbose, while those used by ordinary users tend to be short.
  • Whether to print a warning when this mode is used.

@hsbt
Copy link
Member

hsbt commented Dec 6, 2023

@HParker Thank you for your quick action ❤️

@HParker
Copy link
Contributor Author

HParker commented Dec 6, 2023

@mame and @hsbt,

Sorry for the hassle

No thank you both for the noticing and helping me learn the correct process! ❤️

I included mame's questions in the ticket. I think those are all really good points and many things I didn't consider enough.

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