-
-
Notifications
You must be signed in to change notification settings - Fork 925
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
[Truffle] New system for options. #4335
Conversation
@jtulach is this what we're supposed to be doing with the config in |
|
||
public class NewOptions { | ||
|
||
public final String[] ARGUMENTS; |
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.
Ideally also @CompilationFinal(dimensions = 1)
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.
Ah I didn't know about that - good idea.
@@ -35,6 +36,7 @@ public RubyEngine(RubyInstanceConfig instanceConfig) { | |||
|
|||
engine = PolyglotEngine.newBuilder() | |||
.config(RubyLanguage.MIME_TYPE, INSTANCE_CONFIG_KEY, instanceConfig) | |||
.config(RubyLanguage.MIME_TYPE, OptionsCatalogue.ARGUMENTS.getName(), instanceConfig.getArgv()) |
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.
Is this line needed?
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.
We're getting rid of the RubyInstanceConfig
on the line above when I convert all options over. So this is the only way we'll have to communicate arguments across. And it's a good thing because you'll then be able to set Ruby arguments for code that you invoke from other languages.
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.
How about passing them as a HashMap or so instead?
This could become quickly a long list, isn't it?
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.
Ah, but options can just not be passed and then use the defaults I guess, except from some key things like ARGV coming from non-system property or OptionDescriptor defaults.
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.
It's already a hash map - one that the polyglot engine maintains - these config
calls are like put
. And it's already private to our language.
There are only ten or so fields like this that we need to pass. We don't need to do this for options that RubyInstanceConfig
doesn't currently set.
Looks good to me 😃 👍 |
Looks good 👍 Only one suggestion, could the txt file be something structured? E.g.: a Ruby hash in a file using |
I thought the simple structure was a bonus? I'll write it as Ruby DSL or a Yaml or JSON file if people want. Chime in if you want that changed. |
But in unstructured txt you have to format manually and then parsed. Hash can be eval-ed (or yaml loaded) and it's done. |
I agree with Petr. If the idea is the text file isn't going to change that frequently, I'd rather use an established file format, even if it's more verbose. |
👍 for Yaml. |
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 like the look of this overall. I think we still need an argv processor of some sort though. RubyInstanceConfig
takes care of a lot internally that isn't represented here.
|
||
// This file would be automatically generated from the list of options in the text file. | ||
|
||
public class OptionsCatalogue { |
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 we settled on using the improved American spelling.
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.
Done.
@@ -296,7 +296,7 @@ | |||
|
|||
public static final Option<Integer> TRUFFLE_INSTRUMENTATION_SERVER_PORT = integer(TRUFFLE, "truffle.instrumentation_server_port", 0, "Port number to run an HTTP server on that provides instrumentation services"); | |||
public static final Option<Boolean> TRUFFLE_EXCEPTIONS_STORE_JAVA = bool(TRUFFLE, "truffle.exceptions.store_java", false, "Store the Java exception with the Ruby backtrace"); | |||
public static final Option<Boolean> TRUFFLE_EXCEPTIONS_PRINT_JAVA = bool(TRUFFLE, "truffle.exceptions.print_java", false, "Print Java exceptions at the point of translating them to Ruby exceptions."); | |||
//public static final Option<Boolean> TRUFFLE_EXCEPTIONS_PRINT_JAVA = bool(TRUFFLE, "truffle.exceptions.print_java", false, "Print Java exceptions at the point of translating them to Ruby exceptions."); |
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 take it this is being removed?
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.
Yes.
@@ -33,7 +33,7 @@ | |||
import static org.jruby.util.cli.Options.TRUFFLE_ENCODING_COMPATIBLE_QUERY_CACHE; | |||
import static org.jruby.util.cli.Options.TRUFFLE_ENCODING_LOADED_CLASSES_CACHE; | |||
import static org.jruby.util.cli.Options.TRUFFLE_EVAL_CACHE; | |||
import static org.jruby.util.cli.Options.TRUFFLE_EXCEPTIONS_PRINT_JAVA; | |||
//import static org.jruby.util.cli.Options.TRUFFLE_EXCEPTIONS_PRINT_JAVA; |
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 take it this is being removed?
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.
Yes.
@@ -159,7 +159,7 @@ | |||
|
|||
public final int INSTRUMENTATION_SERVER_PORT = TRUFFLE_INSTRUMENTATION_SERVER_PORT.load(); | |||
public final boolean EXCEPTIONS_STORE_JAVA = TRUFFLE_EXCEPTIONS_STORE_JAVA.load(); | |||
public final boolean EXCEPTIONS_PRINT_JAVA = TRUFFLE_EXCEPTIONS_PRINT_JAVA.load(); | |||
//public final boolean EXCEPTIONS_PRINT_JAVA = TRUFFLE_EXCEPTIONS_PRINT_JAVA.load(); |
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 take it this is being removed?
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.
Yes.
|
||
public void set(Properties properties) { | ||
for (Map.Entry<Object, Object> property : properties.entrySet()) { | ||
final String name = (String) property.getKey(); |
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.
Is there a case you're trying to handle here by not just calling toString
?
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.
Properties are just a very old API and don't use generics.
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 meant is there a reason to avoid property.getKey().toString()
?
if (description == null) { | ||
//throw new UnsupportedOperationException(name); | ||
|
||
// Don't throw for now - not all the options are transalted across |
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.
It might be beneficial to print out a warning (configurable via option!) for any option without a description.
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 will do when they're all in this system.
|
||
// Allows input such as [foo, "bar", 'baz']. Doesn't support escape sequences. | ||
|
||
private String[] parseStringArray(String string) { |
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.
Unless I've misunderstood the intent of this method, I think it could go away if we just used something like JSON or YAML for the config file format.
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 guess that wouldn't help the argv case, so maybe a value parser is still necessary. It just seems a bit complex.
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.
Indeed, what is this for? Do we have a use-case to pass a String[] as a String? A comma-separated list is much nicer to parse and to give on the command line: someOption=foo,bar,baz
.
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 switched to a simple comma separated format, with escaping for commas if needed. The reason for the string format is so you can set things like Ruby ARGV
using system properties. Imagine starting a JS node application and for some reason wanting Ruby code called using interop still have an ARGV
.
*/ | ||
package org.jruby.truffle.options; | ||
|
||
public class OptionTypeException extends UnsupportedOperationException { |
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 it'd be helpful for debugging if this exception type took the option name as its argument. Or perhaps we'd just enforce a message passed with the option name.
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.
Done.
*/ | ||
package org.jruby.truffle.options; | ||
|
||
public class UnknownOptionException extends UnsupportedOperationException { |
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.
As with OptionTypeException
, I think it'd help debugging a lot of if the option name were required.
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.
Done.
ed6e12e
to
06242f4
Compare
Can people review the generator. I actually have a problem with my ERB templates giving me extra blank lines. Does anyone know how to fix that? Other than that, raise any final objections now before I do all the work to switch the current options over and merge. |
|
||
private final Map<OptionDescription, Object> options = new HashMap<>(); | ||
|
||
public void set(Properties properties) { |
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 can't readily tell, but I suspect we're going to want a boundary on this.
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 don't think so - you can't set options after the engine is created, so it can't be fast path.
|
||
// Allows input such as foo,bar,baz. You can escape commas. | ||
|
||
private String[] parseStringArray(String string) { |
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 we're going to want a boundary on this.
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 don't think so - you can't set options after the engine is created, so it can't be fast path.
} | ||
|
||
} | ||
}).result) |
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.
File.write('truffle/src/main/java/org/jruby/truffle/options/NewOptions.java', ERB.new(<<-JAVA).result)
/*
* Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved. This
* code is released under a tri EPL/GPL/LGPL license. You can use it,
* redistribute it and/or modify it under the terms of the:
*
* Eclipse Public License version 1.0
* GNU General Public License version 2
* GNU Lesser General Public License version 2.1
*/
package org.jruby.truffle.options;
// This file is automatically generated by options.yml with 'jt build options'
import com.oracle.truffle.api.CompilerDirectives.CompilationFinal;
import javax.annotation.Generated;
@Generated("tool/truffle/generate-options.rb")
public class NewOptions {
<% options.each do |o| %>
<% if o.type.end_with?('[]') %>@CompilationFinal(dimensions=1) <% end %>public final <%= o.type %> <%= o.constant %>;
<% end %>
NewOptions(OptionsBuilder builder) {
<% options.each do |o| %>
<%= o.constant %> = builder.getOrDefault(OptionsCatalog.<%= o.constant %>);
<% end %>
}
}
JAVA
gives highlighting in idea
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.
Wow! Thanks! There's an example great feature, @eregon.
should help |
require 'yaml' | ||
require 'erb' | ||
|
||
options = YAML.load_file('truffle/src/main/java/org/jruby/truffle/options/options.yml') |
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.
Should this yml
file really be in the middle of Java files?
I think it makes more sense outside, like in tool/truffle
.
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.
Done.
private final String name; | ||
|
||
public OptionTypeException(String name) { | ||
this.name = name; |
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.
This should super()
and given a meaningful message.
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.
Done.
f5087f2
to
5d6b60f
Compare
2e2d741
to
352db68
Compare
The full PR looks great, thanks! |
We need to disconnect from the JRuby system because:
PolyglotEngine
has a configuration system and we should be going through that.PolyglotEngine
s in the same VM.I'm suggesting the system implemented below, with the extra phase that we will code generate some of the classes from the text file. This would be like the parser - we'd commit the generated code as it rarely changes and is small. The code generator would just be a Ruby template - you'd
jt build options
.I haven't written the code generator yet. The name
NewOptions
would just becomeOptions
. I didn't make that change to keep this diff small.All configuration which used to come from
RubyInstanceConfig
will now be an option. An example of where this is different to before is thatarguments
(ARGV
) would be an option. At the moment there's no way to set this inPolyglotEngine
- this new system makes it possible to set it both as aPolyglotEngine
configuration option and even in a system property.I'm proposing we use this one system for all options.
For the difference it makes in the key areas, see
RubyInstanceConfig
would instead become one of these lines)Opinions and votes for and against please @pitr-ch @bjfish @eregon @nirvdrum.
Don't merge.