fix: use yarn link instead of yalc in Dockerfile.contrib #181

AlCalzone · 2021-01-08T23:53:46Z

I guess this could avoid the duplicate config dependencies we're currently seeing. However I couldn't test it since I have no clue how to run the container after executing the command from the docs.

/cc @larstobi

fixes: #173
(I hope)

coveralls · 2021-01-08T23:54:44Z

Pull Request Test Coverage Report for Build 474459310

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 25.408%

Totals
Change from base Build 469480398:	0.0%
Covered Lines:	1967
Relevant Lines:	7908

💛 - Coveralls

larstobi · 2021-01-08T23:56:55Z

I wasn't able to get yarn link work great when I tried it. I'll try again tomorrow. 😊

blhoward2 · 2021-01-09T01:40:50Z

I’ll try to build one later and see. Also I find it hilarious that you don’t know how to run docker. (Which I resisted for a long time but it ends up being super useful.)

blhoward2 · 2021-01-09T02:52:13Z

@AlCalzone Two things. First, this image does not start when built with the node-zwave-js master branch. Error message below. That master branch does start when built with #84, so is this just a zwave version mismatch? (Boy I'll be glad when fix#23 is finally merged...)

Second: I dropped into the container and I can confirm that it only contains one copy of the device files, in node_modules/@zwave-js/config/config/devices/

21:50:57.711 DRIVER   ███████╗ ██╗    ██╗  █████╗  ██╗   ██╗ ███████╗             ██╗ ███████╗

                      ╚══███╔╝ ██║    ██║ ██╔══██╗ ██║   ██║ ██╔════╝             ██║ ██╔════╝

                        ███╔╝  ██║ █╗ ██║ ███████║ ██║   ██║ █████╗   █████╗      ██║ ███████╗

                       ███╔╝   ██║███╗██║ ██╔══██║ ╚██╗ ██╔╝ ██╔══╝   ╚════╝ ██   ██║ ╚════██║

                      ███████╗ ╚███╔███╔╝ ██║  ██║  ╚████╔╝  ███████╗        ╚█████╔╝ ███████║

                      ╚══════╝  ╚══╝╚══╝  ╚═╝  ╚═╝   ╚═══╝   ╚══════╝         ╚════╝  ╚══════╝

21:50:57.715 DRIVER   version 6.0.0-alpha.3-034829f

21:50:57.716 DRIVER   

21:50:57.717 DRIVER   starting driver...

21:50:57.745 DRIVER   opening serial port /dev/ttyACM0

2021-01-08 21:50:57.759 INFO APP: Listening on port 8091

21:50:57.779 DRIVER   serial port opened

(node:1) UnhandledPromiseRejectionWarning: TypeError: core_1.createLogTransports is not a function

    at getLogger (/usr/src/app/node_modules/@zwave-js/serial/src/Logger.ts:22:17)

    at logMessageHeader (/usr/src/app/node_modules/@zwave-js/serial/src/Logger.ts:66:2)

    at Object.NAK (/usr/src/app/node_modules/@zwave-js/serial/src/Logger.ts:51:19)

    at ZWaveSerialPort.writeAsync (/usr/src/app/node_modules/@zwave-js/serial/src/ZWaveSerialPortBase.ts:160:17)

    at Driver.writeSerial (/usr/src/app/node_modules/zwave-js/src/lib/driver/Driver.ts:2174:23)

    at Driver.writeHeader (/usr/src/app/node_modules/zwave-js/src/lib/driver/Driver.ts:2169:15)

    at Immediate.<anonymous> (/usr/src/app/node_modules/zwave-js/src/lib/driver/Driver.ts:655:15)

(node:1) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 5)

(node:1) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

jcam · 2021-01-09T02:56:06Z

I just rebuilt my bridge controller branch, and the duplicate copy of the config folder is gone, though it doesn't appear to be using my the config folder from my branch at all, and is now pulling the only copy of config from some upstream (maybe node latest)

blhoward2 · 2021-01-09T02:58:24Z

@jcam I'm surprised it starts for you at all. What build command did you use?

