[pacman-dev] Use absolute path for pacman and pacman-conf in makepkg

Message ID DB7PR10MB2508FAC53911DB211A98412AD2A50@DB7PR10MB2508.EURPRD10.PROD.OUTLOOK.COM
State New
Headers show
Series
  • [pacman-dev] Use absolute path for pacman and pacman-conf in makepkg
Related show

Commit Message

Wouter Wijsman May 7, 2020, 6:13 p.m. UTC
Currently makepkg requires pacman and pacman-conf to be in the path of
the user. Since these executables should never move, it should be safe
to add the full paths to the scripts at compile time, assuming the user
uses the install command as well.

This change works for both autotools and meson.

Signed-off-by: Wouter Wijsman <wwijsman@live.nl>
---
 build-aux/edit-script.sh.in | 1 +
 meson.build                 | 1 +
 scripts/Makefile.am         | 3 ++-
 scripts/makepkg.sh.in       | 6 ++++--
 4 files changed, 8 insertions(+), 3 deletions(-)

Comments

Allan McRae May 11, 2020, 2:42 a.m. UTC | #1
On 8/5/20 4:13 am, Wouter Wijsman wrote:
> Currently makepkg requires pacman and pacman-conf to be in the path of
> the user. Since these executables should never move, it should be safe
> to add the full paths to the scripts at compile time, assuming the user
> uses the install command as well.
> 
> This change works for both autotools and meson.
> 
> Signed-off-by: Wouter Wijsman <wwijsman@live.nl>
> ---

Hi,

Can you let us know more detail about the use case for this patch?  Why
would the user not add the directory pacman and scripts are installed in
to their path?

I have concerns that hardcoding paths on build will lead to difficulty
when in the future we have a test suite for makepkg.

Allan


>  build-aux/edit-script.sh.in | 1 +
>  meson.build                 | 1 +
>  scripts/Makefile.am         | 3 ++-
>  scripts/makepkg.sh.in       | 6 ++++--
>  4 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/build-aux/edit-script.sh.in b/build-aux/edit-script.sh.in
> index 661c22d5..44103f08 100644
> --- a/build-aux/edit-script.sh.in
> +++ b/build-aux/edit-script.sh.in
> @@ -20,6 +20,7 @@ mode=$3
>    -e "s|@DEBUGSUFFIX[@]|@DEBUGSUFFIX@|g" \
>    -e "s|@INODECMD[@]|@INODECMD@|g" \
>    -e "s|@FILECMD[@]|@FILECMD@|g" \
> +  -e "s|@fullbindir[@]|@FULLBINDIR@|g" \
>    "$input" >"$output"
>  
>  if [[ $mode ]]; then
> diff --git a/meson.build b/meson.build
> index 680cf62b..92e8a9a5 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -257,6 +257,7 @@ substs.set('LOCALEDIR', LOCALEDIR)
>  substs.set('sysconfdir', SYSCONFDIR)
>  substs.set('localstatedir', LOCALSTATEDIR)
>  substs.set('PKGDATADIR', PKGDATADIR)
> +substs.set('FULLBINDIR', '@0@/@1@'.format(PREFIX, get_option('bindir')))
>  substs.set('PREFIX', PREFIX)
>  substs.set('BASH', BASH.path())
>  substs.set('PACKAGE_VERSION', PACKAGE_VERSION)
> diff --git a/scripts/Makefile.am b/scripts/Makefile.am
> index 47455ed2..a79e9389 100644
> --- a/scripts/Makefile.am
> +++ b/scripts/Makefile.am
> @@ -182,7 +182,8 @@ edit = sed \
>  	-e 's|@DEBUGSUFFIX[@]|$(DEBUGSUFFIX)|g' \
>  	-e "s|@INODECMD[@]|$(INODECMD)|g" \
>  	-e "s|@FILECMD[@]|$(FILECMD)|g" \
> -	-e 's|@SCRIPTNAME[@]|$@|g'
> +	-e 's|@SCRIPTNAME[@]|$@|g' \
> +	-e 's|@fullbindir[@]|$(FULLBINDIR)|g'
>  
>  ## All the scripts depend on Makefile so that they are rebuilt when the
>  ## prefix etc. changes. Use chmod -w to prevent people from editing the
> diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in
> index d1416d15..2f7b838e 100644
> --- a/scripts/makepkg.sh.in
> +++ b/scripts/makepkg.sh.in
> @@ -44,6 +44,8 @@ declare -r makepkg_version='@PACKAGE_VERSION@'
>  declare -r confdir='@sysconfdir@'
>  declare -r BUILDSCRIPT='@BUILDSCRIPT@'
>  declare -r startdir="$(pwd -P)"
> +declare -r PACMAN_EXECUTABLE='@fullbindir@/pacman'
> +declare -r PACMAN_CONF_EXECUTABLE='@fullbindir@/pacman-conf'
>  
>  LIBRARY=${LIBRARY:-'@libmakepkgdir@'}
>  
> @@ -235,7 +237,7 @@ run_pacman() {
>  		else
>  			cmd=(su root -c "$(printf '%q ' "${cmd[@]}")")
>  		fi
> -		local lockfile="$(pacman-conf DBPath)/db.lck"
> +		local lockfile="$(${PACMAN_CONF_EXECUTABLE} DBPath)/db.lck"
>  		while [[ -f $lockfile ]]; do
>  			local timer=0
>  			msg "$(gettext "Pacman is currently in use, please wait...")"
> @@ -1123,7 +1125,7 @@ unset var
>  PACKAGER=${PACKAGER:-"Unknown Packager"}
>  
>  # set pacman command if not already defined
> -PACMAN=${PACMAN:-pacman}
> +PACMAN=${PACMAN:-${PACMAN_EXECUTABLE}}
>  # save full path to command as PATH may change when sourcing /etc/profile
>  PACMAN_PATH=$(type -P $PACMAN)
>  
>
Wouter Wijsman May 11, 2020, 1:01 p.m. UTC | #2
On Mon, 2020-05-11 at 12:42 +1000, Allan McRae wrote:
> On 8/5/20 4:13 am, Wouter Wijsman wrote:
> > Currently makepkg requires pacman and pacman-conf to be in the path
> > of
> > the user. Since these executables should never move, it should be
> > safe
> > to add the full paths to the scripts at compile time, assuming the
> > user
> > uses the install command as well.
> > 
> > This change works for both autotools and meson.
> > 
> > Signed-off-by: Wouter Wijsman <wwijsman@live.nl>
> > ---
> 
> Hi,
> 
> Can you let us know more detail about the use case for this
> patch?  Why
> would the user not add the directory pacman and scripts are installed
> in
> to their path?
> 
> I have concerns that hardcoding paths on build will lead to
> difficulty
> when in the future we have a test suite for makepkg.
> 
> Allan

Hi,

I've since realized that this patch is not really needed. It was
supposed to go together with another patch which didn't end up working.
Sorry for the spam.

I'm working on making pacman manage libraries for the Playstation
Portable in a homebrew PSP software development kit. For this to work
on both systems which already have pacman and systems which do not, I
initially tried to make pacman build with different binary names. This
approach had many issues and would require forking, which I'm not keen
on doing. The Playstation Vita homebrew SDK and the devkit pro
(multiple systems, mostly Nintendo) also use pacman, but they have
these forks which are old and have some bugs not found in upstream
pacman.

Now I'm working on a different solution, which is to change the bindir
in the meson options while building (to keep the binaries out of the
path of the user) and using wrapper scripts for pacman and makepkg. Now
I realize that I can just set the path in the wrapper script and be
done with the issue this patch was supposed to solve.

Right now an unpatched pacman almost works for my use case, but there
still is one issue with building it. Currently ninja will install the
bash-completion scripts system wide, regardless of which user runs
ninja install or what the prefix is. Only if bash-completion is not
available, will the scripts be installed inside the prefix. The code
which makes this happen is here: 
https://git.archlinux.org/pacman.git/tree/meson.build#n46

This is why I submitted the patch which makes bash-completion optional.
It may need a different solution, though. Would you take a patch which
adds an option to install the bash-completion scripts inside the
prefix? I'd prefer not to maintain any private patches if that's
possible.

Kind regards,
Wouter

P.S. For the people interested in the wrapper, here is a link: 
https://github.com/sharkwouter/psp-pacman-1/tree/upstream-pr

The pacman.sh script builds and installs pacman. The wrapper scripts
are in scripts. If the user already has pacman, it doesn't build it at
all and the wrapper will use the system's pacman with a different
configuration, root and dbpath.
Eli Schwartz May 11, 2020, 1:12 p.m. UTC | #3
On 5/11/20 9:01 AM, wwijsman@live.nl wrote:
> Right now an unpatched pacman almost works for my use case, but there
> still is one issue with building it. Currently ninja will install the
> bash-completion scripts system wide, regardless of which user runs
> ninja install or what the prefix is. Only if bash-completion is not
> available, will the scripts be installed inside the prefix. The code
> which makes this happen is here: 
> https://git.archlinux.org/pacman.git/tree/meson.build#n46
> 
> This is why I submitted the patch which makes bash-completion optional.
> It may need a different solution, though. Would you take a patch which
> adds an option to install the bash-completion scripts inside the
> prefix? I'd prefer not to maintain any private patches if that's
> possible.

https://git.archlinux.org/users/eschwartz/pacman.git/commit/?h=queue2&id=b4e3e1c7d93463dffdbb63ff78008df0f57780db

It's simple enough to do, I've just been trying to decide whether I
think it is the right solution before proposing it.

Patch

diff --git a/build-aux/edit-script.sh.in b/build-aux/edit-script.sh.in
index 661c22d5..44103f08 100644
--- a/build-aux/edit-script.sh.in
+++ b/build-aux/edit-script.sh.in
@@ -20,6 +20,7 @@  mode=$3
   -e "s|@DEBUGSUFFIX[@]|@DEBUGSUFFIX@|g" \
   -e "s|@INODECMD[@]|@INODECMD@|g" \
   -e "s|@FILECMD[@]|@FILECMD@|g" \
+  -e "s|@fullbindir[@]|@FULLBINDIR@|g" \
   "$input" >"$output"
 
 if [[ $mode ]]; then
diff --git a/meson.build b/meson.build
index 680cf62b..92e8a9a5 100644
--- a/meson.build
+++ b/meson.build
@@ -257,6 +257,7 @@  substs.set('LOCALEDIR', LOCALEDIR)
 substs.set('sysconfdir', SYSCONFDIR)
 substs.set('localstatedir', LOCALSTATEDIR)
 substs.set('PKGDATADIR', PKGDATADIR)
+substs.set('FULLBINDIR', '@0@/@1@'.format(PREFIX, get_option('bindir')))
 substs.set('PREFIX', PREFIX)
 substs.set('BASH', BASH.path())
 substs.set('PACKAGE_VERSION', PACKAGE_VERSION)
diff --git a/scripts/Makefile.am b/scripts/Makefile.am
index 47455ed2..a79e9389 100644
--- a/scripts/Makefile.am
+++ b/scripts/Makefile.am
@@ -182,7 +182,8 @@  edit = sed \
 	-e 's|@DEBUGSUFFIX[@]|$(DEBUGSUFFIX)|g' \
 	-e "s|@INODECMD[@]|$(INODECMD)|g" \
 	-e "s|@FILECMD[@]|$(FILECMD)|g" \
-	-e 's|@SCRIPTNAME[@]|$@|g'
+	-e 's|@SCRIPTNAME[@]|$@|g' \
+	-e 's|@fullbindir[@]|$(FULLBINDIR)|g'
 
 ## All the scripts depend on Makefile so that they are rebuilt when the
 ## prefix etc. changes. Use chmod -w to prevent people from editing the
diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in
index d1416d15..2f7b838e 100644
--- a/scripts/makepkg.sh.in
+++ b/scripts/makepkg.sh.in
@@ -44,6 +44,8 @@  declare -r makepkg_version='@PACKAGE_VERSION@'
 declare -r confdir='@sysconfdir@'
 declare -r BUILDSCRIPT='@BUILDSCRIPT@'
 declare -r startdir="$(pwd -P)"
+declare -r PACMAN_EXECUTABLE='@fullbindir@/pacman'
+declare -r PACMAN_CONF_EXECUTABLE='@fullbindir@/pacman-conf'
 
 LIBRARY=${LIBRARY:-'@libmakepkgdir@'}
 
@@ -235,7 +237,7 @@  run_pacman() {
 		else
 			cmd=(su root -c "$(printf '%q ' "${cmd[@]}")")
 		fi
-		local lockfile="$(pacman-conf DBPath)/db.lck"
+		local lockfile="$(${PACMAN_CONF_EXECUTABLE} DBPath)/db.lck"
 		while [[ -f $lockfile ]]; do
 			local timer=0
 			msg "$(gettext "Pacman is currently in use, please wait...")"
@@ -1123,7 +1125,7 @@  unset var
 PACKAGER=${PACKAGER:-"Unknown Packager"}
 
 # set pacman command if not already defined
-PACMAN=${PACMAN:-pacman}
+PACMAN=${PACMAN:-${PACMAN_EXECUTABLE}}
 # save full path to command as PATH may change when sourcing /etc/profile
 PACMAN_PATH=$(type -P $PACMAN)