Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

luci-app-p910nd: convert to JS #6907

Merged
merged 1 commit into from
Mar 12, 2024
Merged

luci-app-p910nd: convert to JS #6907

merged 1 commit into from
Mar 12, 2024

Conversation

systemcrash
Copy link
Contributor

@systemcrash systemcrash commented Feb 13, 2024 •
edited
Loading

Simple p910nd printer management, and some level of automation that detects whether the typically necessary kmods are installed, and if not provides quick links to install those.

This effectively replaces the old LUA version.

Feedback welcome - there may be some slightly better ways of doing things like opkg processes and the NamedSection.

Tested on: 21.02.1, 22.03.6, 23.05.2

Screenshot 2024-02-13 at 18 31 34

Screenshot 2024-02-13 at 18 31 21

Screenshot 2024-02-13 at 18 30 42

  • This PR is not from my main or master branch 💩, but a separate branch ✅
  • Each commit has a valid ✒️ Signed-off-by: <my@email.address> row (via git commit --signoff)
  • Each commit and PR title has a valid 📝 <package name>: title first line subject for packages
  • Tested on: (architecture, openwrt version, browser) ✅
  • ( Preferred ) Screenshot or mp4 of changes:
  • Description: (describe the changes proposed in this PR)

@systemcrash systemcrash force-pushed the p910nd_to_js_2024 branch 3 times, most recently from 3028388 to 3a60c6c Compare February 17, 2024 17:02
jow-
jow- reviewed Feb 18, 2024
View reviewed changes
applications/luci-app-p910nd/root/usr/share/rpcd/acl.d/luci-app-p910nd.json Outdated
"uci": [ "p910nd" ]
"uci": [ "p910nd" ],
"file": {
"/usr/bin/find -L /dev -maxdepth 3 -type c*": ["exec"],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please change the command pattern to exactly:

/usr/bin/find -L /dev -maxdepth 3 -type c -name lp[0-9]

This will prevent unwanted parameter injection.

Copy link
Contributor Author

@systemcrash systemcrash Feb 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing it to exactly /usr/bin/find -L /dev -maxdepth 3 -type c -name lp[0-9] causes 403 when issuing the command via cgi-bin, and Error executing "find" command: Access to command denied by ACL.

Changing it to exactly /usr/bin/find -L /dev -maxdepth 3 -type c -name lp\[0-9\] causes 403 and the page does not even load (prevented by 403).

I tried many permutations and could not find a specific one that worked. ( All variants work find at the command line, however. )

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As an alternative we may add a script to find drivers and call it. rpcd may also work but I'm not sure if it needed if no one else will ever call it.

@systemcrash systemcrash force-pushed the p910nd_to_js_2024 branch 2 times, most recently from 6113aad to dd24f51 Compare March 2, 2024 00:07
stokito
stokito reviewed Mar 2, 2024
View reviewed changes
Copy link
Contributor

@stokito stokito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add yourself as a maintainer

@systemcrash systemcrash force-pushed the p910nd_to_js_2024 branch 2 times, most recently from 4cb6655 to a733732 Compare March 2, 2024 02:48
@systemcrash systemcrash force-pushed the p910nd_to_js_2024 branch 3 times, most recently from d9d94b3 to 35352f2 Compare March 7, 2024 00:57
@systemcrash
luci-app-p910nd: convert to JS
7053272 
Simple p910nd printer management, and some level of automation that
detects whether the typically necessary kmods are installed, and if not
provides quick links to install those.

Tested on: 22.03.6, 23.05.2

Signed-off-by: Paul Donald <newtwen@gmail.com>
@systemcrash systemcrash merged commit 7053272 into openwrt:master Mar 12, 2024
5 checks passed
@systemcrash systemcrash deleted the p910nd_to_js_2024 branch April 4, 2024 21:50
@pkgadd
Copy link

pkgadd commented Nov 21, 2024

Hi @systemcrash

While this did the conversion from lua to JS, it didn't drop the now superfluous(?) dependency on luci-compat from applications/luci-app-p910nd/Makefile. A very quick review and superficial test suggests that luci-app-p910nd now does work fine on a system without luaand luci-compat, so the dependency can just be dropped .

Disclaimer: I only (successfully) went through the luci menus for p910nd and couldn't actually test it with a printer, yet (I still need luci-compat on my production routers for ddns).

--- a/applications/luci-app-p910nd/Makefile
+++ b/applications/luci-app-p910nd/Makefile
@@ -9,3 +9,3 @@ include $(TOPDIR)/rules.mk
 LUCI_TITLE:=p910nd - Printer server module
-LUCI_DEPENDS:=+luci-base +luci-compat +p910nd
+LUCI_DEPENDS:=+luci-base +p910nd

@systemcrash
Copy link
Contributor Author

Well spotted. Fix is in.

I have you covered with the ddns thing. I've just finished converting it to ucode, update is in master/24/23.

@pkgadd
Copy link

pkgadd commented Nov 21, 2024 •
edited
Loading

Wow, that's great - seems to work, no lua anymore (after giving the luci-app-ddns Makefile the same dependency treatment) :)

--- a/applications/luci-app-ddns/Makefile
+++ b/applications/luci-app-ddns/Makefile
@@ -14,3 +14,3 @@ PKG_MAINTAINER:=Ansuel Smith <ansuelsmth@gmail.com>
 LUCI_TITLE:=LuCI Support for Dynamic DNS Client (ddns-scripts)
-LUCI_DEPENDS:=+luci-base +luci-lua-runtime +ddns-scripts
+LUCI_DEPENDS:=+luci-base +ddns-scripts
 

$ grep -v -e ^\# .config -e i18n | grep -i -e lua -e luci
CONFIG_FEED_luci=y
CONFIG_LUA_ECO_MBEDTLS=y
CONFIG_HAS_LUAJIT_ARCH=y
CONFIG_PACKAGE_liblucihttp=m
CONFIG_PACKAGE_liblucihttp-ucode=m
CONFIG_PACKAGE_rpcd-mod-luci=m
CONFIG_PACKAGE_luci=m
CONFIG_PACKAGE_luci-light=m
CONFIG_PACKAGE_luci-ssl=m
CONFIG_PACKAGE_luci-ssl-openssl=m
CONFIG_PACKAGE_luci-base=m
CONFIG_LUCI_JSMIN=y
CONFIG_LUCI_CSSTIDY=y
CONFIG_LUCI_LANG_de=m
CONFIG_PACKAGE_luci-mod-admin-full=m
CONFIG_PACKAGE_luci-mod-network=m
CONFIG_PACKAGE_luci-mod-status=m
CONFIG_PACKAGE_luci-mod-system=m
CONFIG_PACKAGE_luci-app-adblock=m
CONFIG_PACKAGE_luci-app-banip=m
CONFIG_PACKAGE_luci-app-bcp38=m
CONFIG_PACKAGE_luci-app-ddns=m
CONFIG_PACKAGE_luci-app-firewall=m
CONFIG_PACKAGE_luci-app-nlbwmon=m
CONFIG_PACKAGE_luci-app-p910nd=m
CONFIG_PACKAGE_luci-app-package-manager=m
CONFIG_PACKAGE_luci-app-sqm=m
CONFIG_PACKAGE_luci-app-statistics=m
CONFIG_PACKAGE_luci-app-uhttpd=m
CONFIG_PACKAGE_luci-app-wol=m
CONFIG_PACKAGE_luci-theme-bootstrap=m
CONFIG_PACKAGE_luci-theme-openwrt-2020=m
CONFIG_PACKAGE_luci-proto-ipv6=m
CONFIG_PACKAGE_luci-proto-ppp=m
CONFIG_PACKAGE_luci-proto-wireguard=m
CONFIG_PACKAGE_luci-lib-chartjs=m
CONFIG_PACKAGE_luci-lib-uqr=m

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jow- jow- jow- left review comments

@stokito stokito stokito approved these changes

@yurtpage yurtpage yurtpage approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@systemcrash @pkgadd @stokito @jow- @yurtpage