jcam · 2021-01-09T03:02:10Z

I'm building against https://github.com/jcam/node-zwave-js/tree/bridge-application-command which was rebased against master yesterday, so it doesn't have any of the commits after ci: disable index update bot command

though, that is only two commits and neither of them should have had any effect on your error... odd...

jcam · 2021-01-09T03:06:52Z

Are you sure you're building against master? that seems to have some of the unfinished logging code in the error?
In any case, here was my build command:
$ DOCKER_BUILDKIT=1 docker build --build-arg SRC=git-clone-src --build-arg Z2M_BRANCH=docker/yarn-link --build-arg ZWJ_BRANCH=bridge-application-command --no-cache -f zwavejs2mqtt/docker/Dockerfile.contrib -t hikeit/zwavejs2mqtt:zwave700 .

sort of related, are any of y'all on slack or discord?

blhoward2 · 2021-01-09T03:08:41Z

Are you sure you're building against master? that seems to have some of the unfinished logging code in the error?

And I just double checked. And they were brand new clones from minutes ago.

I'm on the zwave2mqtt slack channel

blhoward2 · 2021-01-09T03:12:17Z

I'm going to try forking and merging in both fix#23 and this PR, then build that against the master node-zwave-js.

blhoward2 · 2021-01-09T05:55:18Z

@AlCalzone I've run this down. You are removing:

RUN yalc add zwave-js && \
    yalc add @zwave-js/config && \
    yalc add @zwave-js/core && \
    yalc add @zwave-js/shared && \
    yalc add @zwave-js/serial

Instead of removing those entirely changing:

RUN yarn link zwave-js && yarn link @zwave-js/core
to
RUN yarn link zwave-js && yarn link @zwave-js/core && yarn link @zwave-js/config

fixes this and causes the correct device files to be in the correct place when the cp command is later run. I've built an image off this and it works. I'm unsure if @zwave-js/shared or @zwave-js/serial should also be linked vs added/removed.

AlCalzone · 2021-01-09T09:24:52Z

@blhoward2 the strange thing is - if I work locally on my PC without docker, I don't even need to link @zwave-js/core. Think I'm going to add all links just to be sure.

AlCalzone · 2021-01-09T09:27:49Z

@blhoward2 try now please

blhoward2 · 2021-01-09T15:28:46Z

@blhoward2 the strange thing is - if I work locally on my PC without docker, I don't even need to link @zwave-js/core. Think I'm going to add all links just to be sure.

Do you have the up-to-date core and device files being used though? It’ll start with just building Zwavejs2mqtt because it pulls in the alpha release as a dependency. It’s just not using the master branch.

Or are you copying node-zwave-js into the final folder manually, which overwrites them? That’s all this seems to do. It dynamically maps them into the Zwavejs2mqtt folder so they’re in position when you issue the final cp. You could instead cp those folders from node-zwave-js and then not copy them from Zwavejs2mqtt. It’s a sequence thing so anytime you subsequently build node-zwave-js and copy it into position without touching Zwavejs2mqtt you’d fix the problem.

Or you’re somehow installing node-zwave-js on a global basis and it’s automatically linking them to satisfy the dependency. Which is sort of how I’d think it would work.

jcam · 2021-01-09T15:36:56Z

I'm still seeing issues related to this.

I merged the latest zwavejs2mqtt master into this branch, and I also merged fix#23, if that could be related, but I don't think so.

In https://github.com/zwave-js/zwavejs2mqtt/blob/master/lib/Constants.js there is a reference:
const { lookupMeterScale, lookupMeter } = require('@zwave-js/config')

With this latest Dockerfile.contrib update, I'm getting this:

