mooltipass / moolticute

Mooltipass crossplatform daemon/tools
GNU General Public License v3.0
143 stars 67 forks source link

Remove Makefile in repository root #1130

Closed VincentVanlaer closed 1 year ago

VincentVanlaer commented 1 year ago

This Makefile was only used for the Debian package. It is therefore confusing if this file lives in the root of the repository as it hints that using make is the canonical way to build moolticute. Moreover, it duplicates information that is already in the qmake project files.

As debhelper can handle qmake projects, only minor changes need to be made to debian/rules. These are

bobsaintcool commented 1 year ago

This seems acceptable, @limpkin @raoulh are we sure this Makefile is never used? Sure you will break the moolticute AUR but you own the decision.

Thanks for you contribution

limpkin commented 1 year ago

Sure you will break the moolticute AUR but you own the decision.

do you mean we actually use it for the AUR?

bobsaintcool commented 1 year ago

It seems so https://aur.archlinux.org/cgit/aur.git/tree/PKGBUILD?h=moolticute#n29 So maybe the usage is more wide than only Debian packaging.

We can however pick the following stance for our customer:

However we can also decide not to remove the Makefile and just add what is needed for Debian, what do you think @VincentVanlaer In the end, the canonical way to build is described here https://aur.archlinux.org/cgit/aur.git/tree/PKGBUILD?h=moolticute#n29

Could @VincentVanlaer change this documentation (and other documentation that should reflect use of make) in this PR? However I'm not 100% convinced that we gain something by removing the Makefile

principis commented 1 year ago

@bobsaintcool The aur package actually does use the Makefile, but in a different place than you expect. I think you overlooked the -C parameter.

The -C parameter is documented as followed: Change to DIRECTORY before doing anything.. So the aur package first runs qmake that creates a Makefile in the build directory, after which make is run on build/Makefile. The build section is thus not a problem. However, the makefile is used on line 34.

The Fedora package does use the Makefile for building and testing, but you can safely ignore that. I'll fix it by the next release.

I believe that removing the Makefile is good idea. It already caused confusion and errors (for example the metainfo file is only installed using the Makefile, and not if using qmake), and keeping all configuration in one place is always better for consistency.

VincentVanlaer commented 1 year ago

@bobsaintcool Which documentation do you mean exactly? The README only mentions using qmake and not the root Makefile.

@principis Unless I don't understand what is going on, building already seems to be done with the qmake Makefile? https://src.fedoraproject.org/rpms/moolticute/blob/af97d154ebf4ddd6907fdce26011d29304a79796/f/moolticute.spec#_75-78 Installing and testing do seem to use the root Makefile.

I also want to add that it seems that two packages are using a mix of qmake and the root Makefile (hence why I thought that only the Debian package was using the root Makefile), which does not seem healthy. I believe that leaving the root Makefile like it currently is makes it too easy to end up which such a situation.

principis commented 1 year ago

@VincentVanlaer thanks for the correction!

bobsaintcool commented 1 year ago

@bobsaintcool The aur package actually does use the Makefile, but in a different place than you expect. I think you overlooked the -C parameter.

@principis thanks for the details and picked up the good line on the PKGBUILD thanks. Overall my comment pointed that the assumption "only used in for Debian packaging" might not be 100% right.

@bobsaintcool Which documentation do you mean exactly? The README only mentions using qmake and not the root Makefile.

@VincentVanlaer I was at least referring to the README (but did not check if there is more doc). However I double check and the make command is done after cd into build so my comment does not apply.

To be 100% good I believe we should be able to run what root Makefile test target from the qmake. @VincentVanlaer do you believe this is achievable? In addition to the above I do not have anything against removing Makefile but still suggest a release or something like that.

VincentVanlaer commented 1 year ago

Afaics the only thing the root Makefile does for test is run make check on the qmake Makefile. Hence replacing make test by make -C build check should be sufficient (assuming the qmake Makefile was generated in build)

limpkin commented 1 year ago

hello everyone, what's the current consensus on this PR?

VincentVanlaer commented 1 year ago

From my side it is ready to be merged, I believe I have addressed all remarks

limpkin commented 1 year ago

thanks a lot guys :)