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

Use libremesh.mk #729

Merged
merged 7 commits into from
Sep 28, 2020
Merged

Use libremesh.mk #729

merged 7 commits into from
Sep 28, 2020

Conversation

aparcar
Copy link
Member

@aparcar aparcar commented Jun 29, 2020

Instead of having a complex Makefile per package this commit introduces
a Makefile template called `libremesh.mk` which is included in all
compatible LibreMesh packages.

This follow the approach of LuCI where only very basic information is
required per package and the rest is evaluated automatically.

The following variables can be set:

* LIME_TITLE
* LIME_MAINTAINER
* LIME_DESCRIPTION
* LIME_DEPENDS

Everything else is automatically determined.

@codecov-commenter
Copy link

codecov-commenter commented Jun 29, 2020

Codecov Report

Merging #729 into master will decrease coverage by 0.28%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #729      +/-   ##
==========================================
- Coverage   73.90%   73.62%   -0.29%     
==========================================
  Files          34       35       +1     
  Lines        2817     2972     +155     
==========================================
+ Hits         2082     2188     +106     
- Misses        735      784      +49     
Impacted Files Coverage Δ
...e-proto-bmx6/files/usr/lib/lua/lime/proto/bmx6.lua 68.38% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 80c7937...6962300. Read the comment docs.

@aparcar aparcar force-pushed the libremesh.mk branch 2 times, most recently from 9c6bdb7 to ba7b5fd Compare June 29, 2020 02:03
@ilario
Copy link
Member

ilario commented Jun 30, 2020

It sounds very good to me. I hate writing Makefiles and to have a unified template seems very convenient.

Some of the packages could need special stuff (e.g. @aparcar mentioned that tinc package Makefile cannot be simplified this way).
The packages and maintainers of the packages involved in this PR are (maintainers, could you comment if this change is ok?):

@altergui

  • auto-usb-wwan

@p4u

  • bmx7-auto-gw-mode

@G10h4ck

  • lime-proto-babeld
  • lime-proto-batadv
  • lime-proto-bmx6
  • lime-proto-bmx7
  • lime-proto-wan
  • shared-state-babeld_hosts
  • shared-state-bat_hosts
  • shared-state

@aparcar

  • lime-smart-wifi

@aparcar
Copy link
Member Author

aparcar commented Jun 30, 2020

This can be seen as a drop in replacement. All other packages with regular Makefiles keep working. Special cases like tinc, where actual C compiling is required, keep their individual Makefiles.

@spiccinini
Copy link
Contributor

spiccinini commented Jul 6, 2020

It is simpler/cleaner.
But for new developers IMHO it is not easier as it is different than "normal" openwrt packages, we are adding a layer of indirection (and in makefiles that are not very friendly). So with things like this I tend to think that maybe we can improve upstream somehow. But well it is only a personal opinon i am not -1.
Also not all packages will be converted right? As they would be harder to upstream. So we will have two ways of doing packages, the "simplified lime way" and the normal package way.

I would prefer something that use composition instead of inheritance, something like:

include $(INCLUDE_DIR)/lime_package.mk

define Build/Compile
	default_compile
	remove_script_comments
endef

@aparcar
Copy link
Member Author

aparcar commented Jul 6, 2020

I don't see how this makes anything more difficult. Whoever understands the regular Makefile for packages likely also saw a LuCI Makefile, which is pretty much the same as the LiMe Makefile. Also, as it is still possible to use regular Makefile, existing developers can show the new developer how to convert the Makefile.

I did not count but I think the libremesh.mk covers 85% of the lime-packages, I don't see the point using the same tweaks in multiple Makefiles instead of defining it one.

Regarding upstream, I never saw any ambitions to get this done. Even if so, we can also migrate the libremesh.mk file as it's done for e.g. Python, they ship their own file, too1.

G10h4ck
G10h4ck previously requested changes Jul 9, 2020
Copy link
Member

@G10h4ck G10h4ck left a comment

Choose a reason for hiding this comment

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

The underlining idea is good, but it seems there are also a bunch of unrelated changes mixed in into this PR, that makes review error prone, please split this PR in smaller pieces taking in account also other comments

libremesh.mk Show resolved Hide resolved
@ilario
Copy link
Member

ilario commented Aug 29, 2020

Some duplicate files accidentally appeared in the pull request, @aparcar could you delete them?

packages/lime-proto-bmx6/src/etc/uci-defaults/85-add-bmx6-addresses-to-hosts
packages/lime-proto-bmx6/src/usr/bin/bmx6hosts
packages/lime-proto-bmx6/src/usr/bin/bmx6hosts_dnsmasq_update
packages/lime-proto-bmx6/src/usr/lib/lua/lime/proto/bmx6.lua

@ilario
Copy link
Member

ilario commented Aug 29, 2020

And also: this PR includes the .github/workflows/build.yml file which better fits #728. @aparcar Can you remove it from here?

@aparcar
Copy link
Member Author

aparcar commented Aug 30, 2020

And also: this PR includes the .github/workflows/build.yml file which better fits #728. @aparcar Can you remove it from here?

@ilario done

@ilario
Copy link
Member

ilario commented Sep 23, 2020

@aparcar have you seen my other comment on the accidental creation of duplicated files #729 (comment) ?

@aparcar aparcar force-pushed the libremesh.mk branch 2 times, most recently from e6f111d to 1eaecba Compare September 23, 2020 20:17
@aparcar
Copy link
Member Author

aparcar commented Sep 23, 2020

@aparcar have you seen my other comment on the accidental creation of duplicated files #729 (comment) ?

Removed.

Instead of having a complex Makefile per package this commit introduces
a Makefile template called `libremesh.mk` which is included in all
compatible LibreMesh packages.

This follow the approach of LuCI where only very basic information is
required per package and the rest is evaluated automatically.

The following variables can be set:

* LIME_TITLE
* LIME_MAINTAINER
* LIME_DESCRIPTION
* LIME_DEPENDS

Everything else is automatically determined.
Signed-off-by: Paul Spooren <mail@aparcar.org>
Copy link
Member

@ilario ilario left a comment

Choose a reason for hiding this comment

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

An additional comment: you merged the files in the lime-smart-wifi packages into a srv/ directory instead than into a files/ directory.

packages/lime-proto-bmx6/Makefile Outdated Show resolved Hide resolved
@aparcar aparcar force-pushed the libremesh.mk branch 2 times, most recently from 7bad4c0 to 6962300 Compare September 24, 2020 19:46
@ilario
Copy link
Member

ilario commented Sep 26, 2020

While we wait for @aparcar to change the location of the files inside lime-smart-wifi, can some other dev say if you're ok with this new style of Makefile for LibreMesh specific packages (up to now we had a positive opinion from @G10h4ck and from @dangowrt and a neutral opinion from @spiccinini)? @nicopace @gmarcos87 @germanferrero

Signed-off-by: Paul Spooren <mail@aparcar.org>
Signed-off-by: Paul Spooren <mail@aparcar.org>
Signed-off-by: Paul Spooren <mail@aparcar.org>
Signed-off-by: Paul Spooren <mail@aparcar.org>
Signed-off-by: Paul Spooren <mail@aparcar.org>
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.

6 participants