jarun / nnn

n³ The unorthodox terminal file manager
BSD 2-Clause "Simplified" License
19.35k stars 761 forks source link

Makefile bug causes `nnn` to build incorrectly with user patches in automated build systems #1493

Closed freddylist closed 2 years ago

freddylist commented 2 years ago

Environment details

Exact steps to reproduce the issue

  1. Clone the repository and cd into it
  2. make O_GITSTATUS=1
    1. ./nnn
    2. Marvel at your new nnn build with git status 😌
    3. Quit nnn
  3. make install
    1. Speculate at why nnn would be rebuilding 🤔
    2. Shrug it off 🤷
  4. /usr/local/bin/nnn and despair as your git status is no more 😱

I'm new to makefiles and this is my first time debugging a makefile, but I believe this happens because after the first make, the source gets "unpatched" after creating the nnn binary, so make thinks the sources have been updated and wants to redo the binary on the next make install.

https://github.com/jarun/nnn/blob/0907895865f632876dfc478fe0c50811724009a2/Makefile#L211

This is a problem because Void's xbps-src GNU Makefile recipe essentially first runs make ${build_opts} followed by a make install. That means if a user compiles nnn with a user patch using xbps-src, they will get a default build of nnn. I imagine this could also be a problem for other similar automated build systems that generate flavors of packages.

A simple workaround is to simply touch the binary after building and before installing, but I imagine that could potentially lead to other subtle bugs.

As I said before, I'm a complete noob (it took me several hours to debug this 😅) so I'm not confident enough to touch the makefile...

That said, if you're super busy I can definitely make an attempt if someone points me in the right direction. 😄

N-R-K commented 2 years ago

but I believe this happens because after the first make, the source gets "unpatched" after creating the nnn binary, so make thinks the sources have been updated and wants to redo the binary on the next make install.

Indeed. A "simple fix" would be to remove all from the prerequisite of install

- install: all
+ install:

This would always install the current binary without trying to recompile. But that'd break make install (without running make or make all first).

I think the right answer here is to compile and install at the same time:

$ doas make O_GITSTATUS=1 all install

works fine on my end.

For package managers, I think they should patch out all from install prerequisite as I've shown above.

jarun commented 2 years ago

Closing as resolved.