Node-{arduino-firmata,cylon,hid,serialport}: allow compilation with `CONFIG_AUTOREMOVE` #7652

cotequeiroz · 2018-12-13T11:55:51Z

Maintainer: @blogic
Compile tested: ramips, mipsel_74kc, openwrt master
Run tested: none

Description:
node-arduino-firmata, node-cylon, node-hid, and node-serialport all set npm_config_nodedir to node's build dir. When the buildbots, or buildpr build the packages, they do a clean-build, clearing up node's build dir. Normally using node's build dir is the best way to do it, but it is not the only way. It can be set to STAGING_DIR/usr, and it will pickup the include files installed there.
I can't properly run-test this. I've compile-tested the packages, and compared the resulting files. These packages install some (all?) of their build files, so the files don't match exactly. More specifically, files like this:

--- a/lib/node/node-hid/build/HID.target.mk
+++ b/lib/node/node-hid/build/HID.target.mk
@@ -79,13 +79,13 @@ CFLAGS_CC_Release := \
        -exceptions

 INCS_Release := \
-       -I/home/equeiroz/src/openwrt-asus/build_dir/target-mipsel_74kc_musl/node-v8.12.0/include/node \
-       -I/home/equeiroz/src/openwrt-asus/build_dir/target-mipsel_74kc_musl/node-v8.12.0/src \
-       -I/home/equeiroz/src/openwrt-asus/build_dir/target-mipsel_74kc_musl/node-v8.12.0/deps/openssl/config \
-       -I/home/equeiroz/src/openwrt-asus/build_dir/target-mipsel_74kc_musl/node-v8.12.0/deps/openssl/openssl/include \
-       -I/home/equeiroz/src/openwrt-asus/build_dir/target-mipsel_74kc_musl/node-v8.12.0/deps/uv/include \
-       -I/home/equeiroz/src/openwrt-asus/build_dir/target-mipsel_74kc_musl/node-v8.12.0/deps/zlib \
-       -I/home/equeiroz/src/openwrt-asus/build_dir/target-mipsel_74kc_musl/node-v8.12.0/deps/v8/include \
+       -I/home/equeiroz/src/openwrt-asus/staging_dir/target-mipsel_74kc_musl/usr/include/node \
+       -I/home/equeiroz/src/openwrt-asus/staging_dir/target-mipsel_74kc_musl/usr/src \
+       -I/home/equeiroz/src/openwrt-asus/staging_dir/target-mipsel_74kc_musl/usr/deps/openssl/config \
+       -I/home/equeiroz/src/openwrt-asus/staging_dir/target-mipsel_74kc_musl/usr/deps/openssl/openssl/include \
+       -I/home/equeiroz/src/openwrt-asus/staging_dir/target-mipsel_74kc_musl/usr/deps/uv/include \
+       -I/home/equeiroz/src/openwrt-asus/staging_dir/target-mipsel_74kc_musl/usr/deps/zlib \
+       -I/home/equeiroz/src/openwrt-asus/staging_dir/target-mipsel_74kc_musl/usr/deps/v8/include \
        -I$(srcdir)/hidapi/hidapi \
        -I$(srcdir)/node_modules/nan

There's no build_dir/target-mipsel_74kc_musl/node-v8.12.0/include/node, but there is staging_dir/target-mipsel_74kc_musl/usr/include/node where node installs its headers.

More important, the resulting binaries are all the same.

I have concerns about the deps directories, and even copied them under staging_dir/usr/deps, but they did not make any difference in the final binaries. What tipped me into not wanting to copy them was the presence of openssl headers there. They are from version 1.0.2, but the packages all compile with openwrt/openwrt#965 (choosing version 1.1.1), and there's indication that openwrt 19.01 should use openssl 1.1.1. Packages should not work if you mix 1.0.2 headers with 1.1.1 libraries, given the overwhelming changes in API.

Another alternatives are:

instruct the bots to not clean up node's build dir (I don't know how to do that), and leave everything as is;
copying after build/extracting the entire node source dir to staging_dir, and point npm_node_dir there.

Signed-off-by: Eneas U de Queiroz [email protected]

cotequeiroz · 2018-12-13T11:57:46Z

@thess, you've helped me with some difficult ones, tell me what you think of this one.

thess · 2018-12-13T21:31:45Z

Working on a few minor tweaks -- you have got the right idea. AUTOREMOVE is not the issue though. PREFIX=xxx should instead be inline --prefix "xxx" to the npm install command. The darn results were ending up in my home directory!!

The post install cleanup in node-serialport is referencing node-hid path. I'm also not sure why the node-hid and node-serialport packages are so much larger than the other node-xxx packages?

cotequeiroz · 2018-12-14T11:56:25Z

