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.

Sign up for GitHub

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

Jump to bottom

Simplify JsThemis packaging and test launch #618

Merged
merged 5 commits into from
Apr 8, 2020
Merged

Simplify JsThemis packaging and test launch #618

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a94725c
Improve JsThemis packaging
ilammy Apr 2, 2020
9ffa7ce
Move and simplify JsThemis installation targets
ilammy Apr 3, 2020
d534906
Simplify JsThemis unit test launch
ilammy Apr 3, 2020
7d588f8
Convenience "jsthemis" target
ilammy Apr 6, 2020
516de7c
Move test directory into JsThemis source tree
ilammy Apr 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 0 additions & 12 deletions .circleci/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -502,9 +502,6 @@ jobs:
nvm deactivate
echo "node --version: $(node --version)"
echo "npm --version: $(npm --version)"
# Install JsThemis locally
rm -rf node_modules package-lock.json
make jsthemis_install
# Run JsThemis tests
make test_js
- run:
Expand All @@ -515,9 +512,6 @@ jobs:
nvm use v8
echo "node --version: $(node --version)"
echo "npm --version: $(npm --version)"
# Install JsThemis locally
rm -rf node_modules package-lock.json
make jsthemis_install
# Run JsThemis tests
make test_js
# Run WasmThemis tests
Expand All @@ -530,9 +524,6 @@ jobs:
nvm use v10
echo "node --version: $(node --version)"
echo "npm --version: $(npm --version)"
# Install JsThemis locally
rm -rf node_modules package-lock.json
make jsthemis_install
# Run JsThemis tests
make test_js
# Run WasmThemis tests
Expand All @@ -545,9 +536,6 @@ jobs:
nvm use v12
echo "node --version: $(node --version)"
echo "npm --version: $(npm --version)"
# Install JsThemis locally
rm -rf node_modules package-lock.json
make jsthemis_install
# Run JsThemis tests
make test_js
# Run WasmThemis tests
Expand Down
7 changes: 1 addition & 6 deletions .github/workflows/test-nodejs.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,15 +50,10 @@ jobs:
run: |
make
sudo make install
make prepare_tests_all
- name: Install JsThemis
- name: Run test suite
run: |
echo Node.js: $(node --version)
echo npm: $(npm --version)
rm -rf node_modules package-lock.json
make jsthemis_install
- name: Run test suite
run: |
make test_js

examples:
Expand Down
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,7 @@ _Code:_
- **Node.js**

- New class `SymmetricKey` can be used to generate symmetric keys for Secure Cell ([#562](https://github.com/cossacklabs/themis/pull/562)).
- New makefile target `make jsthemis` can be used to build JsThemis from source ([#618](https://github.com/cossacklabs/themis/pull/618)).

- **PHP**

Expand Down Expand Up @@ -369,6 +370,8 @@ _Infrastructure:_
- Automated benchmarking harness is now tracking Themis performance. See [`benches`](https://github.com/cossacklabs/themis/tree/master/benches/) ([#580](https://github.com/cossacklabs/themis/pull/580)).
- Added automated tests for all code samples in documentation, ensuring they are always up-to-date ([#600](https://github.com/cossacklabs/themis/pull/600)).
- All 13 supported platforms are verified on GitHub Actions, along with existing CircleCI and Bitrise tests ([#600](https://github.com/cossacklabs/themis/pull/600)).
- New Makefile targets:
- `make jsthemis` builds JsThemis from source ([#618](https://github.com/cossacklabs/themis/pull/618)).

## [0.12.0](https://github.com/cossacklabs/themis/releases/tag/0.12.0), September 27th 2019

Expand Down
28 changes: 0 additions & 28 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -563,34 +563,6 @@ endif

uninstall: rbthemis_uninstall

## Node.js #####################
Copy link
Collaborator

Choose a reason for hiding this comment

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

now we can't install jsthemis from root folder?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

--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.


JSTHEMIS_PACKAGE_VERSION=$(shell cat src/wrappers/themis/jsthemis/package.json \
| grep version \
| head -1 \
| awk -F: '{ print $$2 }' \
| sed 's/[",]//g' \
| tr -d '[[:space:]]')

jsthemis_install: CMD = cd src/wrappers/themis/jsthemis && npm pack && mv jsthemis-$(JSTHEMIS_PACKAGE_VERSION).tgz ../../../../build && cd - && npm install nan && npm install ./build/jsthemis-$(JSTHEMIS_PACKAGE_VERSION).tgz
jsthemis_install:
ifdef NPM_VERSION
@echo -n "jsthemis install "
@$(BUILD_CMD_)
else
@echo "Error: npm not found"
@exit 1
endif

jsthemis_uninstall: CMD = rm -rf build/jsthemis-$(JSTHEMIS_PACKAGE_VERSION).tgz && npm uninstall jsthemis
jsthemis_uninstall:
ifdef NPM_VERSION
@echo -n "jsthemis uninstall "
@$(BUILD_CMD_)
endif

uninstall: jsthemis_uninstall

## Python ######################

ifdef PIP_VERSION
Expand Down
1 change: 1 addition & 0 deletions src/wrappers/themis/jsthemis/.gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
node_modules
5 changes: 5 additions & 0 deletions src/wrappers/themis/jsthemis/.npmignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
# Themis build system
jsthemis.mk

# Results of local "npm pack"
*.tgz
29 changes: 29 additions & 0 deletions src/wrappers/themis/jsthemis/jsthemis.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@ JSTHEMIS_OBJ = $(OBJ_PATH)/$(JSTHEMIS_SRC)

JSTHEMIS_SOURCES = $(wildcard $(JSTHEMIS_SRC)/*.cpp)
JSTHEMIS_HEADERS = $(wildcard $(JSTHEMIS_SRC)/*.hpp)
JSTHEMIS_PACKAGE += $(JSTHEMIS_SRC)/README.md
JSTHEMIS_PACKAGE += $(JSTHEMIS_SRC)/LICENSE
JSTHEMIS_PACKAGE += $(JSTHEMIS_SRC)/package.json

# Unfortunately, clang-tidy requires full compilation flags to be able to work and
# node-gyp add quite a few custom flags with include search paths. It also requires
Expand Down Expand Up @@ -58,3 +61,29 @@ $(JSTHEMIS_OBJ)/warning_check:
$(warning NodeJS not installed, JsThemis code will not be checked)

endif # ifdef NPM_VERSION

$(BUILD_PATH)/jsthemis.tgz: $(JSTHEMIS_SOURCES) $(JSTHEMIS_HEADERS) $(JSTHEMIS_PACKAGE)
@cd $(BUILD_PATH) && npm pack $(abspath $(JSTHEMIS_SRC)) > /dev/null
@mv $(BUILD_PATH)/jsthemis-*.tgz $(BUILD_PATH)/jsthemis.tgz
@echo $(BUILD_PATH)/jsthemis.tgz

jsthemis: $(BUILD_PATH)/jsthemis.tgz

jsthemis_install: CMD = npm install $(abspath $(BUILD_PATH)/jsthemis.tgz)
jsthemis_install: $(BUILD_PATH)/jsthemis.tgz
ifdef NPM_VERSION
@echo -n "jsthemis install "
@$(BUILD_CMD_)
else
@echo "Error: npm not found"
@exit 1
endif

jsthemis_uninstall: CMD = npm uninstall jsthemis
jsthemis_uninstall:
ifdef NPM_VERSION
@echo -n "jsthemis uninstall "
@$(BUILD_CMD_)
endif

uninstall: jsthemis_uninstall
Loading