TypeError: lookupMeter is not a function
at Object.meterType (/usr/src/app/lib/Constants.js:21:19)
at Gateway.discoverValue (/usr/src/app/lib/Gateway.js:1598:32)
at Gateway.onNodeStatus (/usr/src/app/lib/Gateway.js:364:12)
at ZwaveClient.emit (events.js:314:20)
at ZwaveClient.EventEmitter.emit (domain.js:483:12)
at ZwaveClient.onNodeStatus (/usr/src/app/lib/ZwaveClient.js:246:10)
at ZwaveClient.onNodeReady (/usr/src/app/lib/ZwaveClient.js:274:16)
at ZWaveNode.emit (events.js:326:22)
at ZWaveNode.EventEmitter.emit (domain.js:483:12)
at ZWaveNode.onReadyChange (/usr/src/app/node_modules/zwave-js/src/lib/node/Node.ts:440:19)
at /usr/src/app/node_modules/zwave-js/src/lib/node/Node.ts:226:10
at Interpreter.update (/usr/src/app/node_modules/xstate/lib/interpreter.js:255:17)
at /usr/src/app/node_modules/xstate/lib/interpreter.js:125:23
at Scheduler.process (/usr/src/app/node_modules/xstate/lib/scheduler.js:60:13)
at Scheduler.schedule (/usr/src/app/node_modules/xstate/lib/scheduler.js:44:14)
at Proxy.Interpreter.send (/usr/src/app/node_modules/xstate/lib/interpreter.js:121:29)

blhoward2 · 2021-01-09T15:58:36Z

Just had a though. @AlCalzone would it make more sense to link those in before you ever do the npm install in Zwavejs2mqtt? As it is you end up building node-zwave-js twice. Would that cause npm to see that the dependency is already met? Do we need to list it as a dependency at all if it’s being linked in by the script? Or is that necessary for other installation methods?

blhoward2 · 2021-01-09T17:27:14Z

I'm still seeing issues related to this.

@jcam Which version of node-zwave-js are you building against? I'm building one now based on the cc/hail branch which doesn't include today's merges to see if it is due to one of those.

Edit: It starts fine for me based off that older branch.

jcam · 2021-01-09T17:34:40Z

I'm building against master + the bridge-application-command commit, as my 700 stick won't work without that commit.
It starts up fine, but gets the lookupMeter errors during enumeration. Still works, still has no zwave logs...

blhoward2 · 2021-01-09T17:37:39Z

@robertsLando Is the error above caused by? zwave-js/node-zwave-js@a5d4729

…s2mqtt into docker/yarn-link

AlCalzone · 2021-01-09T18:57:54Z

I changed it again so all packages get linked, just to make sure nothing unnecessary from the original install lingers around

larstobi · 2021-01-09T20:19:43Z

I haven't been able to reproduce the duplicate config files from #173 yet, but I'm still working on it.

robertsLando · 2021-01-09T21:00:53Z

@AlCalzone is this ready to be merged?

AlCalzone · 2021-01-09T21:23:14Z

I think blhoward2 mentioned that it works. But doesn't hurt to have one more confirmation.

jcam · 2021-01-09T21:31:38Z

These build updates work for me as well.

blhoward2 · 2021-01-09T21:32:35Z

Yep, works good on my end. And I dropped into the container and verified the contents of the image.

larstobi

This works on my machine as well. Looks good! :-)

robertsLando

LGTM

fix: use yarn link instead of yalc

1abfbc9

AlCalzone requested a review from robertsLando January 8, 2021 23:54

AlCalzone mentioned this pull request Jan 8, 2021

[bug] GE dimmers/switches do not reflect double taps and take the device dim out of sync in HA #116

Closed

3 tasks

fix: @zwave-js/core should not need to be linked I think

f822cf3

AlCalzone mentioned this pull request Jan 9, 2021

[bug] Meter report errors zwave-js/node-zwave-js#1316

Closed

blhoward2 mentioned this pull request Jan 9, 2021

[bug] Two sets of device files exist in docker images and most recent device files from node-zwave-js are not used #173

Closed

3 tasks

fix: link all packages

2268242

AlCalzone marked this pull request as ready for review January 9, 2021 09:27

Merge branch 'docker/yarn-link' of https://github.com/zwave-js/zwavej…

f63062a

…s2mqtt into docker/yarn-link

larstobi approved these changes Jan 9, 2021

View reviewed changes

robertsLando approved these changes Jan 10, 2021

View reviewed changes

