void-packages icon indicating copy to clipboard operation
void-packages copied to clipboard

nnn: add build options

Open freddylist opened this issue 3 years ago • 15 comments

Testing the changes

  • I tested the changes in this PR: YES

Local build testing

  • I built this PR locally for my native architecture, x86-64-glibc

~~xlint doesn't like my string on multiple lines.~~

freddylist avatar Sep 19 '22 08:09 freddylist

I'm not sure what the point of this change is and I don't think the plethora of build options is necessary, the default ones should just be the default make_build_args without options defined, if anything.

classabbyamp avatar Sep 19 '22 10:09 classabbyamp

I'm not sure what the point of this change is and I don't think the plethora of build options is necessary, the default ones should just be the default make_build_args without options defined, if anything.

I tried "generalizing" the build options. If you take a look at nnn's build options, most of them are reversed like O_NOMOUSE to compile out mouse support. The build options in build_options_default are nnn's default options.

The point of this change is to be able to manage nnn with custom build options like O_NERD and user patches like O_GITSTATUS with xbps-src.

freddylist avatar Sep 19 '22 10:09 freddylist

We ain't Gentoo. I don't think a hundred of build option is a good idea. nnn is fast to be built. I think it's better to patch their Makefile to have let's say -include config.mk and put the option there. Whoever want some custom option could put the config into files directory. With that approach, we won't need to care for nnn's change in build option whatsoever. As for build dependencies, just add them all. nnn is always a leaf (as in graph) packages.

sgn avatar Sep 19 '22 10:09 sgn

We ain't Gentoo.

Then what's the point of xbps-src -o option1,option2,...?

freddylist avatar Sep 19 '22 10:09 freddylist

We ain't Gentoo.

Then what's the point of xbps-src -o option1,option2,...?

When disabling an option makes sense - (sometimes) when a feature should only be enabled in certain situations (only on some archs), when it makes sense that some people might want to disable an option (minimalism is not a reason). Applying patches in minimalist software should be left to the user, I don't think we should wrap that in xbps-src options and keep it up to date.

Sometimes it makes sense to have an option to avoid a big dependency, but then it's usually better to split part of the package to a subpackage (ffmpeg, ffplay).

paper42 avatar Sep 19 '22 11:09 paper42

Applying patches in minimalist software should be left to the user, I don't think we should wrap that in xbps-src options and keep it up to date.

I agree. Now that I think about it, I'm actually not sure why I included the build options for the patches, I think I just shoved them in there. :p

I still think we should add at least the 3 build options to add icon support (icons/O_ICONS, nerdfont/O_NERD, emojis/O_EMOJI) as that seems to be a fairly common thing people want. Maybe we should also add the few other options that compile in features.

Users who want patches should just download them from here and add them to the nnn/patches/ directory.

Anyone who wants to compile out features is on their own I guess.

freddylist avatar Sep 19 '22 11:09 freddylist

I've slimmed it down to just the build options that compile in features, as well as the build option that compiles nnn with static linking, but maybe it's still too much?

freddylist avatar Sep 19 '22 12:09 freddylist

Is there a reason why we can't just unconditionally enable the features?

as well as the build option that compiles nnn with static linking, but maybe it's still too much?

remove that, there is no reason to have it in void-packages

paper42 avatar Sep 19 '22 12:09 paper42

I think it's better to patch their Makefile to have let's say -include config.mk and put the option there. Whoever want some custom option could put the config into files directory. With that approach, we won't need to care for nnn's change in build option whatsoever. As for build dependencies, just add them all. nnn is always a leaf (as in graph) packages.

sgn avatar Sep 19 '22 12:09 sgn

Is there a reason why we can't just unconditionally enable the features?

User's patches, with questionable quality and/or minimalism ;)

sgn avatar Sep 19 '22 12:09 sgn

Is there a reason why we can't just unconditionally enable the features?

You have to choose between icons-in-terminal, nerd font icons, or emojis. They are mutually exclusive, according to this.

The build option that enables 8 contexts (as opposed to 4) might break sessions according to this.

as well as the build option that compiles nnn with static linking, but maybe it's still too much?

remove that, there is no reason to have it in void-packages

I just saw this, will remove. 👍

freddylist avatar Sep 19 '22 13:09 freddylist

