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

Add support for extension modules #252

Merged
merged 7 commits into from Jun 4, 2018

Conversation

3 participants
@smarr
Copy link
Owner

commented May 23, 2018

The main goal of this PR is to allow us to load primitives implemented in Java dynamically at run time.
This is going to be useful to provide support for HTTP, databases, and other things, and avoids to include them into the SOMns codebase directly.

An extension module is loaded the same way as other modules:

module:: system loadModule: 'extension.jar' nextTo: self.

The result of such a load is a class, as for other modules.
It can be instantiated with new, and then provides the primitives as normal Smalltalk messages:

m:: module new
db:: m openDatabase: 'file.db'.
db close.

(This is just a possible example. Database support is not included in this PR.)

Documentation is added in docs/extensions.md.

The PR includes a few other minor cleanups, somewhat related to this work.

@smarr smarr added the enhancement label May 23, 2018

@smarr smarr added this to the v0.7.0 milestone May 23, 2018

@smarr smarr added this to Open Issues in Completeness via automation May 23, 2018


private system = platform system.

private jarName = 'extension/build/test-extension.jar'.

This comment has been minimized.

Copy link
@richard-roberts

richard-roberts May 23, 2018

Contributor

Will the extensions generally be in the same directory? If so, we could further hide the fact that these are written in Java and instead write the name as just test-extension. With this approach, it would be simple to swap out the Java module for a Newspeak module later on (without changing any code that uses the replaced module).

This comment has been minimized.

Copy link
@smarr

smarr May 24, 2018

Author Owner

Same directory is not necessary.
Why do we want to hide the fact that they are written in Java?
That would be a departure from what we currently do: https://github.com/smarr/SOMns/blob/release/core-lib/Platform.ns#L6
vmMirror load: 'System.ns' nextTo: self.

Not sure this is useful, and not sure it is desirable.
I would avoid unnecessary magic, I think.

This comment has been minimized.

Copy link
@richard-roberts

richard-roberts May 24, 2018

Contributor

Why do we want to hide the fact that they are written in Java?

It seemed like a good idea when I suggested it, but now that I'm trying to write something down I can't come up with an answer. Probably best we ignore this in that case :)

In Newspeak, the first argument, i.e., the receiver is always present,
but in this case it is simply ignored by the `inc(.)` method.

Note further the use of `@GenerateNodeFactory`, which is ensures that factory

This comment has been minimized.

Copy link
@richard-roberts

richard-roberts May 23, 2018

Contributor

which is ensures that a factory class is generated and that we will use it to create

