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

Allow programmatic override of the default runtime options #3342

Merged
merged 5 commits into from
Oct 18, 2019

Conversation

dipinhora
Copy link
Contributor

Prior to this commit, the only way to override the default pony
runtime options was via the command line.

This commit allows for programmatic override of the default pony
runtime options via a specific bare function created on the Main
actor:

fun @runtime_override_defaults(rto: RuntimeOptions) => None

In order to override the values, a programmer would replace the
None with logic to directly set the various options on the rto
variable. This bare function is limited in what is allowed and
is only allowed to call functions on primitives to ensure no
memory allocation occurs as it is called before the runtime is
fully initialized.

This commit also renames the internal variable in options_t from
nopin to pin to better align with how it is set from the
command line so it is easier to programmatically change.


Example that sets --ponyhelp to true:

actor Main
  new create(env: Env) =>
    env.out.print("Hello, world.")

  fun @runtime_override_defaults(rto: RuntimeOptions) =>
    rto.ponyhelp = true

Prior to this commit, the only way to override the default pony
runtime options was via the command line.

This commit allows for programmatic override of the default pony
runtime options via a specific bare function created on the Main
actor:

`fun @runtime_override_defaults(rto: RuntimeOptions) => None`

In order to override the values, a programmer would replace the
`None` with logic to directly set the various options on the `rto`
variable. This bare function is limited in what is allowed and
is only allowed to call functions on `primitive`s to ensure no
memory allocation occurs as it is called before the runtime is
fully initialized.

This commit also renames the internal variable in `options_t` from
`nopin` to `pin` to better align with how it is set from the
command line so it is easier to programmatically change.
Copy link
Member

@jemc jemc left a comment

Choose a reason for hiding this comment

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

This is a clever solution.

Other than my small comment below, it looks good.

One other question, though - what's the best way to document that this feature exists? Should it maybe be mentioned in the pony help output where the runtime options are documented?

#include "../type/subtype.h"
#include "ponyassert.h"
#include <string.h>

static bool verify_calls(pass_opt_t* opt, ast_t* ast)
Copy link
Member

Choose a reason for hiding this comment

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

Could this be named something less generic than verify_calls? It is very specific to the runtime_override_defaults method, but that isn't clear at all from the name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@SeanTAllen
Copy link
Member

I feel like this should be documented somewhere. At a minimum, the runtime options Pony class should have documentation for what it does.

I'm open to suggestions on documentation.

@jemc
Copy link
Member

jemc commented Oct 15, 2019

Yeah, upon further thought I think for documentation it would be good to:

  • add a docstring to the RuntimeOptions type that describes what it does and gives a usage example code snippet
  • In the shell usage output where we describe the runtime options and their defaults, we should include a note that the defaults can be modified by the program, and refer the reader to look at the RuntimeOptions type for more details.

@dipinhora
Copy link
Contributor Author

docstrings and help text added.

This is a clever solution.

clever good or clever bad? i'm not sure......

@SeanTAllen
Copy link
Member

@dipinhora this is failing on Windows.

@dipinhora
Copy link
Contributor Author

@SeanTAllen Yes, it is. It's having troubling finding the new function during link time of libponyc.tests.exe.

Unfortunately, i know almost nothing about windows development or the tools available to debug the issue. Hopefully someone more knowledgeable can help or point me to a guide of some kind that might help me figure things out.

@chalcolith
Copy link
Member

I'll take a look tomorrow.

@jemc
Copy link
Member

jemc commented Oct 17, 2019

clever good or clever bad? i'm not sure......

😆

Sean uses "clever" as a criticism, but I use it as a compliment!

@chalcolith
Copy link
Member

On Windows everything is compiled as C++. Making the function declaration extern "C" should fix the Windows build.

--- a/src/libponyrt/sched/start.c
+++ b/src/libponyrt/sched/start.c
@@ -87,7 +87,13 @@ static opt_arg_t args[] =
  OPT_ARGS_FINISH
};

-void Main_runtime_override_defaults_oo(options_t* opt);
+#ifdef __cplusplus
+extern "C" {
+#endif
+  void Main_runtime_override_defaults_oo(void* opt);
+#ifdef __cplusplus
+}
+#endif

@dipinhora
Copy link
Contributor Author

@kulibali makes sense. thanks for the help.

@SeanTAllen
Copy link
Member

@dipinhora when this gets merged, can you add release notes to #3314?

@SeanTAllen
Copy link
Member

SeanTAllen commented Oct 18, 2019

Also, can you include an Added section addition for the CHANGELOG?

@SeanTAllen
Copy link
Member

When this passes CI, it is good to:

  • be squashed with just the original commit message kept
  • merged

@SeanTAllen SeanTAllen merged commit b6265be into ponylang:master Oct 18, 2019
@SeanTAllen
Copy link
Member

Thanks, @dipinhora.

Please add release notes to #3314.

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

Successfully merging this pull request may close these issues.

4 participants