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.

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

luci-base: move some generic classes into a separate luci-base-libs package #2817

Closed

Conversation

SvenRoederer
Copy link
Contributor

@SvenRoederer SvenRoederer commented Jul 3, 2019

The new package luci-base-libs provides the modules that not strictly relate to the web-interface of luci. By separating these libs they can be used by other packages without having to install the web-components.
This change was inspired by providing a shell-only interface for 4MB-flash devices, by keeping as much code common with a full install. Dropping the luci-web stuff reduces the package by 100kb.

@SvenRoederer
Copy link
Contributor Author

SvenRoederer commented Jul 3, 2019

a later version of this code should not have a separate Makefile, the luci-base packge should be modified to generate both packages.

Edit: not sure about this idea

@feckert feckert added the WIP pull request the author is still working on label Sep 22, 2019
@SvenRoederer
Copy link
Contributor Author

@feckert just seen that you marked it with WIP-flag and not gave it a NACK yet. So I'll continue to work on this.

@SvenRoederer SvenRoederer force-pushed the package_luci-base-libs branch 2 times, most recently from da1cb64 to 9890b30 Compare October 19, 2019 18:24
@SvenRoederer SvenRoederer changed the title [WIP] luci-base: move some generic classes into a separate luci-base-libs package luci-base: move some generic classes into a separate luci-base-libs package Oct 19, 2019
@SvenRoederer
Copy link
Contributor Author

I pushed the missing changes to finish the integration of the rearranged libraries. I ran a test based on "luci-mod-admin-full".

@feckert can you review?

@feckert feckert added feature pull request adding a new feature and removed WIP pull request the author is still working on labels Oct 19, 2019
SvenRoederer added a commit to freifunk-berlin/firmware that referenced this pull request Oct 20, 2019
this is a squashed version of openwrt/luci#2817
SvenRoederer added a commit to freifunk-berlin/firmware that referenced this pull request Oct 20, 2019
this is a squashed version of openwrt/luci#2817
SvenRoederer added a commit to freifunk-berlin/firmware that referenced this pull request Oct 21, 2019
This is a backport of openwrt/luci#2817 for
OpenWrt-19.07
SvenRoederer added a commit to freifunk-berlin/firmware that referenced this pull request Oct 25, 2019
This is a backport of openwrt/luci#2817 for
OpenWrt-19.07
SvenRoederer added a commit to freifunk-berlin/firmware that referenced this pull request Oct 25, 2019
This is a backport of openwrt/luci#2817 for
OpenWrt-19.07
@SvenRoederer
Copy link
Contributor Author

Just rebased to recent master. Any other issues preventing from merging?

@SvenRoederer
Copy link
Contributor Author

@jow- @feckert any opinions on this change?
We are using it for some months in our freifunk firmware and were able to improve the support for 4MB devices.

@feckert
Copy link
Member

feckert commented Nov 27, 2019

@SvenRoederer from my side ok, but that's up to @jow- if he wants to merge this.

A little question then. If you don't need the LuCI you have still include the whole LuCI feed repo during compilation and build. Doesn't it make then not more sense to move it into the package feed?

@SvenRoederer
Copy link
Contributor Author

