Unable to build from source

awhitty commented 5 years ago

Hi LaserWeb folks 👋

I've been using LaserWeb for a few days through a binary on the releases page and have been generally pleased with how well the software works! I'm still familiarizing with the project, so I hope you'll bear with me.

The Issue

I would like to contribute to the project, so I cloned the repo and ran npm install and npm run installdev. At this point, npm start fails with an error related to a dependency:

TypeError: require(...).load is not a function
    at Object.<anonymous> (<...>/LaserWeb4/node_modules/lw.comm-server/config.js:1:81)

This is due to a version mismatch between what's expected in the lw.comm-server library and what's being installed. That module's package.json specifies "dotenv": "*", and there appear to be breaking changes in dotenv (folks use require('dotenv').configure(...) these days).

Digging in a little further, I noticed that npm 5's package-lock.json was intentionally ignored in this repo's .gitignore file. A package-lock.json in this repo would have prevented that breaking change in the short run, but the issue would still have shown itself if folks worked on the lw.comm-server package independently.

I also noticed while running npm run start-app that the webpack build fails due to a missing object.omit dependency. Sure enough, src/reducers/index.js imports from object.omit, but the dependency is not listed in the package.json. I assume this was working for folks due to its presence as a transitive dependency?

Recommendations

This is as far as I have gotten so far. I have a few recommendations that I think would open this project up for more third party contributions in the future. I've ordered them from smallest scope to largest:

Remove package-lock.json from this repo's .gitignore file and commit a working package-lock.json to the repo

This change at the least should enable other developers to work on this project without spending too much time fiddling with dependencies.
Use exact package versions for all package.jsons for this repo as well as the other lw.* packages

It's very rare that something like "dotenv": "*" is actually a useful declaration in a package.json. Instead I recommend installing packages with the --save-exact flag to pin to the exact version. This policy, in addition to the package-lock.json change, should eliminate a class of issues related to breaking changes in dependencies and transitive dependencies.

The tradeoff here is that folks won't always get the latest features from a new version of a package "for free". The truth is is that they never really did get them for free. Developer A might make a change to use a method introduced in version 2.6 of a package while Developer B is still using version 2.5. Developer B might pull Developer A's changes and notice that the code has broken. Developer B has no way of knowing whether Developer A wrote buggy code or if Developer B's packages are out of date compared to Developer A's local setup. Requiring developers to save the exact version of a package in the repo eliminates this ambiguity.

Tools like Greenkeeper can reduce the friction of keeping packages up to date. In my experience, this hasn't actually been as necessary as folks would expect.
Introduce a CI tool to ensure this repo can always be built from source

I noticed y'all listed "Automate Electron Builds for all platforms" on the wishlist inside the readme. Introducing a CI workflow that ensures the project can at least be built from source in a development context is a good first step in this direction. A lot of the groundwork here could be repurposed into an automated testing rig as well as an automatic build system, and both would open this project up to more contributions.
Consolidate all lw.* packages into a monorepo

This suggestion might be a bit controversial, but given that this project has many associated packages that need to change in tandem, it may be easier to keep track of the history of the project and coordinate cross-cutting changes if every package were checked into the same repo. This approach works well for projects like React and Babel, and I imagine it would work well here too.

I'm not sure if y'all are familiar with Lerna, but it's a tool that reduces the friction around managing a monorepo of multiple packages.

tl;dr

I couldn't build from source, there's a few issues precipitating that, I have some suggestions for how to eliminate those issues, and I'm happy to incorporate those suggestions if y'all are open to them. Thanks for making useful software!!

easytarget commented 5 years ago

I just referenced this in another issue; I'm pretty concerned that laserweb could easily become abandonware unless we can find a way to maintain it against the relatively fast moving npm dependency chains, browser requirements, evolving web standards, etc.

The first need is to get to the step 2) in your scheme and use this as an opportunity to bring dependencies up to date as far as possible. I hope that could solve the scrollbar issue, for instance. I'd be useless at that but I could help with step 3) since I am a CM/Build/Devops engineer by trade.

Critically, if we can get there then we can think of how to extend Laserweb for new controllers and maybe work on a more responsive interface.

cprezzi commented 5 years ago

