Add package.json js-plugins support #4389

devinrsmith · 2023-08-28T21:17:46Z

This allows the additional configuration of js-plugins via the configuration properties deephaven.jsPlugins.<x>=<packageRoot>, with the expectation that <packageRoot>/package.json exists. This should make some use cases easier, as it's now possible to configure js-plugins without the need to create a manifest.json.

This re-introduces some code that was originally part of #2908; namely, an on-disk zip filesystem is used as the installable state for js-plugins.

In support of #4405

devinrsmith · 2023-08-29T23:05:01Z

Here's how I tested:

FROM node:lts-slim as build
ARG PACKAGE 
RUN set -eux; \
        npm pack ${PACKAGE}; \
        tar --touch --extract --file *.tgz

FROM scratch
COPY --link --from=build /package /

docker build --build-arg PACKAGE=@deephaven/js-plugin-table-example --output type=local,dest=/tmp/example .
docker build --build-arg PACKAGE=@deephaven/js-plugin-plotly --output type=local,dest=/tmp/plotly .
docker build --build-arg PACKAGE=@deephaven/js-plugin-matplotlib --output type=local,dest=/tmp/matplotlib .

and then started the server up a variety of different configurations:

START_OPTS="-Ddeephaven.jsPlugins.foo=/tmp/example -Ddeephaven.jsPlugins.bar=/tmp/plotly -Ddeephaven.jsPlugins.baz=/tmp/matplotlib"

and then also mixed it in with the existing way of configuring plugins via deephaven.jsPlugins.resourceBase and <configDir>/js-plugins/

devinrsmith · 2023-09-01T22:44:26Z

The latest commit re-introduces io.deephaven.plugin.js.JsPlugin; this does make the PR larger, but ultimately makes the structure cleaner with respect to how the system provides js plugins from Configuration. Note: there is no python-side plumbing, but there doesn't need to be at this time. This can be committed standalone, with python-side support coming after.

chipkent

Approved the python. I did not review the rest.

niloc132 · 2023-09-01T21:36:55Z

server/jetty/src/main/java/io/deephaven/server/jetty/JettyBackedGrpcServer.java

