Simplify JsThemis packaging and test launch #618

ilammy · 2020-04-03T15:09:20Z

This patch set unifies JsThemis with WasmThemis a bit.

package.json files are updated with latest versions and more complete metadata
jsthemis_install, jsthemis_uninstall targets are simplified
test_js target no longer requires JsThemis to be installed and prepared

See details in commit messages.

The main goal of these changes is to make it easier to maintain JsThemis. With these changes the developers can iterate on JsThemis by running npm test directly from src/wrappers/themis/jsthemis or make test_js from the source root. It is no longer necessary to not forget to run make jsthemis_install after each change in JsThemis code.

Additionally, there is a new makefile target make jsthemis which can be used to build a package tarball for local usage or publishing.

Checklist

Change is covered by automated tests
The coding guidelines are followed
Public API has proper documentation
Changelog is updated

Add package-lock.json file into the repo. It is recommended to keep this file committed into VCS. GitHub analyzer will scan it to see if there are known security vulnerabilities in our dependencies. Update the package.json file, add some missing metadata fields that we fill in for WasmThemis. Also, add modern "mocha" as dev dependency. Add an .npmignore file to prevent internal files from being packed into the resulting distribution package.

First of all, move them into the dedicated makefile from the main one. Introduce a proper file target that will build the package in the build directory (not simply "./build"). This is more friendly to systems where the source directory is mounted as read-only, with a separate writable build directory. Unfortunately, "npm pack" always spits out its results into the current directory. We have to cd into the build directory before calling it. Also, since we don't really need to have multiple versions in the build directory, we can rename the result into just "jsthemis.tgz" and avoid doing all that awk magic to extract the version. Note the use of "abspath". We need to ensure that we refer to correct files, and for "npm install" to install the archive without treating "build/jsthemis.tgz" as a scoped package name.

Add an npm target "test" to run "mocha" for our tests. This makes is possible to develop and test JsThemis directly from its source directory (like WasmThemis) and also packages the tests so that can be used by other developers to verify our package after installation. These changes make it possible to remove the preparation step from unit test runs of JsThemis. We can now simply do "make test_js", without installing it into the source root and running "make prepare_tests_all".

Lagovas · 2020-04-06T08:45:50Z

.circleci/config.yml

            # Run JsThemis tests
+            rm -rf src/wrappers/themis/jsthemis/node_modules


can we call something like make jsthemis_uninstall?

can we call something like make jsthemis_uninstall?

My goal was to avoid doing that specifically, but it does not seem to be working. So my current plan is to revert the last commit which messes with jsthemis_install usage during tests, as soon as I figure out the way to install dev dependencies of JsThemis into node_modules in the source tree root along with JsThemis itself. Then call jsthemis_install to clear our compiled code before switching to a different Node.js.

Lagovas · 2020-04-06T08:46:27Z

Makefile

@@ -563,34 +563,6 @@ endif

 uninstall: rbthemis_uninstall

-## Node.js #####################


now we can't install jsthemis from root folder?

Well, to be honest, make jsthemis_install is not of much use for the users. Yes, it installs JsThemis into node_modules... in Themis source tree root. You can't use it to build and install JsThemis from source into your own application's node_modules right now. And I don't really see how it could be used at all.

With these changes make jsthemis_install works are before, installing JsThemis into node_modules in the current directory (which must be Themis source root).

I also don't really like the fact that we have to pollute the source tree with node_modules (the source may be mounted as read-only, with only build directory being writable), but I guess there is no way around that in JavsScript land.

I figured that maybe limiting it to the wrapper source directory will be better (as it allows to special mocha's version in one place), but that brings other set of issues: the tests currently require('jsthemis') which requires JsThemis to be installed into node_modules somewhere. The proper way to do this seems to be npm link, but for some reason it cannot be used twice because it removes other dependencies like nan from node_modules. I've been experimenting with this for some time since I've noticed that red build, but have not found a good solution to that for now.

Actually I don't know ways how to install js package from sources to some app folder. Only by manual copying required folder or installing by tarball from app's folder. In common way all devsinstall dependencies from registry and package.json. So if someone wants to install it from sources, it is his responsibility to do it correct. Simplest way is to install it globally to system or at some place of a hierarchy of dev environment (https://nodejs.org/api/modules.html#modules_loading_from_node_modules_folders). So user should change folder to any folder in such hierarchy and copy here built jsthemis to use it in own app.

One moment where we can simplify installing is to use user's npm flags in our makefile and allow to pass --global/--local params to npm install to allow them install it globally, IMHO

--global flag is mostly useful to install binaries, not libraries. Most projects should use local installation to fetch only their dependencies and have something packageable in their local node_modules, not install a library globally and expect it to be available when the application is deployed. That's what npm install will do by default when a project has JsThemis in package.json as a dependency.

Now I've add a jsthemis target that can be used to prepare a tarball with JsThemis from the source tree which can then be used by developers to install JsThemis manually in whatever way they want.

vixentael

agree with @Lagovas questions

Add a new target "jsthemis" that can be used to build JsThemis tarball in the build directory. Finally we have something to provide as instructions on how to build JsThemis from source. Make sure "npm pack" does not output not-so-correct filename to stdout. If only there was a way to make it put its output into a different directory...

npm seems to be really insinstent on that we either publish the thing we want to test on npm, or use relative paths. An alternative to this is using "npm link" to link the current directory into local node_modules, but that 1) modifies global npm state, 2) does not work reliably if run twice as for some reason it removes package dependencies on the second run. So I gave up. Move the test directory from top-level "tests" directly into JsThemis source tree, just like we do with WasmThemis (for the same reason).

