Revamp makefile #1402

tharvik · 2023-10-04T15:57:21Z

Changes

Makefile is gone out of sync since at least #1173 so I updated it (a bunch).

rewrite it in a single, more standard makefile
forward most commands to npm
support parallel use of it (make -j get_images is nice to have)
get rid of load-time auth check and trust user
rework targets
- prep_commit -> format, lint
- dev, beta, release -> build-dev, build-prod, build-tests: match closer to npm
- prep_staging, package: now uneeded
update devguide accordingly
- nicer split between vscode and CLI
~~package.json: also remove unworking target~~ it was needed
npm run lint now runs every linter
bsconfig-tests create a pkg to have make build-tests behaving the same as the other build-* targets

Fixes

#1401

cewert · 2023-10-04T19:31:00Z

Thanks for the PR! Going to give the code a quick look and I'll give it a test when I can.

package.json

DEVGUIDE.md

tharvik · 2023-10-05T01:05:08Z

arf, I force pushed the branch to remove the package.json commit, sorry about that: I wanted to avoid having a revert commit but didn't knew the policy there.

while at it, for bsconfig-tests.json, why does it actively doesn't create a package? running the suite via make build-tests install doesn't work as I wanted to

cewert · 2023-10-06T01:09:46Z

arf, I force pushed the branch to remove the package.json commit, sorry about that: I wanted to avoid having a revert commit but didn't knew the policy there.

Good question. I don't think we have a policy on that. It's fine :)

while at it, for bsconfig-tests.json, why does it actively doesn't create a package? running the suite via make build-tests install doesn't work as I wanted to

It's currently setup so that the vscode brightscript extension creates and deploys the testing app (the unit tests have to be ran on the actual device). You can change it to make a zip by setting the createPackage var to true here.

DEVGUIDE.md

tharvik · 2023-10-11T08:24:45Z

so I reworked it more, maybe its overeaching, tell me what you think

we now have only two methods, one VScode, one CLI, the latter using make. a single tool is easier than mixing npm and make IMO and some calls can only be made via make (install, key presses, screenshot, …)
"Deploy" and "Bug/Crash Reports" has been moved to "Method 2" as theses were CLI specific
- if you want to had some precision for the VScode part, please do, I don't know the first thing about it
"Committing" is kept at the ~top level and using npm as you suggested
"Update Images" reference "Method 2" but kept as top level, as I don't think there is a way to do it via VScode (maybe?)

so now, there is a clearer separation of how to dev with each tool: debugging via VScode or use the CLI and telnet; deploying? press run on VScode or run make install on CLI

changing npm run lint to run all of theses.

I didn't touched the .github/workflows/lint.yml but it can be easily rewritten to just run lint now (excepted for xml checks maybe?).

while at it, for bsconfig-tests.json, why does it actively doesn't create a package? running the suite via make build-tests install doesn't work as I wanted to

You can change it to make a zip by setting the createPackage var to true here.

perfect! added a commit to generate the pkg (by removing the line thus using the default as in the other bsconfig) :)

tharvik · 2023-10-11T08:52:35Z

btw, I'm using git commit --fixup to allow for easier rebasing w/ autosquash, to not discard the comments and make a nice history.

jellyfin-bot · 2023-10-15T23:13:18Z

This pull request has merge conflicts. Please resolve the conflicts so the PR can be reviewed. Thanks!

jellyfin-bot · 2023-10-15T23:13:19Z

This pull request has merge conflicts. Please resolve the conflicts so the PR can be reviewed. Thanks!

cewert · 2023-10-15T23:19:33Z

Sorry I created a couple merge conflicts. If you have any questions or you just want me to do it give me a ping.

tharvik · 2023-10-16T20:50:58Z

Sorry I created a couple merge conflicts.

no worries, it was easy :)

cewert · 2023-10-16T23:04:06Z

so I reworked it more, maybe its overeaching, tell me what you think

* we now have only two methods, one VScode, one CLI, the latter using `make`. a single tool is easier than mixing `npm` and `make` IMO and some calls can only be made via `make` (`install`, key presses, `screenshot`, …)

* "Deploy" and "Bug/Crash Reports" has been moved to "Method 2" as theses were CLI specific
  
  * if you want to had some precision for the VScode part, please do, I don't know the first thing about it

* "Committing" is kept at the ~top level and using `npm` as you suggested

* "Update Images" reference "Method 2" but kept as top level, as I don't think there is a way to do it via VScode (maybe?)

I think you did a great job 👍 I was a little bummed at first to see method 2 get cut but it's essentially the same thing as the Beta Test instructions on the readme and since the file is called DEVGUIDE and is meant to be more advanced deployment methods, I think it makes perfect sense to remove it.