@@ -170,6 +171,16 @@ public JettyBackedGrpcServer(
        // Wire up the provided grpc filter
        context.addFilter(new FilterHolder(filter), "/*", EnumSet.noneOf(DispatcherType.class));

+        final JsPluginsZipFilesystem fs;
+        try {
+            fs = JsPlugins.initJsPlugins(Configuration.getInstance());


this pattern (making the current class obtain the config instance rather than letting initJsPlugins call it) makes it look like you anticipate multiple instances of JsPlugins to be built from different configs, but I think by its nature we will only have one JsPlugins instance at a time (or at least per-server), since there can only be one absolute path called /js-plugins/manifest.json?

It seems to push us towards a "better" pattern of injecting configuration, but our existing Configuration type inherently is global, and it requires a bit of boilerplate to achieve this, with no gain that i can see?

In the latest version, I actually am injecting Configuration.

Right, I'm suggesting not bothering with that - more source churn, minimal payoff (since Configuration isn't mockable, can only have One True Instance, etc), and just referencing Configuration.getInstance() once where it is read

Okay, will undo the Configuration injection.

server/jetty/src/main/java/io/deephaven/server/jetty/JsPlugins.java

niloc132 · 2023-09-02T16:49:41Z

plugin/src/main/java/io/deephaven/plugin/js/JsPlugin.java

+import io.deephaven.plugin.Plugin;
+
+/**
+ * A js plugin.


This doesn't really add much (except as a "there is javadoc" checkbox") - the linked items below are a little dry too, leading one to take apart the code or read the PR's comments rather than gain understanding of it directly. Can you beef these up, at least from the perspective of 'this is an interface that deliberately hides the implementation from you, here's how to use it, what to do with it, etc'?

Let me know what you think about the latest. Kind of punting some specifics to the deephaven-plugins repo.

niloc132 · 2023-09-11T14:14:07Z

server/jetty/src/main/java/io/deephaven/server/jetty/JsPluginsZipFilesystem.java

+    }
+
+    private void init() throws IOException {
+        try (final FileSystem fs = FileSystems.newFileSystem(filesystem, Map.of("create", "true"))) {


i think this method should be synchronized too, since it is going to modify the zip file in place. It isn't technically necessary, but it seems odd to me to let it be possible to call it again.

that said, why create a new filesystem each time? filesystems are threadsafe, or is there an issue with needing to explicitly close so jetty can read it?

Will sync the init.

I think the zip filesystem does need to be closed to actually write out the contents. The reason being is that the whole .zip file is replaced with a new one; and you can imagine that if you are writing multiple files, anything else is very inefficient:

try (final FileSystem zipfs = FileSystems.newFileSystem(...)) { for (int i = 0; i < 100; ++i) { Files.writeString(fs.getPath("f" + i + ".txt"), "some content"); } }

That said, I'm going to dig in more to verify and will add comment in code.

Yep: verified FileSystem#close is necessary.

niloc132 · 2023-09-11T15:18:10Z

server/jetty/src/main/java/io/deephaven/server/jetty/JsPluginsZipFilesystem.java

+                    .distributionPath(srcEntry.main());
+            CopyHelper.copyRecursive(srcDist, destDist);
+            entries.add(srcEntry);
+            writeManifest(fs);


If I understand this correctly, each copyFrom call re-opens the zip file, appends new files to it, then appends a new ~~manifest~~ central directory entry to it, and writes a new toc at the end of the file, and closes it.

The reason for doing this is that we then pass the path to this zip directly to jetty, so it can open it to use as its resource base, and serve files from it. Jetty prefers to not serve from zips, but we disable that to avoid unpacking to /tmp and risk something cleaning out that directory. So we're finding already-unpacked contents on disk so we can pack it and tell jetty to server it still-packed...

Tradeoffs:

the contents are immutable (changes won't break the server), but the contents are immutable (harder to develop live)

extra copy on disk (deleted on exit, assuming non-crash exit)

contents served from zip (shouldn't be a big deal, assuming good caching, small files)

contents ostensibly filtered to only what is expected to be served

We don't support reading zips/tarballs (i.e. jars, wheels, npm packages), and so always already have an unpacked structure on disk somewhere, and we require (i think) that the unpacked structure matches what we will zip (though we only copy/serve a subset of what was in that directory). Why not point to those existing unpacked dirs, with some filtering for "you only need to see these files"?

Jetty prefers to not serve from zips, but we disable that

Is this documented somewhere? I don't think we need to disable anything to make jetty work with zips...

Why not point to those existing unpacked dirs, with some filtering for "you only need to see these files"?

This might be possible, but it's somewhat dependent on being able to configure jetty later in our startup lifecycle (or, get the plugin information earlier in our lifecycle) than we currently are.

Right now, the zip represents the single-point of entry that we can later add to via plugins:

// Wire up /js-plugins/* context.addServlet(zipFsServlet, "/js-plugins/*");

If we were to allow plugins to add to them, it could look something like:

WebAppContext context = new WebAppContext(null, "/js-plugins/<pluginName>", null, null, null, new ErrorPageErrorHandler(), NO_SESSIONS); context.setBaseResource(createResource(<pluginPath>)); addHandler.accept(context);

And then we'd need to create an immutable manifest.json resource as well.

I think it would be a tricky change to the order of when things are created during startup, ala io.deephaven.server.runner.DeephavenApiServer#run.

Of course, there may be ways we could use a Servlet and add later w/ with uncopied plugin Resources, but I don't really know the specific jetty machinations to see if that would be possible.

niloc132 · 2023-09-11T15:21:02Z

plugin/src/main/java/io/deephaven/plugin/js/JsPluginPackagePath.java

+     * @return the distribution path
+     */
+    public final Path distributionPath(String main) {
+        return path().resolve(main).getParent();


I'm not at all sure that this is correct, except for projects that follow the convention that you're used to. That is, two cases this doesn't do what I think you want (but docs are light, so I can't be sure):

nested directories, like dist/js/index.js (unsure how common this is) - the other files in the dist/ output dir will be missed this way (for example in this contrived case, a dist/css/main.css could exist)

no dist dir, like mylibrary.min.js (this is decently common, where the .min.js file is generated, and just lives adjacent to the mylibrary.js file - now the whole dir, including package.json, possibly .git/ dir will be copied/served

Okay; I think we can copy the full contents over as is, and remove this assumption.

…-js-plugin

chipkent

Approved python. Have not reviewed the rest.

server/jetty/src/main/java/io/deephaven/server/jetty/JsPlugins.java

server/src/main/java/io/deephaven/server/runner/DeephavenApiServerComponent.java

server/src/main/java/io/deephaven/server/plugin/PluginsModule.java

server/src/main/java/io/deephaven/server/plugin/js/JsPluginModule.java

server/src/test/java/io/deephaven/server/runner/DeephavenApiServerTestBase.java

niloc132 · 2023-10-04T21:11:47Z

server/src/main/java/io/deephaven/server/plugin/js/JsPluginModule.java

-                .stream()
-                .map(Registration.class::cast)
-                .collect(Collectors.toSet());
+        return Set.copyOf(jsPluginsPackageRoots());


why not apply to the other unnecessary streams above too?

The above aren't streams, but Optionals that turn into Set.of(Element) or Set.of().

providesConfigDirRegistration and providesResourceBaseRegistration, the two methods above this?

Correct. @IntoSet might be more appropriate if there is a way to ignore null or optional return values, but I don't suspect that would work...

Ah got it, thanks for clarifying.

deephaven-internal · 2023-10-05T15:24:38Z

Labels indicate documentation is required. Issues for documentation have been opened:

How-to: https://github.com/deephaven/deephaven.io/issues/3272
Reference: https://github.com/deephaven/deephaven.io/issues/3271

Add multi-homed, package.json JS plugin support

29c18bb

devinrsmith self-assigned this Aug 28, 2023

devinrsmith added 2 commits August 28, 2023 16:54

Fix

8d1d20f

Consolidate paths

d5e3631

devinrsmith added DocumentationNeeded ReleaseNotesNeeded Release notes are needed labels Aug 29, 2023

Do an move when copying plugins

a6d70d7

devinrsmith changed the title ~~Add multi-homed, package.json JS plugin support~~ Add package.json JS plugin support Aug 29, 2023

devinrsmith changed the title ~~Add package.json JS plugin support~~ Add package.json js-plugins support Aug 29, 2023

devinrsmith requested review from jnumainville and mofojed August 29, 2023 23:05

devinrsmith added the feature request New feature or request label Aug 29, 2023

devinrsmith marked this pull request as ready for review August 29, 2023 23:05

devinrsmith requested a review from niloc132 August 29, 2023 23:07

devinrsmith added this to the September 2023 milestone Aug 30, 2023

JsPlugin

b2d7503

devinrsmith requested review from chipkent, jmao-denver and rcaudy as code owners September 1, 2023 22:42

Fix test compile

eaac8d9

chipkent previously approved these changes Sep 6, 2023

View reviewed changes

niloc132 reviewed Sep 11, 2023

View reviewed changes

devinrsmith added 2 commits September 11, 2023 08:51

Merge remote-tracking branch 'upstream/main' into multi-homed-package…

a4cc53c

…-js-plugin

Review points

cd9b816

devinrsmith dismissed chipkent’s stale review via cd9b816 September 13, 2023 20:29

devinrsmith requested a review from niloc132 September 13, 2023 20:29

chipkent previously approved these changes Sep 26, 2023

View reviewed changes

jnumainville previously approved these changes Sep 28, 2023

View reviewed changes

niloc132 reviewed Oct 2, 2023

View reviewed changes

More review points

3a51bc8

devinrsmith dismissed stale reviews from jnumainville and chipkent via 3a51bc8 October 4, 2023 19:45

devinrsmith requested a review from niloc132 October 4, 2023 20:15

niloc132 reviewed Oct 4, 2023

View reviewed changes

devinrsmith requested a review from niloc132 October 4, 2023 21:46

niloc132 approved these changes Oct 5, 2023

View reviewed changes

devinrsmith merged commit 87ee5ec into deephaven:main Oct 5, 2023
10 checks passed

devinrsmith deleted the multi-homed-package-js-plugin branch October 5, 2023 15:24

github-actions bot locked and limited conversation to collaborators Oct 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add package.json js-plugins support #4389

Add package.json js-plugins support #4389

devinrsmith commented Aug 28, 2023 •

edited

Loading

devinrsmith commented Aug 29, 2023

devinrsmith commented Sep 1, 2023

chipkent left a comment

niloc132 Sep 1, 2023

devinrsmith Sep 11, 2023

niloc132 Sep 11, 2023

devinrsmith Sep 13, 2023

niloc132 Sep 2, 2023

devinrsmith Sep 13, 2023

niloc132 Sep 11, 2023

devinrsmith Sep 13, 2023

devinrsmith Sep 13, 2023

niloc132 Sep 11, 2023

devinrsmith Sep 13, 2023

devinrsmith Sep 13, 2023

niloc132 Sep 11, 2023

devinrsmith Sep 13, 2023

chipkent left a comment

niloc132 Oct 4, 2023

devinrsmith Oct 4, 2023

niloc132 Oct 5, 2023

devinrsmith Oct 5, 2023

niloc132 Oct 5, 2023

deephaven-internal commented Oct 5, 2023

Add package.json js-plugins support #4389

Add package.json js-plugins support #4389

Conversation

devinrsmith commented Aug 28, 2023 • edited Loading

devinrsmith commented Aug 29, 2023

devinrsmith commented Sep 1, 2023

chipkent left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chipkent left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deephaven-internal commented Oct 5, 2023

devinrsmith commented Aug 28, 2023 •

edited

Loading