robertsLando merged commit 107471b into master Jan 10, 2021

robertsLando deleted the docker/yarn-link branch January 10, 2021 12:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: use yarn link instead of yalc in Dockerfile.contrib #181

fix: use yarn link instead of yalc in Dockerfile.contrib #181

AlCalzone commented Jan 8, 2021 •

edited

Loading

coveralls commented Jan 8, 2021 •

edited

Loading

larstobi commented Jan 8, 2021

blhoward2 commented Jan 9, 2021

blhoward2 commented Jan 9, 2021 •

edited

Loading

jcam commented Jan 9, 2021

blhoward2 commented Jan 9, 2021

jcam commented Jan 9, 2021 •

edited

Loading

jcam commented Jan 9, 2021

blhoward2 commented Jan 9, 2021

blhoward2 commented Jan 9, 2021

blhoward2 commented Jan 9, 2021 •

edited

Loading

AlCalzone commented Jan 9, 2021

AlCalzone commented Jan 9, 2021

blhoward2 commented Jan 9, 2021 •

edited

Loading

jcam commented Jan 9, 2021 •

edited

Loading

blhoward2 commented Jan 9, 2021

blhoward2 commented Jan 9, 2021 •

edited

Loading

jcam commented Jan 9, 2021

blhoward2 commented Jan 9, 2021 •

edited

Loading

AlCalzone commented Jan 9, 2021 •

edited

Loading

larstobi commented Jan 9, 2021

robertsLando commented Jan 9, 2021

AlCalzone commented Jan 9, 2021

jcam commented Jan 9, 2021

blhoward2 commented Jan 9, 2021 •

edited

Loading

larstobi left a comment

robertsLando left a comment

fix: use yarn link instead of yalc in Dockerfile.contrib #181

fix: use yarn link instead of yalc in Dockerfile.contrib #181

Conversation

AlCalzone commented Jan 8, 2021 • edited Loading

coveralls commented Jan 8, 2021 • edited Loading

Pull Request Test Coverage Report for Build 474459310

💛 - Coveralls

larstobi commented Jan 8, 2021

blhoward2 commented Jan 9, 2021

blhoward2 commented Jan 9, 2021 • edited Loading

jcam commented Jan 9, 2021

blhoward2 commented Jan 9, 2021

jcam commented Jan 9, 2021 • edited Loading

jcam commented Jan 9, 2021

blhoward2 commented Jan 9, 2021

blhoward2 commented Jan 9, 2021

blhoward2 commented Jan 9, 2021 • edited Loading

AlCalzone commented Jan 9, 2021

AlCalzone commented Jan 9, 2021

blhoward2 commented Jan 9, 2021 • edited Loading

jcam commented Jan 9, 2021 • edited Loading

blhoward2 commented Jan 9, 2021

blhoward2 commented Jan 9, 2021 • edited Loading

jcam commented Jan 9, 2021

blhoward2 commented Jan 9, 2021 • edited Loading

AlCalzone commented Jan 9, 2021 • edited Loading

larstobi commented Jan 9, 2021

robertsLando commented Jan 9, 2021

AlCalzone commented Jan 9, 2021

jcam commented Jan 9, 2021

blhoward2 commented Jan 9, 2021 • edited Loading

larstobi left a comment

Choose a reason for hiding this comment

robertsLando left a comment

Choose a reason for hiding this comment

AlCalzone commented Jan 8, 2021 •

edited

Loading

coveralls commented Jan 8, 2021 •

edited

Loading

blhoward2 commented Jan 9, 2021 •

edited

Loading

jcam commented Jan 9, 2021 •

edited

Loading

blhoward2 commented Jan 9, 2021 •

edited

Loading

blhoward2 commented Jan 9, 2021 •

edited

Loading

jcam commented Jan 9, 2021 •

edited

Loading

blhoward2 commented Jan 9, 2021 •

edited

Loading

blhoward2 commented Jan 9, 2021 •

edited

Loading

AlCalzone commented Jan 9, 2021 •

edited

Loading

blhoward2 commented Jan 9, 2021 •

edited

Loading