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.mk: allow more customization (for external feeds/packages) #2637

Merged
merged 3 commits into from
Jun 2, 2021

Conversation

SvenRoederer
Copy link
Contributor

@SvenRoederer SvenRoederer commented Mar 23, 2019

Split the current luci.mk file into two files, "luci-common.mk" with definitions that might be relevant for other feeds too and "luci.mk" to be included by our packages.

EDIT: After the NACK of my 1st version in #2637 (comment) I did a complete rework, which only adds / changes some macros.
This new Version will not split the current luci.mk but allows nearly all defaults to be overwritten externally. This allows including the luci.mk in external Makefiles and allowing strong customization.

After the move of the freifunk-packages to it's own feed (55cd0c4) such "luci-common.mk" will make the freifunk-packages build again without recreating the Makefile or copying the common code over to this feed.
The luci-apps of the openwrt-routing feed might also benefit from the common definition of the LuCI-menu and the srcdiet feature.

I send this PR in 2 individual commits for easy reviewing but finally all can be squashed into one single commit.

Further down @feckert suggested to just link to luci.mk from 3rd-party repos to benefit from the features of luci.mk, which I also did before creating this PR. Linking had several drawbacks:

  • inclusion of unneeded definitions e.g. LuciTranslation, luci-base (V1 removed this unneeded code for external repos; V2 keeps itas it'S just conditional code that is not causing problems)
  • some hardcoded defaults, that can't be changed, e.g. CATEGORY:=LuCI can't be changed (This has been made customizable in patch v2)
  • no way to define the URL or MAINTAINER variable of a Package, which is not required for LuCI, but might be for 3rd-party (This has been made customizable in patch v2)

SvenRoederer referenced this pull request in SvenRoederer/freifunk_openwrt-packages Mar 23, 2019
SvenRoederer added a commit to freifunk-berlin/firmware that referenced this pull request Mar 30, 2019
This adds PR openwrt/luci#2637 to the build, in
order to fix freifunk-feed issue of brocken packages as of missing
luci.mk (freifunk/openwrt-packages#9).
SvenRoederer added a commit to freifunk-berlin/firmware that referenced this pull request Mar 30, 2019
This adds PR openwrt/luci#2637 to the build, in
order to fix freifunk-feed issue of brocken packages as of missing
luci.mk (freifunk/openwrt-packages#9).
SvenRoederer added a commit to freifunk-berlin/firmware that referenced this pull request Mar 30, 2019
This adds PR openwrt/luci#2637 to the build, in
order to fix freifunk-feed issue of brocken packages as of missing
luci.mk (freifunk/openwrt-packages#9).
SvenRoederer added a commit to SvenRoederer/freifunk_openwrt-packages that referenced this pull request Mar 31, 2019
@jow-
Copy link
Contributor

jow- commented Apr 8, 2019

What is preventing other packages from using luci.mk directly?

@SvenRoederer
Copy link
Contributor Author

One fact of suggesting this is making the Makefile more readable. When working on fixing the freifunk-packages, recently split off the luci-feed, it was really handy to get not distracted by these luci-only definitions like:

  • ifeq ($(PKG_NAME),luci-base)
  • define LuciTranslation
    Having this separate luci-common.mk might help also others to reuse this definitions in other feeds.

SvenRoederer added a commit to freifunk/openwrt-packages that referenced this pull request Apr 19, 2019
SvenRoederer added a commit to freifunk/openwrt-packages that referenced this pull request Apr 19, 2019
@SvenRoederer
Copy link
Contributor Author

@jow- using the unmodified luci.mk causes a lot of warnings:

make[2]: Entering directory '/mnt/hosts/build/src/openwrt/packages_sven/freifunk/modules/luci-mod-freifunk'
../../freifunk.mk:48: warning: overriding recipe for target '/mnt/hosts/build/src/openwrt/freifunk/lede/openwrt/staging_dir/target-arm_cortex-a9+vfpv3_musl_eabi/pkginfo/luci-mod-freifunk.default.install.luci-mod-freifunk'

I guess this is caused by calling the "Buildpackage" macro by the feeds Makefile and a 2nd time by luci.mk

@feckert
Copy link
Member

feckert commented Sep 23, 2019

I have also luci packages in my own feed.
To use luci.mk from my own feed directly I added a symlink to the main luci.mk file in the luci feed
An ls in your own feed in the toplevel directory would show luci.mk -> ../luci/luci.mk.
With this I could call luci.mk in my makefiles.

@feckert feckert changed the title Split luci.mk into a common and a feed-internal file luci.mk: into a common and a feed-internal file Sep 23, 2019
@feckert feckert added the feature pull request adding a new feature label Sep 23, 2019
@feckert
Copy link
Member

feckert commented Dec 7, 2019

@SvenRoederer Does my last message solve your issue with luci.mk?

@SvenRoederer
Copy link
Contributor Author

@feckert linking / including the original luci.mk was also done by me. But as this file defines many things that are not needed in other feeds (e.g. LuciTranslation, package luci-base) I liked the idea of splitting this file. This also improves the readability when using in other feeds, as of less uninteresting definitions and receipts.

@SvenRoederer
Copy link
Contributor Author

@feckert by just linking / including the original luci.mk I found no way to make a package appear outside of the LuCI-menu of menuconfig (caused by hardcoded CATEGORY:=LuCI of Package/$(PKG_NAME) section).
For our freifunk-firmware, we like to keep most of our packages in a separate menu-section.

SvenRoederer added a commit to freifunk-berlin/firmware that referenced this pull request Dec 27, 2019
SvenRoederer added a commit to freifunk-berlin/firmware that referenced this pull request Dec 27, 2019
…on.mk

This include was added by PR openwrt/luci#2637 (see
the relating patch-file)
SvenRoederer added a commit to freifunk-berlin/firmware that referenced this pull request Dec 27, 2019
…on.mk

This include was added by PR openwrt/luci#2637 (see
the relating patch-file)
SvenRoederer added a commit to SvenRoederer/freifunk-berlin-firmware that referenced this pull request Dec 27, 2019
SvenRoederer added a commit to SvenRoederer/freifunk-berlin-firmware that referenced this pull request Dec 27, 2019
…on.mk

This include was added by PR openwrt/luci#2637 (see
the relating patch-file)
@feckert
Copy link
Member

feckert commented Dec 29, 2019

From my side, the explanation sounds conclusive.
👍
But the last word has @jow-

@SvenRoederer
Copy link
Contributor Author

in our freifunk-firmware we use the following code to locate and include the luci-common.mk file

LUCIMKFILE:=$(wildcard $(TOPDIR)/feeds/*/luci-generic.mk)

# verify that there is only one single file returned
ifneq (1,$(words $(LUCIMKFILE)))
ifeq (0,$(words $(LUCIMKFILE)))
$(error did not find luci.mk in any feed)
else
$(error found multiple luci.mk files in the feeds)
endif
else
#$(info found luci.mk at $(LUCIMKFILE))
endif

...
# place Makefile here
...

include $(LUCIMKFILE)

SvenRoederer added a commit to SvenRoederer/freifunk-berlin-firmware that referenced this pull request Jan 2, 2020
SvenRoederer added a commit to SvenRoederer/freifunk-berlin-firmware that referenced this pull request Jan 2, 2020
…on.mk

This include was added by PR openwrt/luci#2637 (see
the relating patch-file)
SvenRoederer added a commit to SvenRoederer/freifunk-berlin-firmware that referenced this pull request Jan 2, 2020
SvenRoederer added a commit to SvenRoederer/freifunk-berlin-firmware that referenced this pull request Jan 2, 2020
…on.mk

This include was added by PR openwrt/luci#2637 (see
the relating patch-file)
SvenRoederer added a commit to freifunk-berlin/firmware that referenced this pull request Jan 2, 2020
@SvenRoederer
Copy link
Contributor Author

@behzaddana What do you mean by "M3 config"?

SvenRoederer added a commit to SvenRoederer/freifunk-berlin-firmware that referenced this pull request May 8, 2021
The PR openwrt/luci#2637 has been completely rewritten, so update to it.
SvenRoederer added a commit to SvenRoederer/freifunk-berlin-firmware that referenced this pull request May 8, 2021
The PR openwrt/luci#2637 has been completely rewritten, so update to it.
SvenRoederer added a commit to SvenRoederer/freifunk-berlin-firmware that referenced this pull request May 9, 2021
The PR openwrt/luci#2637 has been completely rewritten, so update to it.
@SvenRoederer
Copy link
Contributor Author

@jow- did you have time to review?

SvenRoederer added a commit to SvenRoederer/freifunk-berlin-firmware that referenced this pull request May 13, 2021
The PR openwrt/luci#2637 has been completely rewritten, so update to it.
SvenRoederer added a commit to SvenRoederer/freifunk-berlin-firmware that referenced this pull request May 16, 2021
The PR openwrt/luci#2637 has been completely rewritten, so update to it.
@SvenRoederer
Copy link
Contributor Author

@feckert can you verify this change with your own feed? I'm currently using this change in my personal builds of our firmware for the Freifunk- and Freifunk-Berlin feeds (only)

@SvenRoederer
Copy link
Contributor Author

@Freifunk-Potsdam @Freifunk-Spalter as you are also using my old "LuCI-include" code, you might also benefit from the improvements of this PR (esp. placing the packages outside of the LuCI menu, embedding your own Repo-URL).

@SvenRoederer
Copy link
Contributor Author

@jow- just seen that OpenWrt-21.02-rc2 has been tagged. How are the chances to get this merged to master and backported to 21.02 before official release?

SvenRoederer added a commit to SvenRoederer/freifunk-berlin-firmware that referenced this pull request May 30, 2021
* for LuCI, packages_berlin feed

squashed:
* "patches/luci: replace patches for LuCI PR 2637 with recent version"
* "patches/berlin: adapt to rewritten LuCI-PR2637"
* "patches/berlin: update "rework-package-defines-to-changes-of-luci.mk" to reapply"
* "adapt to renamed package "freifunk-berlin-uplink-notunnel""
SvenRoederer added a commit to SvenRoederer/freifunk-berlin-firmware that referenced this pull request May 30, 2021
* for LuCI, packages_berlin feed

squashed:
* "patches/luci: replace patches for LuCI PR 2637 with recent version"
* "patches/berlin: adapt to rewritten LuCI-PR2637"
* "patches/berlin: update "rework-package-defines-to-changes-of-luci.mk" to reapply"
* "adapt to renamed package "freifunk-berlin-uplink-notunnel""
SvenRoederer added a commit to SvenRoederer/freifunk-berlin-firmware that referenced this pull request May 30, 2021
* for LuCI, packages_berlin feed

squashed:
* "patches/luci: replace patches for LuCI PR 2637 with recent version"
* "patches/berlin: adapt to rewritten LuCI-PR2637"
* "patches/berlin: update "rework-package-defines-to-changes-of-luci.mk" to reapply"
* "adapt to renamed package "freifunk-berlin-uplink-notunnel""
SvenRoederer added a commit to SvenRoederer/freifunk-berlin-firmware that referenced this pull request May 30, 2021
* for LuCI, packages_berlin feed

squashed:
* "patches/luci: replace patches for LuCI PR 2637 with recent version"
* "patches/berlin: adapt to rewritten LuCI-PR2637"
* "patches/berlin: update "rework-package-defines-to-changes-of-luci.mk" to reapply"
* "adapt to renamed package "freifunk-berlin-uplink-notunnel""
SvenRoederer added a commit to SvenRoederer/freifunk-berlin-firmware that referenced this pull request May 30, 2021
* for LuCI, packages_berlin feed

squashed:
* "patches/luci: replace patches for LuCI PR 2637 with recent version"
* "patches/berlin: adapt to rewritten LuCI-PR2637"
* "patches/berlin: update "rework-package-defines-to-changes-of-luci.mk" to reapply"
* "adapt to renamed package "freifunk-berlin-uplink-notunnel""
@aparcar
Copy link
Member

aparcar commented May 31, 2021

Looks pretty good to me, i don't see why this should be merged.

@SvenRoederer
Copy link
Contributor Author

Looks pretty good to me, i don't see why this should be merged.

@aparcar I'm confused now ... "looks pretty good" as luci.mk is now, that this change should not be needed?

@aparcar
Copy link
Member

aparcar commented Jun 1, 2021

Oh snap I meant to say "shouldn't be merged"

@aparcar
Copy link
Member

aparcar commented Jun 2, 2021

Thanks for your patience, i hope we didn't discourage your from further contributions!

@aparcar aparcar merged commit 2b11ec6 into openwrt:master Jun 2, 2021
@SvenRoederer SvenRoederer deleted the split-luci.mk branch June 2, 2021 23:02
SvenRoederer added a commit to SvenRoederer/freifunk-berlin-firmware that referenced this pull request Jun 2, 2021
@SvenRoederer
Copy link
Contributor Author

@aparcar indeed took some time, but it's volunteer work for all of us.

AS it's a non-breaking change, can this be cherry-picked into openwrt-21.02 too?

@aparcar
Copy link
Member

aparcar commented Jun 3, 2021

To github.com:openwrt/luci.git
74e04dd..51da4d1 openwrt-21.02 -> openwrt-21.02

SvenRoederer added a commit to SvenRoederer/freifunk-berlin-firmware that referenced this pull request Jun 3, 2021
SvenRoederer added a commit to freifunk-berlin/firmware that referenced this pull request Jun 13, 2021
* switch to OpenWrt 21.02.0-rc2+
  * changed UCI-configuration for bridges (freifunk-berlin/firmware-packages#221)
* move OWM add to Freifunk-repo
* fix build of our packages after accepting openwrt/luci#2637
SvenRoederer added a commit to SvenRoederer/freifunk-berlin-firmware that referenced this pull request Jul 13, 2021
* for LuCI, packages_berlin feed

squashed:
* "patches/luci: replace patches for LuCI PR 2637 with recent version"
* "patches/berlin: adapt to rewritten LuCI-PR2637"
* "patches/berlin: update "rework-package-defines-to-changes-of-luci.mk" to reapply"
* "adapt to renamed package "freifunk-berlin-uplink-notunnel""
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.

6 participants