Update Images hasn't been needed in years but maybe it should be under Method 2 since thats the only method that supports it? I don't think it's a big deal either way just seems odd to have everything organized except that one thing.

changing npm run lint to run all of theses.

I didn't touched the .github/workflows/lint.yml but it can be easily rewritten to just run lint now (excepted for xml checks maybe?).

I'm glad you didn't 😄 Technically we could change the workflows to use npm run lint but the overall UX would suffer. You have all the commands chained together with && (which is fine) but if the first command fails then the rest aren't run. So if your code made every lint command fail, you wouldn't know it or be able to fix them all. You'd have to fix the first error, rerun lint, fix error, rerun lint etc. Also, if they were all together, you wouldn't be able to see which lint command produced the error from the PR GUI. You'd have to open up the job log to see which command produced the error.

We will have to update the workflow though since you changed the command that lints brightscript.

There was a couple little things I saw in the devguide I'll leave comments for soon

cewert · 2023-10-17T15:25:58Z

I tried to test this using WSL with ubuntu 22.04 but that didn't work because sudo apt install npm gave me nodejs version 10 which made bsfmt fail because one of it's dependencies needs nodejs version 12 or higher. I'll have to figure something else out for testing.

> npx bsfmt --check

/mnt/c/Users/Charlie/Documents/PROJECTS/tharvik/node_modules/yargs/node_modules/yargs-parser/build/index.cjs:1013
        throw Error(`yargs parser supports a minimum Node.js version of ${minNodeVersion}. Read our version support policy: https://github.com/yargs/yargs-parser#supported-nodejs-versions`);
        ^

Error: yargs parser supports a minimum Node.js version of 12. Read our version support policy: https://github.com/yargs/yargs-parser#supported-nodejs-versions

tharvik · 2023-10-17T18:28:05Z

I tried to test this using WSL with ubuntu 22.04 but that didn't work because sudo apt install npm gave me nodejs version 10

hum, ubuntu 22.04 nodejs pkg is version 12, maybe you simply need to apt update && apt full-upgrade?

cewert · 2023-10-18T00:45:37Z

I had originally installed ubuntu 22 using chocolaty (or some other windows package manager) so apparently that was my issue because after I reinstalled ubuntu 22 using the windows store I got nodejs v12 when i installed npm.

That being said, is npm the right package to install? After installing npm I ran npm install in WSL and I got a bunch of warnings about dependencies needing a higher version of node:

charlie@DESKTOP-23CQ5C2:/mnt/c/Users/Charlie/Documents/PROJECTS/tharvik$ npm install
npm WARN EBADENGINE Unsupported engine {
npm WARN EBADENGINE   package: '[email protected]',
npm WARN EBADENGINE   required: { node: '>=14' },
npm WARN EBADENGINE   current: { node: 'v12.22.9', npm: '8.5.1' }
npm WARN EBADENGINE }
npm WARN EBADENGINE Unsupported engine {
npm WARN EBADENGINE   package: '[email protected]',
npm WARN EBADENGINE   required: { node: '>=14' },
npm WARN EBADENGINE   current: { node: 'v12.22.9', npm: '8.5.1' }
npm WARN EBADENGINE }
npm WARN EBADENGINE Unsupported engine {
npm WARN EBADENGINE   package: '[email protected]',
npm WARN EBADENGINE   required: { node: '>=14' },
npm WARN EBADENGINE   current: { node: 'v12.22.9', npm: '8.5.1' }
npm WARN EBADENGINE }
npm WARN EBADENGINE Unsupported engine {
npm WARN EBADENGINE   package: '[email protected]',
npm WARN EBADENGINE   required: { node: '^14.13.1 || >=16.0.0' },
npm WARN EBADENGINE   current: { node: 'v12.22.9', npm: '8.5.1' }
npm WARN EBADENGINE }
npm WARN EBADENGINE Unsupported engine {
npm WARN EBADENGINE   package: '[email protected]',
npm WARN EBADENGINE   required: { node: '>=14' },
npm WARN EBADENGINE   current: { node: 'v12.22.9', npm: '8.5.1' }
npm WARN EBADENGINE }
npm WARN EBADENGINE Unsupported engine {
npm WARN EBADENGINE   package: '[email protected]',
npm WARN EBADENGINE   required: { node: '>=16' },
npm WARN EBADENGINE   current: { node: 'v12.22.9', npm: '8.5.1' }
npm WARN EBADENGINE }
npm WARN EBADENGINE Unsupported engine {
npm WARN EBADENGINE   package: '[email protected]',
npm WARN EBADENGINE   required: { node: '>=16' },
npm WARN EBADENGINE   current: { node: 'v12.22.9', npm: '8.5.1' }
npm WARN EBADENGINE }
npm WARN EBADENGINE Unsupported engine {
npm WARN EBADENGINE   package: '[email protected]',
npm WARN EBADENGINE   required: { node: '>=14.16' },
npm WARN EBADENGINE   current: { node: 'v12.22.9', npm: '8.5.1' }
npm WARN EBADENGINE }
npm WARN EBADENGINE Unsupported engine {
npm WARN EBADENGINE   package: '[email protected]',
npm WARN EBADENGINE   required: { node: '>=16' },
npm WARN EBADENGINE   current: { node: 'v12.22.9', npm: '8.5.1' }
npm WARN EBADENGINE }
npm WARN EBADENGINE Unsupported engine {
npm WARN EBADENGINE   package: '[email protected]',
npm WARN EBADENGINE   required: { node: '>=16 || 14 >=14.17' },
npm WARN EBADENGINE   current: { node: 'v12.22.9', npm: '8.5.1' }
npm WARN EBADENGINE }
npm WARN EBADENGINE Unsupported engine {
npm WARN EBADENGINE   package: '[email protected]',
npm WARN EBADENGINE   required: { node: '>=16 || 14 >=14.17' },
npm WARN EBADENGINE   current: { node: 'v12.22.9', npm: '8.5.1' }
npm WARN EBADENGINE }
npm WARN EBADENGINE Unsupported engine {
npm WARN EBADENGINE   package: '[email protected]',
npm WARN EBADENGINE   required: { node: '14 || >=16.14' },
npm WARN EBADENGINE   current: { node: 'v12.22.9', npm: '8.5.1' }
npm WARN EBADENGINE }
npm WARN EBADENGINE Unsupported engine {
npm WARN EBADENGINE   package: '[email protected]',
npm WARN EBADENGINE   required: { node: '>=14' },
npm WARN EBADENGINE   current: { node: 'v12.22.9', npm: '8.5.1' }
npm WARN EBADENGINE }
npm WARN EBADENGINE Unsupported engine {
npm WARN EBADENGINE   package: '[email protected]',
npm WARN EBADENGINE   required: { node: '>=16 || 14 >=14.17' },
npm WARN EBADENGINE   current: { node: 'v12.22.9', npm: '8.5.1' }
npm WARN EBADENGINE }
npm WARN EBADENGINE Unsupported engine {
npm WARN EBADENGINE   package: '[email protected]',
npm WARN EBADENGINE   required: { node: '>=16 || 14 >=14.17' },
npm WARN EBADENGINE   current: { node: 'v12.22.9', npm: '8.5.1' }
npm WARN EBADENGINE }
npm WARN EBADENGINE Unsupported engine {
npm WARN EBADENGINE   package: '[email protected]',
npm WARN EBADENGINE   required: { node: '>=14' },
npm WARN EBADENGINE   current: { node: 'v12.22.9', npm: '8.5.1' }
npm WARN EBADENGINE }
npm WARN EBADENGINE Unsupported engine {
npm WARN EBADENGINE   package: '[email protected]',
npm WARN EBADENGINE   required: { node: '>= 14' },
npm WARN EBADENGINE   current: { node: 'v12.22.9', npm: '8.5.1' }
npm WARN EBADENGINE }

> [email protected] postinstall
> npm run ropm
...

I know it's possible for me to manually install a higher version but shouldn't the docs give instructions to install a higher version of node? Looks like we'd need at least v14.17.

EDIT: Here's the instructions to install the latest versions of nodejs https://github.com/nodesource/distributions/blob/master/README.md#debian-and-ubuntu-based-distributions

cewert · 2023-10-18T01:45:21Z

The latest LTS releases for nodejs that are still receiving updates are 18 and 20. Support for 16 just ended a month ago. If we end up copying all the instructions for debian we might as well recommend one of the newer versions. Otherwise we'll just run into the same problem with dependencies when they eventually stop supporting v14

That's crazy that the 22.04 LTS version of ubuntu would choose to use nodejs v12. Ubuntu 22.04 came out April 21st 2022 and is supported for 5 years while nodejs v12 stopped receiving updates 9 days later on April 30th 2022 🤔

tharvik · 2023-10-23T13:34:40Z

That's crazy that the 22.04 LTS version of ubuntu would choose to use nodejs v12. Ubuntu 22.04 came out April 21st 2022 and is supported for 5 years while nodejs v12 stopped receiving updates 9 days later on April 30th 2022 🤔

yeah, the Ubuntu LTS are not really following upstream's LTS as their developpement cycles and length of LTS doesn't always match. they even ended up supporting a non-LTS kernel some years ago: that was cumbersome to say the least.

The latest LTS releases for nodejs that are still receiving updates are 18 and 20. Support for 16 just ended a month ago. If we end up copying all the instructions for debian we might as well recommend one of the newer versions. Otherwise we'll just run into the same problem with dependencies when they eventually stop supporting v14

