feature request: add `--destdir` option to install.sh

[ehaupt]

As a package maintainer I have a small request:

During ports/package building on FreeBSD the files need to be installed in a staging area first (or call it a sandbox). After the installation in the sandbox among other things, the package will be created.

The sandbox directory will look something like this:

/usr/ports/deskutils/arttime/work/stage/usr/local/share/arttime/textart/eye2
/usr/ports/deskutils/arttime/work/stage/usr/local/share/arttime/textart/pizza2
/usr/ports/deskutils/arttime/work/stage/usr/local/share/arttime/textart/mirror2
/usr/ports/deskutils/arttime/work/stage/usr/local/share/arttime/textart/freebsd1
...

In order to tell install.sh to install the files under /usr/ports/deskutils/arttime/work/stage I used to modify install.sh as follows: Patch for packaging

You can see that I'm prefixing install paths with $(DESTDIR), an ENV variable that is defined, and set to /usr/ports/deskutils/arttime/work/stage during packaging.

In the past I've just patched the install.sh but since it's changing constantly this has become a bit of an arduous task.

Hence my request:

Could you maybe add an --destdir option to install.sh that by default would be undefined but could be used by packagers such as me.

Supporting a sandbox installation directory is a common practice that is supported by many other build systems.

While looking at the patch I am also completely skipping everything that comes after:

 # Check if path to arttime excutable is on user's $PATH

Since that is not relevant in a sandboxed directory (during package building). While here, could you maybe also think of a install.sh option to skip that? Maybe: --skip-execution-test

[poetaman]

@ehaupt I added a switch --noupdaterc for the second problem you mention. If passed, installer won't add anything to rc files. https://github.com/poetaman/arttime/commit/0f79226fa87cebbf0140ad49c27a31d8919e596b. That way you don't need to patch anything.

For the first problem, I am not sure what exactly is needed here? Why can't the --prefix you pass to the installer be /usr/ports/deskutils/arttime/work/stage/usr/local?

Like this (assuming DESTDIR=/usr/ports/deskutils/arttime/work/stage):

./install.sh --noupdaterc --prefix $DESTDIR/usr/local --zompdir $DESTDIR/usr/local/share/zsh/functions
# OR
./install.sh --noupdaterc --prefix $DESTDIR/usr/local --zompdir $DESTDIR/usr/share/zsh/functions

From your description it looks like you are just adding a constant value to an existing prefix option. If I am missing something please let me know...

[ehaupt]

In the case of arttime's installer the --prefix argument might suffice unless it's value is used to adjust paths within the projects code.

Consider this:

I use --prefix=/mnt/sandbox/usr/local/share/zsh/functions and during the installation/building prefix is also used to adjust paths within the code.

Now you enter the chroot (in my example /mnt/sandbox). You execute the script and it won't find the paths that have been modified to /mnt/sandbox/usr/local/share/zsh/functions because within the chroot the files are located under /usr/local/share/zsh/functions

Again, this is not the case here. But its generally a good practice to have something like destdir for sandbox use in case path substitution is done at some point.

I hope that makes sense. Maybe this explains it better.

[poetaman]

@ehaupt Please pardon my lack of understanding of installers, though what do you mean by adjust paths within project code?

In the case of arttime's installer the --prefix argument might suffice unless it's value is used to adjust paths within the projects code.

From your comments it looks like it is not necessary to have DESTDIR prefix for arttime's installer? If I got it right then let's refrain from adding it at the moment. In future if I am doing a "adjust paths" (which I know nothing about), then it would make most sense to add DESTDIR. My concern is that if I add it now without fully understanding what can go wrong, I might break something in future and request you to review anyway. So it makes sense to delay this till its really needed.

One last thing I notice in your installer patch is the addition of exit 0 at the end. Did you have to do that because there was some kind of error in a CI run for installer? I noticed that Homebrew's CI would fail because echoti cnorm (last line in install.sh) would return error as TERM was not really set there. In a reasonable machine, TERM would be set to vt100 or its derivative. Check my comment there: https://github.com/Homebrew/homebrew-core/pull/124403#issuecomment-1449041953. If you added exit 0 to bypass problems with echoti, then I would recommend setting TERM to xterm. In future I will anyway refactor the code so this would work fine on CI environments that don't set TERM to a reasonable value.

[ehaupt]

So it makes sense to delay this till its really needed.

Sure.

The exit 0 is required because the script exits with a non zero signal causing the build framework to immediately fail. Changing TERM in the build environment is no option.

[poetaman]

@ehaupt With this change: https://github.com/poetaman/arttime/commit/6716e6b536f4861b8f8a28d44e380a9f3a035d22, installer would not exit with error due to missing term capability. So hopefully you would not require patching with exit 0 anymore... Let me know if you need anything else to avoid patching install.sh. I think one more thing would be to avoid printing some messages as they won't make sense, especially ones that ask user to add some directory to PATH or fpath.

[ehaupt]

Confirmed. https://github.com/poetaman/arttime/commit/6716e6b536f4861b8f8a28d44e380a9f3a035d22 won't require me patching in an exit 0 in the future. Thanks!