I disagree with you assessment of AUTOREMOVE. Build fails (here at least) because it can't find the node sources passed on as the npm_config_node parameter after they were cleaned up by AUTOREMOVE (unless it is not what AUTOREMOVE does, in which case, substitute it with the real culprit). I never had trouble with having files end up in my home directory, but your proposed change may or may not change that.

The way these node packages work is not really ideal for the openwrt build system. They download their dependencies using the current version, and build them as needed. There is a provision in their build files to lock the versions, but irrc, it has to be done at every level.

If you really want to dig deep into this, you can take a look at the huge node-mozila-iot-gateway. At one point, as I started adding openssl compatibility, it built 1741 dependent packages. I needed to patch one of those dependent packages. See #6088, if you wish. Thankfully, they updated it and dropped crypto support, so the need for patches went away. However, it exposes the unpredictable nature of the way they are built. I'm writing this from memory, BTW.

To really fix this, we would need to add these 1741 packages, or at least add them to a list, like many packages do with modules, so that they can be built predictably. How often do you think a version bump will occur?

About the node-hid references by node-serialport (this is wrong, see next message)
What happens is that node-hid is a dependency of node-serialport, so it ends up being built and installed by node-serialport. The cleanup is to remove that part, and use the openwrt-built node-hid instead.

cotequeiroz · 2018-12-14T12:22:33Z

On a second thought, this was my bad copying and pasting stuff from one package to the other in 93d6d1b 🤦‍♂️. I will add a commit to fix this right away. node-arduino-firmata is the one removing the node-serialport it builds:

rm -rf  $(1)/usr/lib/node/arduino-firmata/node_modules/serialport/

thess · 2018-12-15T23:52:14Z

IIRC - the serialport path ends with something like .../node/serial/

Also, the PREFIX var needs to be removed and use the --prefix command option for all packages. If you are missing files they are in ~/npm-global/ (or these here are duplicates - In any event it is wrong)

cotequeiroz · 2018-12-16T12:41:40Z

I’ll add a commit, probably on Tuesday.

cotequeiroz · 2018-12-18T17:03:08Z

I've added a commit changing PREFIX to --prefix, and another one to test for ~/npm-global/ presence in buildpr. I can't reproduce it here.

cotequeiroz · 2018-12-18T17:27:00Z

@thess
Help me here: buildpr is failing at the "Downloading the SDK" stage. If this is temporary, please let me know when I can try it again.

thess · 2018-12-18T18:34:41Z

Given the age of the PR, I would guess all you need to do is rebase from master to pick up the SDK file version change in the CCI script. The new script will not fall victim to this particular issue going forward.

It breaks buildbot compilation, as the node build dir is cleared before node-cylon gets built. Use files installed in staging_dir instead. Signed-off-by: Eneas U de Queiroz <[email protected]>

It breaks buildbot compilation, as the node build dir is cleared before node-hid gets built. Use files installed in staging_dir instead. Signed-off-by: Eneas U de Queiroz <[email protected]>

It breaks buildbot compilation, as the node build dir is cleared before node-serialport gets built. Use files installed in staging_dir instead. Signed-off-by: Eneas U de Queiroz <[email protected]>

It breaks buildbot compilation, as the node build dir is cleared before node-arduino-firmata gets built. Use files installed in staging_dir instead. Signed-off-by: Eneas U de Queiroz <[email protected]>

This is a fixup for a copy & paste done in 93d6d1b. Signed-off-by: Eneas U de Queiroz <[email protected]>

Signed-off-by: Eneas U de Queiroz <[email protected]>

thess · 2018-12-18T18:39:44Z

lang/node-serialport/Makefile

@@ -56,6 +56,7 @@ define Build/Compile
 	npm install --build-from-source --target_arch=$(CPU) -g \
 		--prefix="$(PKG_INSTALL_DIR)/usr/" \
 		`npm pack $(PKG_BUILD_DIR) | tail -n 1`
+	if test -d ~/npm-global; then ls -alR ~/npm-global/; false; else true; fi


I think we all would prefer it if one does not put "debug" code in their submissions for us to review - and waste time.

You should be able to test in your own environment by either building with the SDK or setting up your own CircleCI account and use a test branch if you want to debug with our CI scripts and setup.

Sorry about that. I've never used the CircleCI before, and don't know how to mimic the buildbots exactly, and that's why I sometimes struggle to fix some build failures. Can you give me a pointer to get started?
For example, I don't get any files in ~/npm-global, so I don't know what is getting copied.

cotequeiroz · 2019-04-05T11:23:34Z

@nxhack is taking over these, and he has a different way of building them that will avoid this altogether, so I'm closing this.

cotequeiroz mentioned this pull request Dec 13, 2018