```

With the primitive nodes, the `Extension` class, and the `somns.extension` file,
we have everything we need in the JAR file.

This comment has been minimized.

Copy link
@richard-roberts

richard-roberts May 23, 2018

Contributor

Is it possible, without much change, to allow a jar to provide multiple extension classes? Not sure this would ever be helpful but might be worth thinking about.

This comment has been minimized.

Copy link
@smarr

smarr May 24, 2018

Author Owner

I would not want to go there at this point.
I would stick with the one file == one module as we have it for Newspeak modules. Minimal differences between the two is desirable, I think.

return SNodeFactory.createMessageSend(superClass,
new ExpressionNode[] {selfRead}, false, source, null, language);
}

This comment has been minimized.

Copy link
@richard-roberts

richard-roberts May 23, 2018

Contributor

Why has this been added / changed?

This comment has been minimized.

Copy link
@smarr

smarr May 24, 2018

Author Owner

It's a refactor, extracting common code.

SSymbol signature = Symbols.symbolFor(name);
assert !primitives.containsKey(signature) : "clash of vmMirrorPrimitive names";
primitives.put(signature,
constructPrimitive(signature, specializer, lang));

This comment has been minimized.

Copy link
@richard-roberts

richard-roberts May 23, 2018

Contributor

Should we prefix primitives by their module names to avoid potential clashes?

This comment has been minimized.

Copy link
@smarr

smarr May 24, 2018

Author Owner

There are no clashes possible. They are entirely isolated from each other.

This comment has been minimized.

Copy link
@daumayr

daumayr May 24, 2018

Contributor

If we are correct, there can be clash only between primitive signatures within a plugin, not between the primitive signature in 1 plugin with the signature in vmMirror or in another plugin.

So why the error msg "assert !primitives.containsKey(signature) : "clash of vmMirrorPrimitive names";"?

A different message could be better, for example ("clash of primitive named " + name + " in " + moduleName).

This looks important when writing the module to know what is problematic (typically when copy pasting one primitive, modifying the copied version to new code and forgetting to change the primitive signature).

@daumayr
Copy link
Contributor

left a comment

How much access do the Java implementations of extensions have to the SOMns implementation?
For example when tracing external Data, can I just import and use ActorExecutionTrace, or do i have to pass a reference to the extension somehow?

Is there a dependency system planned for the plugins? Assuming the plugin java code can import anything from the SOMns core codebase, is it possible for a plugin to also import classes from another plugin?

@@ -606,24 +606,24 @@ DAMAGE.>> *)

public readStream ^ <ExternalReadStream> = (
^ExternalReadStream onDescriptor: (
self open: #read ifFail: [ :err <Exception> |
self error: err

This comment has been minimized.

Copy link
@daumayr

daumayr May 24, 2018

Contributor

In our PR, you mentioned "unnecessary change" for such changes. There are a lot of those in this PR. Is this because the formatter changed or something like that?

This comment has been minimized.

Copy link
@smarr

smarr May 24, 2018

Author Owner

Yeah, I blame this change on you. You should set your editor to trim unnecessary whitespace at the end of lines ;)

# Extensions

SOMns aims to be a general purpose language that supports a wide range of use
cases. Some applications might needs to interact with the underlying system.

This comment has been minimized.

Copy link
@daumayr

daumayr May 24, 2018

Contributor

Some applications might needs => Singular or plural?

SSymbol signature = Symbols.symbolFor(name);
assert !primitives.containsKey(signature) : "clash of vmMirrorPrimitive names";
primitives.put(signature,
constructPrimitive(signature, specializer, lang));

This comment has been minimized.

Copy link
@daumayr

daumayr May 24, 2018

Contributor

If we are correct, there can be clash only between primitive signatures within a plugin, not between the primitive signature in 1 plugin with the signature in vmMirror or in another plugin.

So why the error msg "assert !primitives.containsKey(signature) : "clash of vmMirrorPrimitive names";"?

A different message could be better, for example ("clash of primitive named " + name + " in " + moduleName).

This looks important when writing the module to know what is problematic (typically when copy pasting one primitive, modifying the copied version to new code and forgetting to change the primitive signature).

@smarr

This comment has been minimized.

Copy link
Owner Author

commented May 24, 2018

How much access do the Java implementations of extensions have to the SOMns implementation?
For example when tracing external Data, can I just import and use ActorExecutionTrace, or do i have to pass a reference to the extension somehow?

Extensions are currently loaded into their own class loader. Thus, there isn't really much interaction between the interpreter and the extension at this point.
The normal class loader is the parent, so, the extension might see the interpreter state.
I am not entirely sure how the isolation between class loaders works.
So, that probably should be tested.
If we need to make things accessible, this could probably done via the Extension interface.
So, we kind of need to know what's needed.

Is there a dependency system planned for the plugins? Assuming the plugin java code can import anything from the SOMns core codebase, is it possible for a plugin to also import classes from another plugin?

Importing? Well, class loading logic applies.
If a class is on the classpath, you get access to it.
If it doesn't, like it is the case for other extension, you don't. (Also, please do not use a new term. I don't know what a plugin is. I don't think I use "plugin" anywhere.)
I am not sure why we would want the complexity of making extension modules aware of other modules beside by using standard mechanisms (passing in references as needed).
You got a specific use case in mind?

@smarr smarr force-pushed the extension-modules branch 2 times, most recently from 7e872e7 to 42ef3dc May 27, 2018

@smarr smarr changed the title Add support for extension modules [WIP] Add support for extension modules May 27, 2018

@smarr

This comment has been minimized.

Copy link
Owner Author

commented May 27, 2018

I did some more work on this.

@daumayr since you requested changes, could you please confirm all issues are solved, and request new changes or approve the PR?

@smarr smarr force-pushed the extension-modules branch from 42ef3dc to f933d60 May 28, 2018

@daumayr

This comment has been minimized.

Copy link
Contributor

commented May 29, 2018

@smarr I think it would be good to define how library dependencies of the extensions should be handled.
For example for a database that needs the Derby jar, should the extensions provide a setup script?
I don't see any other issues.

@smarr

This comment has been minimized.

Copy link
Owner Author

commented May 29, 2018

I think it would be good to define how library dependencies of the extensions should be handled.

Hm, you mean in terms of other class path dependencies only? Perhaps this could work: https://docs.oracle.com/javase/tutorial/deployment/jar/downman.html

should the extensions provide a setup script?

What would the setup script do?

@daumayr

This comment has been minimized.

Copy link
Contributor

commented May 29, 2018

  1. Yes, this looks like a solution. Do we want a uniform directory structure for jar dependencies of extensions?

  2. The script would for example download derby from the Appache server and put in the right location.

@smarr

This comment has been minimized.

Copy link
Owner Author

commented May 29, 2018

Yes, this looks like a solution. Do we want a uniform directory structure for jar dependencies of extensions?

Probably, to ease maintenance. Is that something that needs to be enforced somehow?

The script would for example download derby from the Appache server and put in the right location.

Isn't that something the build script needs to do anyway? I am not sure why you need a separate 'script' for that.

@daumayr

This comment has been minimized.

Copy link
Contributor

commented May 29, 2018

Probably, to ease maintenance. Is that something that needs to be enforced somehow?

Maybe put a best practice/guideline/convention in the doc, enforcement doesn't seem necessary right now.

Isn't that something the build script needs to do anyway? I am not sure why you need a separate 'script' for that.

Right

smarr added some commits May 13, 2018

Make StreamTests independent from current directory
- fix style issues, too

Signed-off-by: Stefan Marr <git@stefan-marr.de>
Fix style
Signed-off-by: Stefan Marr <git@stefan-marr.de>
Avoid hardcoding test suites
Use the file API to actually read the *Tests.ns files from the directory.

Signed-off-by: Stefan Marr <git@stefan-marr.de>

@smarr smarr force-pushed the extension-modules branch from f933d60 to b24f132 Jun 3, 2018

@smarr

This comment has been minimized.

Copy link
Owner Author

commented Jun 3, 2018

@daumayr could you please take another look? (your review still indicates that changes are requested)
I added some more documentation, and a test for using the Class-Path attribute of a JAR.
If that's all that we need at the moment, I'd merge this.

smarr added some commits May 23, 2018

Improve error message in Class>>#new
Signed-off-by: Stefan Marr <git@stefan-marr.de>
Add support for extension modules
Extension modules are jar files, and can be loaded like normal modules.
This means, we get back a class, which can be instantiated.
The methods on the class are defined by the @primitive annotations on node classes.
These primitives have to be made available via the list of factories to the primitive loader.

- added basic tests
- added test extension
- added extension primitive loader
- some refactoring of mixin builder and parser
- added Eclipse and Ant setup
- added documentation

Added extension test Eclipse and Ant setup

Signed-off-by: Stefan Marr <git@stefan-marr.de>
Test and document JAR classpath dependencies
Signed-off-by: Stefan Marr <git@stefan-marr.de>
Update test data
Signed-off-by: Stefan Marr <git@stefan-marr.de>

@smarr smarr force-pushed the extension-modules branch from 28e455f to b6f8958 Jun 3, 2018

@daumayr

daumayr approved these changes Jun 4, 2018

Copy link
Contributor

left a comment

I don't see any issues

@smarr smarr merged commit 71ab0a3 into dev Jun 4, 2018

4 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.2%) to 77.775%
Details

Completeness automation moved this from Open Issues to Completed Jun 4, 2018

@smarr smarr deleted the extension-modules branch Jun 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.