chore(ci): fix syntax error in workflow file, remove Node 20 for now

[JKRhb]

#974 introduced a small syntax error that caused the workflow not to run anymore. This PR provides a simple fix.

[JKRhb]

Hmm, looks as if with this change, the node-gyp error under Node 20 reappears :/ (I also get the same error locally with Node 20.)

[danielpeintner]

@egekorkan @relu91

Shall we revert #974 again as a quick solution and work on the actual issue again?

EDIT: as an alternative we can exclude mbus for now as we do or firestore?

[relu91]

EDIT: as an alternative we can exclude mbus for now as we do or firestore?

I think it's okay to exclude it. @LennyHEVS if you have time to look into this it would be nice.

[danielpeintner]

@JKRhb may I ask you to exclude mbus in this PR for the time being as we do for firestore?

see

node-wot/package.json

Line 41 in 137f2e5

"./packages/binding-!(firestore-browser-bundle|firestore)",

and see

node-wot/tsconfig.json

Line 32 in 137f2e5

/*{ "path": "./packages/binding-firestore" }, Disabled see https://github.com/eclipse/thingweb.node-wot/issues/827*/

[JKRhb]

The mbus package is now excluded/disabled :)

[JKRhb]

Hmm, seems like disabling the mbus package is not enough. So I guess we could either merge this one (without the mbus change) to fix the syntax error or #990 gets merged first and then we rebase this one against the new main branch version.

[danielpeintner]

@relu91 @egekorkan opinions?
I think #990 is a way forward and if there is interest we can bring mbus & firestore back...

[relu91]

Just checked the error, seems that the mbus protocol is using and old version of serialport. There is a newer mbus release, it is worth plugging that in and seeing if it resolves this problem and doesn't cause any other issues.

[danielpeintner]

ae3e3a3 brings back mbus binding and updates node-mbus from version 1.2.2 to 2.0.0.

Locally the mbus tests were working just fine.
Since I don't have Node.js 20 let's see what the CI does...

EDIT: the tests are failing again. I think #990 is the way forward.
@relu91 ?

[relu91]

EDIT: the tests are failing again. I think #990 is the way forward.

Yes, even though I'm now a little bit afraid that the install will also fail for modbus.

[danielpeintner]

Yes, even though I'm now a little bit afraid that the install will also fail for modbus.

Do you want to test/check this before we go along this path...

[relu91]

Do you want to test/check this before we go along this path...

I have a little bit of a problem installing node 20. Maybe we can faster if we just try.

[danielpeintner]

I updated #990 with this syntax fix.
Unfortunately Node.j 20 CI run still fails even without mbus and firestore, see https://github.com/eclipse-thingweb/node-wot/actions/runs/5014360405/jobs/8988511146?pr=990

Mhh, not sure but I cannot properly see where the error comes from.
@relu91 modbus as you were guessing?

`-- @node-wot/binding-modbus@0.8.7 -> .\packages\binding-modbus
  `-- modbus-serial@8.0.3
    `-- serialport@9.2.8

Note: there is modbus-serial@8.0.11
I tried to update locally but I get failures ... even locally.

[danielpeintner]

I think we should have a quick fix soon so that our tests are properly run again for PRs.

A proposal could be to run Node.js 16 and 18 only (for now) and we investigate why mbus and modbus are failing with Node.js 20.
What do you think?

[relu91]

I think we should have a quick fix soon so that our tests are properly run again for PRs.

A proposal could be to run Node.js 16 and 18 only (for now) and we investigate why mbus and modbus are failing with Node.js 20. What do you think?

yes, let's stay just with 16 and 18 while we understand where is the problem. I hope that it is not a serialport issue....

[danielpeintner]

@JKRhb may I ask you to revert all changes other than the actual syntax fix and removing also Node.js 20 for now. Thanks and sorry for the back and forth

[JKRhb]

@JKRhb may I ask you to revert all changes other than the actual syntax fix and removing also Node.js 20 for now. Thanks and sorry for the back and forth

No problem at all :) The PR should now be updated accordingly :)