our package luci-app-owm-cmd (https://github.com/freifunk-berlin/firmware-packages/blob/master/utils/luci-app-owm) is using the luci-lib-httpclient package which pulls in luci-base. The 4MB devices we use w/o web-interface and don't need uhttpd and the web-framework. As luci-app-owm-cmd is just a cmdline-app to report some data, we like to use it on these devices, which will pull also the full web-framework as of the mentioned dependency.
Solving this luci-app-owm-cmd-dependency by moving some packages to OpenWrt-packages would recursively require a bunch of packages to be moved. As our images for other routers will include the full LuCI, we need the luci-feed anyways.

@SvenRoederer
Copy link
Contributor Author

@jow- What do you think on this?

@SvenRoederer
Copy link
Contributor Author

@jow- just seen that your commit cf1219b caused conflict of my PR.
What do you think of the intention of this PR?

@SvenRoederer
Copy link
Contributor Author

@jow- ping ... I would be happy to get your feedback on this idea.

@SvenRoederer
Copy link
Contributor Author

@jow- @NeoRaider @hnyman as @feckert mentioned above (#2817 (comment)) this PR looks fine.
What do you think on merging?

@jow-
Copy link
Contributor

jow- commented Mar 27, 2020

Did you actually runtime test all LuCI views with this change? I do see that dispatcher.lua still references util.striptags and util.pcdata in the tpl.context.viewns meta table while these functions were moved to a new XML class. That will likely break rendering a bunch of templates.

@SvenRoederer
Copy link
Contributor Author

This was introduced when rebasing from 83408a1 to ce8fb70. Pushing an updated version which also updates the docs.

@neocturne
Copy link
Member

@SvenRoederer I'm really not the right person to ping about LuCI packaging - I personally don't use LuCI, and Gluon doesn't even include the feed anymore.

SvenRoederer added a commit to SvenRoederer/freifunk-berlin-firmware that referenced this pull request May 27, 2020
SvenRoederer added a commit to SvenRoederer/freifunk-berlin-firmware that referenced this pull request May 27, 2020
SvenRoederer added a commit to SvenRoederer/freifunk-berlin-firmware that referenced this pull request May 28, 2020
SvenRoederer added a commit to SvenRoederer/freifunk-berlin-firmware that referenced this pull request May 28, 2020
@SvenRoederer
Copy link
Contributor Author

@jow- @hnyman @feckert Any feedback on this would be great.

@feckert
Copy link
Member

feckert commented Jun 15, 2020

@SvenRoederer Sorry I can't help you there either!
This was created by @jow-.
He has the last word on that.

@jow-
Copy link
Contributor

jow- commented Jun 15, 2020

I don't think we should move pcdata() to a new luci.xml class. This has the potential to break a lot 3rd party out of tree code.

@SvenRoederer
Copy link
Contributor Author

As mentioned this change was driven by keeping some commandline tools working on 4MB devices which don't provide a web-ui (no uhhtp, no luci web-framework).
I don't remember the exact details anymore, but it turned out that one of the libs in the new package depends on the striptags() and or the pcdata() function of luci.utils. But luci.utils depends on the luci.dispatcher and its dependencies. This way half of the stuff, required for the webframework ended up in the new "small" package. As this mixup of having web-framework related thing in the stripped down "non-web" package, which did not made sense at all.
So the most clean solution I found, was to refactor the functions into this new module, to break the dependency chain.

This has the potential to break a lot 3rd party out of tree code.

@jow- Every change has this potential, but I think the change of having luci authenticate trough the rpcd had much more consequences. Here the worst case is adding a new "require" line and replacing "util" by "xml".
And we can gain 100kb of flash.

@jow-
Copy link
Contributor

jow- commented Jun 16, 2020

Please add wrappers to luci.util restoring this functionality. You can lazy-load luci.xml if needed.

@SvenRoederer SvenRoederer force-pushed the package_luci-base-libs branch 4 times, most recently from a451d67 to 9c178e6 Compare June 21, 2020 00:22
@SvenRoederer
Copy link
Contributor Author

@jow- added a wrapper to luci.util which calls the luci.xml functions. Also added a message that the code should be updated to use luci.xml directly.

To complete the previous commit these functions are defined in the resulting
luci-base package but are also used in the new luci-base-libs package. So
move them into the new xml-module of the new package.

Signed-off-by: Sven Roederer <freifunk@it-solutions.geroedel.de>
In teh previous commit the luci.xml module was created, Let'S change all
references to the old functions to the new xml-module.

Signed-off-by: Sven Roederer <freifunk@it-solutions.geroedel.de>
To satisfy the dependencies.

Signed-off-by: Sven Roederer <freifunk@it-solutions.geroedel.de>
@SvenRoederer
Copy link
Contributor Author

@SvenRoederer
Copy link
Contributor Author

@jow- Did you had a chance to review this PR again after adding the compatibility-wrapper as requested?

@jow-
Copy link
Contributor

jow- commented Jul 20, 2020

Merged via 8b8d83e, 68521fc, 354e4cb, 7edd635

@jow- jow- closed this Jul 20, 2020
@SvenRoederer
Copy link
Contributor Author

Thanks

@SvenRoederer SvenRoederer deleted the package_luci-base-libs branch July 20, 2020 19:56
SvenRoederer added a commit to SvenRoederer/freifunk-berlin-firmware that referenced this pull request Jul 24, 2020
- see openwrt/luci#2817
- OpenWrt-LuCI: 7edd635026594da577b73f059c0d8b95d653f8fc, 354e4cb4a70d576a4f02e024f3c8fb1e72f09350, 68521fca04e9b2ae801cc2daa4f920a2c43bc03f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature pull request adding a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants