Skip to content

Feat/reorder inst dirs preference order#45

Closed
cgoesche wants to merge 2 commits intoovh:mainfrom
cgoesche:feat/reorder_inst_dirs_preference_order
Closed

Feat/reorder inst dirs preference order#45
cgoesche wants to merge 2 commits intoovh:mainfrom
cgoesche:feat/reorder_inst_dirs_preference_order

Conversation

@cgoesche
Copy link

@cgoesche cgoesche commented Sep 30, 2025

Description

This patch modifies the logic used to determine the
preference order of installation directories. It'll
look like this:

  1. -b PATH
  2. ${BINDIR}
  3. ${XDG_BIN_HOME}
  4. ${HOME}/.local/bin
  5. ./bin

Fixes #42 (issue)

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)
  • Improvement (improvement of existing commands)

Checklist:

  • My code follows the style guidelines of this project

Signed-off-by: Christian Goeschel Ndjomouo cgoesc2@wgu.edu

@cgoesche cgoesche requested a review from a team as a code owner September 30, 2025 17:44
Copy link
Collaborator

@amstuta amstuta left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution !

@cgoesche cgoesche force-pushed the feat/reorder_inst_dirs_preference_order branch from bacb1ec to b28d2a9 Compare October 1, 2025 12:08
install.sh Outdated
BINDIR=${BINDIR:-./bin}
for dir in "${BINDIR}" "${XDG_BIN_HOME}" "${HOME}/.local/bin" "./bin"; do
# As final fallback we use './bin', and create it in execute(), if non-existent.
if [ -d "${dir}" ] || [ "${dir}" = "./bin" ]; then
Copy link
Member

@maxatome maxatome Oct 1, 2025

Choose a reason for hiding this comment

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

if you want to do this, keep it simple:

  • remove ./bin from the for list
  • set BINDIR=bin before the for
  • then:
    Suggested change
    if [ -d "${dir}" ] || [ "${dir}" = "./bin" ]; then
    if [ -d "$dir" ]; then

Copy link
Member

@maxatome maxatome Oct 1, 2025

Choose a reason for hiding this comment

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

I amend my comment as I didn't see $BINDIR in for list at first sight:

  • remove ./bin from the for list
  • then
    Suggested change
    if [ -d "${dir}" ] || [ "${dir}" = "./bin" ]; then
    if [ -d "$dir" ]; then
  • after done do : "${BINDIR:=bin}"

Copy link
Author

Choose a reason for hiding this comment

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

However, if $BINDIR is set and the directory does not exist, it will never be set to bin. Of course, only if "${XDG_BIN_HOME}" and "${HOME}/.local/bin" also do not exist.

In execute() I see that we simply create $BINDIR if it doesn't exist. If that is the desired behavior we might as well just:

  • test -n "${BINDIR}" and "${XDG_BIN_HOME}"
  • use ${HOME}/.local/bin if above two are null
  • try creation of $BINDIR, if that fails set $BINDIR to ./bin
  • try install "${tmpdir}/${BINARY}" "${BINDIR}/"

@cgoesche cgoesche force-pushed the feat/reorder_inst_dirs_preference_order branch from b28d2a9 to 19f2dd5 Compare October 1, 2025 17:05
This patch modifies the logic used to determine the
preference order of installation directories. It'll
look like this:

1. -b PATH
2. ${BINDIR}
3. ${XDG_BIN_HOME}
4. ${HOME}/.local/bin
5. ./bin

If the target directory does not exist, an attempt to create
it will be made. And in case that fails, the script will default
to './bin'.

Addresses: ovh#42
Signed-off-by: Christian Goeschel Ndjomouo <cgoesc2@wgu.edu>
Signed-off-by: Christian Goeschel Ndjomouo <cgoesc2@wgu.edu>
@amstuta
Copy link
Collaborator

amstuta commented Oct 2, 2025

We chose to go with another proposal because it is simpler and we were not fans of the fallback for the directory creation, but thanks for your contribution @cgoesche !

@amstuta amstuta closed this Oct 2, 2025
@cgoesche
Copy link
Author

cgoesche commented Oct 2, 2025

Hey @amstuta ,
I think that the other proposal now provides an inconsistent mechanism, i.e.:

  • $BINDIR is accepted even if the directory does not exist
  • $XDG_BIN_HOME and $HOME/.local/bin are NOT accepted if they do not exist
  • an attempt is being made to create $BINDIR if non-existent
  • NO attempt is being made to create $XDG_BIN_HOME or $HOME/.local/bin if non-existent

Is this the expected behavior ?

@amstuta
Copy link
Collaborator

amstuta commented Oct 2, 2025

Yes that's what we expect, we would prefer avoid creating $XDG_BIN_HOME and $HOME/.local/bin.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Install dir is not the one being advertised

3 participants