I think it's better to patch their Makefile to have let's say -include config.mk and put the option there. Whoever want some custom option could put the config into files directory. With that approach, we won't need to care for nnn's change in build option whatsoever. As for build dependencies, just add them all. nnn is always a leaf (as in graph) packages.

Sorry if this is a dumb question, but where would that be documented? Shouldn't users just place patch files in the nnn template's patches/ directory?

As for build options, if I was trying to manage nnn with xbps-src, I would at least except the options to enable icon support to be present in xbps-src show-options nnn, as it is a popular thing to compile nnn with.

freddylist avatar Sep 19 '22 13:09 freddylist

I think it's better to patch their Makefile to have let's say -include config.mk and put the option there. Whoever want some custom option could put the config into files directory. With that approach, we won't need to care for nnn's change in build option whatsoever. As for build dependencies, just add them all. nnn is always a leaf (as in graph) packages.

Here is possible implementation, no build options required:

 srcpkgs/nnn/patches/config.patch | 11 +++++++++++
 srcpkgs/nnn/template             |  5 ++++-
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/srcpkgs/nnn/patches/config.patch b/srcpkgs/nnn/patches/config.patch
new file mode 100644
index 0000000000..e7d7238276
--- /dev/null
+++ b/srcpkgs/nnn/patches/config.patch
@@ -0,0 +1,11 @@
+--- a/Makefile
++++ b/Makefile
+@@ -36,6 +36,8 @@ O_GITSTATUS := 0 # add git status to det
+ O_NAMEFIRST := 0 # print file name first, add uid and guid to detail view
+ O_RESTOREPREVIEW := 0 # add preview pipe to close and restore preview pane
+ 
++-include config.mk
++
+ # convert targets to flags for backwards compatibility
+ ifneq ($(filter debug,$(MAKECMDGOALS)),)
+ 	O_DEBUG := 1
diff --git a/srcpkgs/nnn/template b/srcpkgs/nnn/template
index cea82d3681..32f5fec3c1 100644
--- a/srcpkgs/nnn/template
+++ b/srcpkgs/nnn/template
@@ -5,7 +5,7 @@ revision=1
 build_style=gnu-makefile
 make_install_target="install install-desktop"
 hostmakedepends="pkg-config"
-makedepends="ncurses-devel readline-devel"
+makedepends="ncurses-devel readline-devel gpm-devel pcre-devel"
 short_desc="Missing terminal file browser for X"
 maintainer="Dennis Kraus <[email protected]>"
 license="BSD-2-Clause"
@@ -22,6 +22,9 @@ pre_build() {
 	if [ "$XBPS_TARGET_LIBC" = "musl" ]; then
 		export LDLIBS=-lfts
 	fi
+	if [ -f "$FILESDIR/config.mk" ]; then
+		cp "$FILESDIR/config.mk" .
+	fi
 }
 
 post_install() {

Then:

mkdir -p srcpkgs/nnn/files
cat <<-EOF >srcpkgs/nnn/files/config.mk
O_ICONS := 1
EOF

sgn avatar Sep 19 '22 13:09 sgn

I've removed the option for static linking.

I also tried to cross-compile for x86_64-musl with icon support, but the makefile fails here with this:

/bin/sh: line 1: ./src/icons-hash-gen: No such file or directory
make: *** [Makefile:211: src/icons-generated.h] Error 127

However, if I look in the builddir for nnn, the src/icons-hash-gen file is there (with necessary permissions)...

If I try again without cleaning, I get different errors because of whacky definitions.

Cross-compiling without icon support works fine.

I will just hope this is just a problem with cross-compiling for me. 😁

Looks like it builds for other platforms. 🤷

freddylist avatar Sep 19 '22 13:09 freddylist

Just want to show that I'm opposing to build_options.

Do any other Void packages do what you're proposing? And again, where is or would it be documented? xbps-src may be simple enough so that you can document it as a comment in the template file and users can just inspect the template file, but that goes a little too far in my opinion. If we do this for nnn, should we do something like this for other similarly minimal software?

As for user submitted patches, again, users can just download the correct patches for their nnn version and drop them into the patches/ directory.

Also, this might have to be maintained more than build options; makefiles change, but build options are more-or-less permanent.

For users who want more build options, maybe there should be a link back to the makefile patch in the template?

All I want are my delicious nnn icons. 🤤

freddylist avatar Sep 20 '22 08:09 freddylist

See st, dwm

sgn avatar Dec 06 '22 00:12 sgn