node: use system libnghttp2 libuv libhttp-parser #7653

Closed

thess added the work needed label Dec 15, 2018

cotequeiroz force-pushed the node-packages_staging-dir branch 2 times, most recently from 2417d15 to f004287 Compare December 18, 2018 17:22

cotequeiroz added 9 commits December 18, 2018 16:39

node-cylon: don't use node PKG_BUILD_DIR

83c56ab

It breaks buildbot compilation, as the node build dir is cleared before node-cylon gets built. Use files installed in staging_dir instead. Signed-off-by: Eneas U de Queiroz <[email protected]>

node-hid: don't use node PKG_BUILD_DIR

92968fb

It breaks buildbot compilation, as the node build dir is cleared before node-hid gets built. Use files installed in staging_dir instead. Signed-off-by: Eneas U de Queiroz <[email protected]>

node-serialport: don't use node PKG_BUILD_DIR

83abec6

It breaks buildbot compilation, as the node build dir is cleared before node-serialport gets built. Use files installed in staging_dir instead. Signed-off-by: Eneas U de Queiroz <[email protected]>

node-arduino-firmata: don't use node PKG_BUILD_DIR

1978f84

It breaks buildbot compilation, as the node build dir is cleared before node-arduino-firmata gets built. Use files installed in staging_dir instead. Signed-off-by: Eneas U de Queiroz <[email protected]>

node-serialport: fix package name in post-install

aa20555

This is a fixup for a copy & paste done in 93d6d1b. Signed-off-by: Eneas U de Queiroz <[email protected]>

node-arduino-firmata: use --prefix, instead of PREFIX

cbce6e0

Signed-off-by: Eneas U de Queiroz <[email protected]>

node-cylon: use --prefix, instead of PREFIX

2e6e804

Signed-off-by: Eneas U de Queiroz <[email protected]>

node-hid: use --prefix, instead of PREFIX

82d9537

Signed-off-by: Eneas U de Queiroz <[email protected]>

node-serialport: use --prefix, instead of PREFIX

c6b49af

Signed-off-by: Eneas U de Queiroz <[email protected]>

thess reviewed Dec 18, 2018

View reviewed changes

cotequeiroz force-pushed the node-packages_staging-dir branch 2 times, most recently from f3b40f8 to c6b49af Compare December 18, 2018 19:06

cotequeiroz closed this Apr 5, 2019

cotequeiroz deleted the node-packages_staging-dir branch July 10, 2019 21:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Node-{arduino-firmata,cylon,hid,serialport}: allow compilation with `CONFIG_AUTOREMOVE` #7652

Node-{arduino-firmata,cylon,hid,serialport}: allow compilation with `CONFIG_AUTOREMOVE` #7652

cotequeiroz commented Dec 13, 2018

cotequeiroz commented Dec 13, 2018

thess commented Dec 13, 2018

cotequeiroz commented Dec 14, 2018 •

edited

Loading

cotequeiroz commented Dec 14, 2018 •

edited

Loading

thess commented Dec 15, 2018

cotequeiroz commented Dec 16, 2018

cotequeiroz commented Dec 18, 2018

cotequeiroz commented Dec 18, 2018

thess commented Dec 18, 2018

thess Dec 18, 2018

cotequeiroz Dec 18, 2018 •

edited

Loading

cotequeiroz commented Apr 5, 2019

Node-{arduino-firmata,cylon,hid,serialport}: allow compilation with CONFIG_AUTOREMOVE #7652

Node-{arduino-firmata,cylon,hid,serialport}: allow compilation with CONFIG_AUTOREMOVE #7652

Conversation

cotequeiroz commented Dec 13, 2018

cotequeiroz commented Dec 13, 2018

thess commented Dec 13, 2018

cotequeiroz commented Dec 14, 2018 • edited Loading

cotequeiroz commented Dec 14, 2018 • edited Loading

thess commented Dec 15, 2018

cotequeiroz commented Dec 16, 2018

cotequeiroz commented Dec 18, 2018

cotequeiroz commented Dec 18, 2018

thess commented Dec 18, 2018

thess Dec 18, 2018

Choose a reason for hiding this comment

cotequeiroz Dec 18, 2018 • edited Loading

Choose a reason for hiding this comment

cotequeiroz commented Apr 5, 2019

Node-{arduino-firmata,cylon,hid,serialport}: allow compilation with `CONFIG_AUTOREMOVE` #7652

Node-{arduino-firmata,cylon,hid,serialport}: allow compilation with `CONFIG_AUTOREMOVE` #7652

cotequeiroz commented Dec 14, 2018 •

edited

Loading

cotequeiroz commented Dec 14, 2018 •

edited

Loading

cotequeiroz Dec 18, 2018 •

edited

Loading