Lagovas · 2020-04-06T14:32:13Z

src/wrappers/themis/jsthemis/test/test.js

@@ -1,4 +1,4 @@
-var addon = require('jsthemis');
+var addon = require('..');


as I remember, the main idea of tests/<wrapper>/ folder is to have one place of all tests which allow to test installed wrapper to system. And tests inside package's folders used to test their source code and functionality.
tests/jsthemis/test.js and src/wrappers/themis/jsthemis/test/test.js have different purposes. First to check the installed package, second to test code from sources.

I think we can get away with not having a standalone test suite, given that JsThemis normally will not be installed globally. However, if we package the tests along with the source code then it's possible to test JsThemis by going into its directory in node_modules and running npm install && npm test from there, without having to check out the repository.

It seems that npm does not provide a way to install a package and run its tests. I guess it is expected that developers do all the testing before publishing it and the users don't need to do it.

I think we should pack tests with source codes anyway. It's useful when user will install via npm and will try to change/patch something in-place and test it.

ilammy · 2020-04-06T14:34:39Z

Okay, this is my best attempt so far which seems to work on my machine. Let's see if it works for CI.

I've introduced a new Make target jsthemis which can be used to manually build but not install JsThemis tarball. This way users finally have some easy way to "build JsThemis from source". Note that it only packs the sources. Native module will be compiled on npm install.

make test_js does not require prior installation of JsThemis, you can run it straight away from the source tree. It will use JsThemis from the source directory. The cost of this is that we have to move tests into JsThemis source tree, out from the top-level tests directory. (This is how we do WasmThemis too.)

I was not able to trick Mocha to load JsThemis from src/wrappers/themis/jsthemis when running tests from tests/jsthemis, it always ignores the current directory and goes looking for node_modules from the location of JavaScript file it currently executes. If we were installing JsThemis into source tree root that would have worked, but that's exactly what I would like to avoid to make test_js target independent. So here it is.

vixentael

🤔

ilammy added 3 commits April 3, 2020 18:02

ilammy added infrastructure Automated building and packaging tests Themis test suite W-JsThemis 🍭 Wrapper: JsThemis for Node.js, JavaScript API, npm packages labels Apr 3, 2020

ilammy requested review from Lagovas, shadinua and vixentael as code owners April 3, 2020 15:09

Lagovas reviewed Apr 6, 2020

View reviewed changes

vixentael reviewed Apr 6, 2020

View reviewed changes

ilammy added 2 commits April 6, 2020 16:22

Lagovas reviewed Apr 6, 2020

View reviewed changes

vixentael approved these changes Apr 6, 2020

View reviewed changes

shadinua approved these changes Apr 8, 2020

View reviewed changes

ilammy merged commit e3a90c1 into cossacklabs:master Apr 8, 2020

ilammy deleted the jsthemis-package branch April 8, 2020 15:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify JsThemis packaging and test launch #618

Simplify JsThemis packaging and test launch #618

ilammy commented Apr 3, 2020 •

edited

Lagovas Apr 6, 2020

ilammy Apr 6, 2020

Lagovas Apr 6, 2020

ilammy Apr 6, 2020

Lagovas Apr 6, 2020

ilammy Apr 6, 2020

vixentael left a comment

Lagovas Apr 6, 2020

ilammy Apr 6, 2020

Lagovas Apr 6, 2020

ilammy commented Apr 6, 2020

vixentael left a comment

		# Run JsThemis tests
		rm -rf src/wrappers/themis/jsthemis/node_modules

		@@ -563,34 +563,6 @@ endif

		uninstall: rbthemis_uninstall

		## Node.js #####################

		@@ -1,4 +1,4 @@
		var addon = require('jsthemis');
		var addon = require('..');

Simplify JsThemis packaging and test launch #618

Simplify JsThemis packaging and test launch #618

Conversation

ilammy commented Apr 3, 2020 • edited

Checklist

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

vixentael 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

ilammy commented Apr 6, 2020

vixentael left a comment

Choose a reason for hiding this comment

ilammy commented Apr 3, 2020 •

edited