Build instructions should document requirements

[robertlipe]

I'm making this a bugreport because it'll probably take a few of us (esp. those that are multi-OS and have ability to make blank OS images for testing...) to produce a list of requirements.

Bug report

New developers, following our instructions, can run into errors like this:

file:///NightDriverStrip-yulc/site/node_modules/vite/bin/vite.js:7 await import('source-map-support').then((r) => r.default.install()) ^^^^^

SyntaxError: Unexpected reserved word at Loader.moduleStrategy (internal/modules/esm/translators.js:133:18) at async link (internal/modules/esm/module_job.js:42:21)

Problem

Steps

1. Follow instructions.
2. Observe build failure.
3. Become sad. Start drinking.

Example We should probably have a section of doc that lists requirements for installing

• Platformio
• (Does ESP-IDF have to be separately installed?)
• Vite (any other npm kind of stuff?)

Notes For MacOS, I know 'brew install vite' is the (a) solution to above. Ideally, we'd have copy-pastable: Linux: yum install platformio vite Windows: whatever MacOS: recommend the excellent homebrew. $ brew install vite Site for platformio, etc.

[rbergen]

I think the main thing is having a sufficiently current version of NodeJS and NPM installed (the latter being included with sufficiently current versions of the former). And yes, it does seem the Gods of NodeJS have, once again, changed the rules for doing that in a way that makes it mildly less obvious than it used to be, in recent-ish times.

To be frank, I think this issue can be quite quickly addressed by a relatively modest change to the build tooling part of the README - more concretely by adding a reference to nodejs.org, and maybe urging the reader to follow the installation instructions published there with some care.

[robertlipe]

I'll admit I don't understand (and this does not make me sad) the relationship between nodejs, npm, and vink to know why it's jumping the rails and how to get it back on. I've tried to coach three new devs aboard this week and the two on Linux have been unable to build because they crash into this error. I went to see if it was documented and I realized we didn't have a "requirements" section and thought we should probably kill more birds with the same stone.

If there's some way to make it automatically fetch vink (like platformio already transparently fetches the kitchen sink - 14 times - for the C++ code) and a dev not have to know/care, even better.

Since the README is what people see when they land on the GitHub page, that's a lovely spot for it.

I honestly can't remember how I installed it myself even on MacOS. If that meant I was going to have to burn an hour installing a fresh VM and documenting the process, I was willing to take one for the team, but that doesn't immediately help the non-Mac devs.)

On Fri, Jul 12, 2024 at 2:36 PM Rutger van Bergen @.***> wrote:

I think the main thing is having a sufficiently current version of NodeJS and NPM installed (the latter being included with sufficiently current versions of the former). And yes, it does seem the Gods of NodeJS have, once again, changed the rules for doing that in a way that makes it mildly less obvious than it used to be, in recent-ish times.

To be frank, I think this issue can be quite quickly addressed by a relatively modest change to the build tooling part of the README - more concretely by adding a reference to nodejs.org, and maybe urging the reader to follow the installation instructions published there with some care.

— Reply to this email directly, view it on GitHub https://github.com/PlummersSoftwareLLC/NightDriverStrip/issues/644#issuecomment-2226240798, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACCSD36Y232VD3LXPJU7DR3ZMAV5TAVCNFSM6AAAAABKZK65JWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMRWGI2DANZZHA . You are receiving this because you authored the thread.Message ID: @.***>

[rbergen]

One of the extremely annoying things about NodeJS is that the "current version" thing tends to be mission-critical, and the versions that get installed by "OS'es own" package managers (and I'll put Homebrew in that category, knowing full well that's not technically correct) tend to be far from current. So you'll end up with NodeJS without NPM, or versions of either/both that don't contain the "automatic dependency installation" cleverness that's only available in later versions.

That stuff can be maddening. So again, I agree making mention of this in the README would be a good idea, but I am also not sure how much hair-pulling it would actually prevent. One really has to read such a mention with small print attention to not just (logically!) go "oh yeah, so I need NodeJS", run a "sudo apt install nodejs" and end up with a NodeJS version that works... but actually doesn't.

[robertlipe]

Coming from the view of a systems software dev of many decades, web dev is just so terrible...

Still, both of my partners got their environments going without further help from me and without really responding to the "how could we make this better", so maybe it's all self-evident enough that once someone hits the bump, they'll figure it out.

If you think there's really nothing better we can do here, let's close this and move on.

Thanx for at least talking it through.

On Fri, Jul 12, 2024 at 6:24 PM Rutger van Bergen @.***> wrote:

One of the extremely annoying things about NodeJS is that the "current version" thing tends to be mission-critical, and the versions that get installed by "OS'es own" package managers (and I'll put Homebrew in that category, knowing full well that's not technically correct) tend to be far from current. So you'll end up with NodeJS without NPM, or versions of either/both that don't contain the "automatic dependency installation" cleverness that's only available in later versions.

That stuff can be maddening. So again, I agree making mention of this in the README would be a good idea, but I am also not sure how much hair-pulling it would actually prevent. One really has to read such a mention with small print attention to not just (logically!) go "oh yeah, so I need NodeJS", run a "sudo apt install nodejs" and end up with a NodeJS version that works... but actually doesn't.

— Reply to this email directly, view it on GitHub https://github.com/PlummersSoftwareLLC/NightDriverStrip/issues/644#issuecomment-2226519137, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACCSD34NXEUNKC24VWHGUQLZMBQTRAVCNFSM6AAAAABKZK65JWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMRWGUYTSMJTG4 . You are receiving this because you authored the thread.Message ID: @.***>

[rbergen]

I'll give it some more thought this weekend. I have a PR open that I intend to merge this weekend anyway, so I can easily add an update to README.md if I have a concrete piece of text to add/modify. Maybe it'll come to me, maybe it won't.

[rbergen]

I've updated the Build tools section in the README in relation to this in PR #643. I'll allow any interested reviewers another day to develop and voice an opinion on that PR, after which I will merge it.

[robertlipe]

Confirmed 'good enough' with my guys that ran ashore on this. They're all happily building (local branches of) NightDriverLED now.

Thank you for your attention. Please feel free to close at will.

On Mon, Jul 15, 2024 at 11:57 AM Rutger van Bergen @.***> wrote:

I've updated the Build tools section in the README in relation to this in PR #643 https://github.com/PlummersSoftwareLLC/NightDriverStrip/pull/643. I'll allow any interested reviewers another day to develop and voice an opinion on that PR, after which I will merge it.

— Reply to this email directly, view it on GitHub https://github.com/PlummersSoftwareLLC/NightDriverStrip/issues/644#issuecomment-2228971073, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACCSD3ZI6H4O2UTBAMIPUX3ZMP5PPAVCNFSM6AAAAABKZK65JWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMRYHE3TCMBXGM . You are receiving this because you authored the thread.Message ID: @.***>

[rbergen]

Thanks for the feedback! I've linked the PR to this issue, so the latter will get closed automatically when the former gets merged.