The situation is, that all frontend devs have left the project. I'm the only dev left, but my part is the backend (lw.comm-server). I'm not realy familar with react/redux/webpack of the frontend. I would be very happy if someone could help to upgrade all dependencies and bring the whole project to a state where also unexperienced devs could compile it.

What's important:

The frontend defs used the branch "dev-es6" for development and compiling (webpack) of the frontend only. They run lw.comm-server manually (nodejs start) to test against it.
The backend is developed in lw.comm-server (maser).
The electron installer is built in electron_bundler branch of lw.comm-server. To compile the exe, you first need to merge the changes of the master branch, then copy the frontend webpack files to the app folder and compile the exe. There are some scripts in package.json to do that.

easytarget commented 5 years ago

@cprezzi ; Thankyou for the reply, taking the time to explain things is really appreciated :-) If I can solve some of my immediate issues (job related) I will try and get a branch building. It would be a good beginning.

I didn't realise electron was involved (new to me..) but I pretty easily got the demo running.

I hope this is not a naive question; its electron only used for building the windows executable?

I'm fully on Linux here, Fedora on my laptop and lw.comm-server as a service on Raspian on the PI3 that serves my CNC. How critical is it for development?
I can see that the electron build will be needed for release, but could development happen without it?

easytarget commented 5 years ago

@awhitty ; is there any chance you could look at getting a sensible/valid package.lock together; either for the dev-es6 branch, or start another based off that to aim for some sort of roundup release so we are at least current with dependencies and can focus on real issues again.

Not: I consider laserweb feature complete for my use. There will always be a big list of improvements it could have, but it currently fits my workflow perfectly.

cprezzi commented 5 years ago

@easytarget You don't need the electron branch for development. It's only needed for building the Windows and OSX executables/installers.

The frontend (LaserWeb4) and backend (lw.comm-server) are theoretically independent applications that just communicate via websockets.

For convenience, the backend nodejs webserver also serves the frontend webpack files. This means it's enough to clone lw.comm-server and run "node server.js", then access the frontend with a chrome or chromium browser on IP port 8000 (like documented in the Raspberry Pi wiki).

For frontend development, you just need LaserWeb4/dev-es6. If you also want to test the server communication, manually start lw.comm-server before testing.

cprezzi commented 5 years ago

I brought the LaserWeb4 dev-es6 branch and lw.comm-server master to a state where I could compile both under nodejs 10.16.3.

To get that working, you have to:

Clone lw.comm-server
Run npm install.
Test if node server.js works.
Clone LaserWeb4
Run npm run installdev.
Run npm run start-app for frontend testing. This does a webpack compile and starts a browser on localhost:8080/webpack-dev-server/. (it takes some time until webpack compile is ready!)

After updating something on the frontend:

Run npm run bundle-dev in the LaserWeb4 folder. This creates the webpack files in the dist folder, which can then be copied over to the app folder of lw.comm-server (embeded website). Instead of manual copying you can run the script npm run update_lw4 in the lw.comm-server folder (needs bash console for ncp command).

I hope this gives you at least a start point which can be compiled!

awhitty commented 5 years ago

Thanks for the replies @cprezzi and @easytarget! The additional context is very helpful.

I’ll take a look at the changes you made to both repos to understand a bit more about what was going on, and I'll follow those instructions! I’m happy to PR changes to introduce a package-lock.json and pin down exact versions for packages. I’ll aim to do that tonight. I’m on pacific time, so that’ll be about 11 or 12 hours from now.

A few other things on my mind, though these topics are mostly unrelated to this specific issue:

Is there any particular structure around contributions to these repos? I assume PRs with an approving review should suffice.
Is there a policy on how the repos are versioned and how releases are made?
Is there a roadmap or backlog of features that we can groom? I see some Github issues that might make sense as a starting point, and @easytarget mentioned support for more controllers and UI adjustments.

cprezzi commented 5 years ago

For contributions I think PRs with approving reviews are a good solution
For the releases: they went with the dev that used to do the release work ;)
For the roadmap: I think the github issues are a good starting point.
For the supported controllers: I think we already support the most common controllers. So far I tried to decouple the controllers from the frontend and handle the differences in the backend (if possible). The only exception was the "marlin gcode compiler" which was easyer to implement in the frontend.

