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

all: revert freifunk-berlin-generic.mk #207

Conversation

PolynomialDivision
Copy link
Contributor

@PolynomialDivision PolynomialDivision commented Jun 24, 2020

Revert the includes to freifunk-berlin-generic.mk and change them to "include $(INCLUDE_DIR)/package.mk" or "include-luci.mk".
Add "include-luci.mk" to search the "luci.mk" to include it.

Fixes freifunk-berlin/firmware#817

Revert the includes to freifunk-berlin-generic.mk and change them to
"include $(INCLUDE_DIR)/package.mk" or "include-luci.mk".

Add "include-luci.mk" to search the "luci.mk" to include it.

Signed-off-by: Nick Hainke <vincent@systemli.org>
@SvenRoederer
Copy link
Contributor

I'm not for reverting, as we made it this way for the reasons:

  • we don't need to bump Versions, Revisions anymore
  • we don't need copy a lot of Makefile code for every package, no matter how small it is (e.g. the defaults-packages are sometimes just a single file)
  • we can reuse a lot of the logic LuCI provides without copying, syncing and maintaining code (this gives us lua-scrdiet, Csstidy, automatic package-translations, ...)
  • no hassle of rebasing PRs (which often become very old) just as of changed version-numbers in the master

My intention is to push on PR openwrt/luci#2637 after PR openwrt/luci#2817 is merged. (Just as I don't have ressources to nag on both PRs in parallel.)
As mentioned in freifunk-berlin/firmware#817 (comment) and freifunk-berlin/firmware#814 (comment) I suggest to use the PR / the individual commits on your buildbase.

@PolynomialDivision
Copy link
Contributor Author

@SvenRoederer
I'm fine with your commit and your style of handling that, if it would work. This PR is just a draft for now and we can talk about it. :)
I just want to temporarily revert the changes. ;)
This is just blocking completely my plan to build firmware-packages repro with imagebuilder. Applying a PR that is for hold since 1 year is somehow against my plan. :S

If it is merged upstream I can directly use a regex to delete all that stuff again. :D

@PolynomialDivision
Copy link
Contributor Author

@SvenRoederer Lets make an agreement. If I get everything else running and working with openwrt master and the only PR left is this PR, we will merge this PR. And I will revert this PR if your PR against luci.mk is accepted. :D
For me this is very important for my motivation. ;) So I know that I can fulfill my milestone.

@pmelange
Copy link
Contributor

#188 is another one of the PR's which was not a group effort. In that PR only one other person had participated and had asked a question. The author was also the one who pushed the changes to the master branch. In fact, the last comment of the PR was a question from the author itself which was not answered before the merge. Therefore I find the statement "I'm not for reverting, as we made it this way for the reasons" in #207 (comment) misleading.

The most importing thing is that building packages and creating images works. If, as @PolynomialDivision states, that it's not working, then the commit should be reverted.

I find it especially disruptive to have a commit pushed to our master branch which relies on patching openwrt. If the patch to openwrt is not accepted (which it hasn't been), then the commits in #188 are not practical.

@SvenRoederer
Copy link
Contributor

  1. As confirmed in #814 this reported issue only exists when using the our packages-feed on top of OpenWrt-master. Our firmware "design-rules" say:
  • our master-branch is based on a stable upstream branch or release
  • our master-branch should always create a working image
  • (are there more rules?)

All these rules apply to the master-branches of the firmware and the firmware-packages. So nothing is broken, by definition.

  1. When a developer uses some other branch of the underlying repos, we can give no guarantee that all works. It's in fact the same as trying to use our packages-feed master on to of OpenWrt-14.04.

  2. @PolynomialDivision As mentioned, reverting this will break other things again (Lua-SrcDiet, css-tidy). It's a bit of the same situation we had in /daily/upstream-1907 luci did not work (missing package "luci-lib-ipkg") firmware#718 (comment), when we were still on OpenWrt-18.06. As suggested there:

  • branch off a "next", "openwrt-master", or similar, branch of the current master and apply the changes needed to mack it compatible with Openwrt-master and Luci-master. Do the backporting of commits from freifunk-packages master
  • you can do you own fork, which is essentially the same as maintaining a dedicated branch
  • use the existing patches I supplied for "upstream-master" branches on top of your code
  1. @PolynomialDivision

Applying a PR that is for hold since 1 year is somehow against my plan.

As already said above, @feckert welcomes this change (openwrt/luci#2637 (comment)) but it's just hard to get a feedback of @jow- for final comment.

  1. @pmelange

In that PR only one other person had participated and had asked a question. The author was also the one who pushed the changes to the master branch. In fact, the last comment of the PR was a question from the author itself which was not answered before the merge.

The mentioned PR was for review, comments and public test for around 7 month. The last 6 months have been without any feedback, which indicates "no interest" but also "no veto". As all developer benefit from not having to take care of versioning, it was an improvement for all.
If some developer would just have "forgotten" to veto, there was time to raise the hand shortly after the merge.

  1. @pmelange

I find it especially disruptive to have a commit pushed to our master branch which relies on patching openwrt. If the patch to openwrt is not accepted (which it hasn't been), then the commits in #188 are not practical.

As said under 1) our firmware-master is building with our package-master without any patch required. Please check yourself, before guessing.

@pmelange
Copy link
Contributor

pmelange commented Jul 2, 2020

For the python lang feed, they have a recommended process for including it in external feeds. https://github.com/openwrt/packages/tree/master/lang/python#using-python-in-externalother-package-feeds

@SvenRoederer
Copy link
Contributor

openwrt/packages@d607b4d is the code I once published on the OpenWrt-ML. The python maintainer seem to have adapted the code, we also have in our repo, to their needs.
But what's the point you want to show?

@SvenRoederer
Copy link
Contributor

My intention is to push on PR openwrt/luci#2637 after PR openwrt/luci#2817 is merged. (Just as I don't have ressources to nag on both PRs in parallel.)

Most of the mentioned patches have been accepted upstream (openwrt/luci#2817 (comment); openwrt/luci@3b2a1e9#diff-2d8a7a9b9acd99058ff6f4d496ea6ebe) and only 2 patches pending as openwrt/luci#2637.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

all: makefiles won't build with vanilla openwrt (master-branch)
3 participants