I'm afraid we are falling into the system-specific rabbit hole now: there is two package manager on Ubuntu now, apt and snap, the first is slowing getting dropped there but not on the Debian side. I can add instruction on how to add deps via apt (mostly a copypaste of https://deb.nodesource.com) which fits both for now but it will probably drift slowly. IMO, we can simply state that one need to have nodejs >= 16, get rid of the debian-based blocks and that should do the trick; what do you prefer?

cewert · 2023-10-25T16:49:19Z

IMO, we can simply state that one need to have nodejs >= 16, get rid of the debian-based blocks and that should do the trick; what do you prefer?

That works for me 👍

cewert · 2023-10-25T23:18:04Z

The docs say to run make build-dev, then if you want to install it on the device to run make install but make install is also running make build-dev so we end up building twice. Seems like we could use a make deploy that only deploys the app without building it, yea?

I'm not sure where this would fit best, but it would be nice to have a list of supported make commands somewhere to have as a quick reference.

PS: I'm still using WSL to test. I don't know why but using WSL makes the npm run commands take forever. make lint took somewhere between 10 and 20 minutes. I don't exactly know because I had to go afk it took so long.

Makefile

jellyfin-bot · 2023-10-27T02:20:23Z

This pull request has merge conflicts. Please resolve the conflicts so the PR can be reviewed. Thanks!

tharvik · 2023-10-30T13:52:53Z

The docs say to run make build-dev, then if you want to install it on the device to run make install but make install is also running make build-dev so we end up building twice.

it doesn't build it twice on my system (Linux w/ GNU Make 4.4.1), neither should it. the package is build only if it doesn't exists already (defaulting to build-dev in this case). that allows for users to build-{dev,prod,tests} at their will.

I know of some issues with old FAT filesystem (it does have timestamp with a 2-second resolution which might confuse it on rapidly firing make), but I don't think anyone is building code directly on it.

I'm not sure where this would fit best, but it would be nice to have a list of supported make commands somewhere to have as a quick reference.

adding a help target listing theses.

This pull request has merge conflicts. Please resolve the conflicts so the PR can be reviewed. Thanks!

and there goes the rebase.

cewert · 2023-10-30T16:30:53Z

Can we show a more helpful error message when the env vars are missing? This is the error I got when running make install without setting my env vars first:

charlie@DESKTOP-23CQ5C2:/mnt/c/Users/Charlie/Documents/PROJECTS/tharvik$ make install
make: *** No rule to make target 'install'.  Stop.

Also, can we show the full list of commands when running make help? I think it's clever to tell the user which commands are linked to the env vars but I still think we should display them all.

When running make install can we still launch the app even if it's identical? make install normally launches the app except when deploying an identical package

charlie@DESKTOP-23CQ5C2:/mnt/c/Users/Charlie/Documents/PROJECTS/tharvik$ make install
curl --show-error -XPOST http://192.168.1.177:8060/keypress/home
sleep 2 # wait for roku reaction
curl --show-error --user rokudev:pw --digest -F "mysubmit=Install" -F "archive=@out/tharvik.zip" -F "passwd=" http://192.168.1.177/plugin_install | grep "<font color" | sed "s/<font color=\"red\">//" | sed "s[</font>[["
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
  0 2184k    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
100 2189k    0  4939  100 2184k   5415  2395k --:--:-- --:--:-- --:--:-- 1461k
    Application Received: Identical to previous version -- not replacing.

* rewrite it in a single, more standard makefile * forward most commands to npm * support parallel use * get rid of load-time auth check and trust user * rework targets * `prep_commit` -> `format`, `lint` * `dev`, `beta`, `release` -> `build-dev`, `build-prod`, `build-tests` * `prep_staging`, `package`: rm * update devguide accordingly Fixes: jellyfin#1401

tharvik · 2023-10-31T15:32:23Z

Can we show a more helpful error message when the env vars are missing?

we could but that makes the Makefile way less readable as we would need to redefine the targets in every ifdef path combination. having the "help" target looks enough for me. IMO one would run it when something doesn't work as expected.

Also, can we show the full list of commands when running make help?

updated to show each target ~group, and document the needed environment, unconditionnaly.

When running make install can we still launch the app even if it's identical? make install normally launches the app except when deploying an identical package

added a "launch" target to do that, and running it at the end of "install".

tharvik requested a review from a team as a code owner October 4, 2023 15:57

tharvik force-pushed the fix-makefile branch from 89b68c7 to 9f27eb6 Compare October 4, 2023 16:22

cewert requested changes Oct 4, 2023

View reviewed changes

package.json Outdated Show resolved Hide resolved

cewert reviewed Oct 4, 2023

View reviewed changes