awhitty commented 5 years ago

@cprezzi Thanks for answering my questions!

I'll assign PRs to you then.

Regarding roadmap, I think we should comb through the GH issues a bit and look for some common themes. Once we get a consistent build working (I think #573 gets us there), it'll be time to prioritize feature work, and that prioritization can inform which other sorts of tech debt are worth addressing sooner vs. later.

Regarding controller support, that makes sense. I'm not yet familiar with how marlin differs from grbl or what else is out there, but I trust that judgement 😛

cprezzi commented 5 years ago

@awhitty I've invited you to the admins of the LaserWeb oragnisation, so you can do what's needed in all our repos ;)

awhitty commented 5 years ago

@cprezzi By the way, this hasn't fallen off my radar! Just got back from a long vacation, so I'm picking things back up slowly.

mfadel85 commented 4 years ago

I am not able to build the application using ubuntu, 19.10, node v13.13.0, when executing node src/index.js I get this `(node:11762) ExperimentalWarning: The ESM module loader is experimental. file:///home/mfadel/Projects/LaserWeb4-dev-es6/src/index.js:63 return ; ^

SyntaxError: Unexpected token '<' at Loader.moduleStrategy (internal/modules/esm/translators.js:81:18) at async link (internal/modules/esm/module_job.js:37:21) `

I tried another way by executing npm install

when executing npm install :

`> serialport@6.2.2 install /home/mfadel/Projects/LaserWeb4-dev-es6/node_modules/lw.comm-server/node_modules/serialport

prebuild-install || node-gyp rebuild

prebuild-install WARN install No prebuilt binaries found (target=13.13.0 runtime=node arch=x64 platform=linux) make: Entering directory '/home/mfadel/Projects/LaserWeb4-dev-es6/node_modules/lw.comm-server/node_modules/serialport/build' CXX(target) Release/obj.target/serialport/src/serialport.o In file included from ../src/./serialport.h:6, from ../src/serialport.cpp:1: ../../../../nan/nan.h: In function ‘void Nan::AsyncQueueWorker(Nan::AsyncWorker)’: ../../../../nan/nan.h:2298:62: warning: cast between incompatible function types from ‘void ()(uv_work_t)’ {aka ‘void ()(uv_work_s)’} to ‘uv_after_work_cb’ {aka ‘void ()(uv_work_s, int)’} [-Wcast-function-type] 2298 | , reinterpret_cast(AsyncExecuteComplete) | ^ ../src/serialport.cpp: In function ‘Nan::NAN_METHOD_RETURN_TYPE Open(Nan::NAN_METHOD_ARGS_TYPE)’: ../src/serialport.cpp:41:48: error: no matching function for call to ‘v8::Value::ToString()’ 41 | v8::String::Utf8Value path(info[0]->ToString()); | ^ In file included from /home/mfadel/.cache/node-gyp/13.13.0/include/node/node.h:67, from ../../../../nan/nan.h:54, from ../src/./serialport.h:6, from ../src/serialport.cpp:1: /home/mfadel/.cache/node-gyp/13.13.0/include/node/v8.h:2750:44: note: candidate: ‘v8::MaybeLocal v8::Value::ToString(v8::Local) const’ 2750 | V8_WARN_UNUSED_RESULT MaybeLocal ToString( | ^~~~ /home/mfadel/.cache/node-gyp/13.13.0/include/node/v8.h:2750:44: note: candidate expects 1 argument, 0 provided ../src/serialport.cpp:48:53: error: no matching function for call to ‘v8::Value::ToObject()’ 48 | v8::Local options = info[1]->ToObject(); | ^ In file included from /home/mfadel/.cache/node-gyp/13.13.0/include/node/node.h:67, from ../../../../nan/nan.h:54, from ../src/./serialport.h:6, from ../src/serialport.cpp:1: /home/mfadel/.cache/node-gyp/13.13.0/include/node/v8.h:2754:44: note: candidate: ‘v8::MaybeLocal v8::Value::ToObject(v8::Local) const’ 2754 | V8_WARN_UNUSED_RESULT MaybeLocal

LaserWeb / LaserWeb4