Stop using BuildPackages.sh

[antonio-rojas]

Loading packagemanager fails if the dir lib/gap/bin doesn't exist (for instance, if gap has been installed somewhere via make install and is not being run from the source tree).

gap> LoadPackage("PackageManager");
Error, no method found! For debugging hints type ?Recovery from NoMethodFound
Error, no 1st choice method found for `Filename' on 2 arguments
The 1st argument is 'fail' which might point to an earlier problem
 at /usr/share/gap/lib/methsel2.g:249 called from
<function "HANDLE_METHOD_NOT_FOUND">( <arguments> )
 called from read-eval loop at /usr/lib/gap/pkg/packagemanager/gap/PackageManager.gd:295
type 'quit;' to quit to outer loop

Simply creating an empty lib/gap/bin subdir makes packagemanager load and work fine, so I assume its presence is not essential. Loading the package should not fail if the dir doesn't exist.

[dimpase]

Moreover, if using GAP installed with make install, no need for lib/gap/bin to exist at all!

[dimpase]

Amusingly, creating lib/gap/bin cures #106 (sic!)

[fingolfin]

Hmm, isn't the actual problem that BuildPackages.sh does not get installed by make install, but PackageManager wants to use it? As such, I am surprised to hear that it "works fine" with just creating that bin dir. I mean, I do see that it would fix this error message, but wouldn't it still fail later on when trying to install a package that needs compilations...?!?

For GAP.jl I deal with this problem by installing a copy of BuildPackages.sh in a bin directory in an additional GAPROOT. I am not suggesting you should necessarily do that, just that it was the trivial quick&dirty solution for me as I already had that extra GAPROOT for other reasons anyway, and wanted something working now, and worry about a proper fix later... so it's good you opened this issue and we can start the worrying here now ;-).

But a better fix would be to bundle a copy of BuildPackages.sh with PackageManager...

... and even better would be to not use that script at all! It contains a lot of complexity that is unnecessary for PackageManager. E.g. it creates its own log files, deals with multiple packages etc. etc. -- but the relevant code is really tiny.

Let's look at it a bit closer... but first let me point out that also e.g. https://github.com/gap-actions/build-pkg/blob/main/action.yml has code with a similar role which is MUCH more compact...

Looking at BuildPackages.sh , the first relevant part is this function:

run_configure_and_make() {
  # We want to know if this is an autoconf configure script
  # or not, without actually executing it!
  if [[ -x autogen.sh && ! -x configure ]]
  then
    ./autogen.sh
  fi
  if [[ -x configure ]]
  then
    if grep Autoconf ./configure > /dev/null
    then
      local PKG_NAME=$($GAP -q -T -A -M <<GAPInput
Read("PackageInfo.g");
Print(GAPInfo.PackageInfoCurrent.PackageName);
GAPInput
)
      local CONFIG_ARGS_FLAG_NAME="`PACKAGE_CONFIG_ARGS`_${PKG_NAME}"
      echo_run ./configure --with-gaproot="$GAPROOT" $CONFIGFLAGS ${!CONFIG_ARGS_FLAG_NAME}
      echo_run "$MAKE" clean
    else
      echo_run ./configure "$GAPROOT"
      echo_run "$MAKE" clean
      echo_run ./configure "$GAPROOT"
    fi
    echo_run "$MAKE"
  else
    notice "No building required for $PKG"
  fi
}

Note that there is a lot of stuff in there we don't need for PackageManager or could do better...

• the whole thing could in fact be replaced by GAP code, that'd make things much easier to understand and debug
• no need to extract the package name and PACKAGE_CONFIG_ARGS_* values here (if we want to specify custom flags, we should just directly pass them to configure
• the message No building required for $PKG is unnecessary
• with a freshly downloaded source tarball, there is no need for the ./configure && make clean && ./configure dance (this is a workaround to supporting re-running BuildPackages.sh on the same package directory multiple times)

The second relevant part is this, I guess:

build_one_package() {
  # requires one argument which is the package directory
  PKG="$1"
  [[ $GITHUB_ACTIONS = true ]] && echo "::group::$PKG"
  echo ""
  date
  echo ""
  notice "==== Checking $PKG"
  (  # start subshell
  set -e
  cd "$CURDIR/$PKG"
  if [[ -x prerequisites.sh ]]
  then
    ./prerequisites.sh "$GAPROOT"
  elif [[ -x build-normaliz.sh ]]
  then
    # used in NormalizInterface; to be replaced by prerequisites.sh in future
    # versions
    ./build-normaliz.sh "$GAPROOT"
  fi
  case "$PKG" in
    # All but the last lines should end by '&&', otherwise (for some reason)
    # some packages that fail to build will not get reported in the logs.
    atlasrep*)
      chmod 1777 datagens dataword
    ;;

    pargap*)
      echo_run ./configure --with-gap="$GAPROOT" && \
      echo_run "$MAKE" && \
      cp bin/pargap.sh "$GAPROOT/bin" && \
      rm -f ALLPKG
    ;;

    xgap*)
      echo_run ./configure --with-gaproot="$GAPROOT" && \
      echo_run "$MAKE" && \
      rm -f "$GAPROOT/bin/xgap.sh" && \
      cp bin/xgap.sh "$GAPROOT/bin"
    ;;

    simpcomp*)
      # Old versions of simpcomp were not setting the executable
      # bit for some files; they also were not copying the bistellar
      # executable to the right place
      (chmod a+x configure depcomp install-sh missing || :) && \
      run_configure_and_make && \
      mkdir -p bin && test -x bin/bistellar || mv bistellar bin
    ;;

    *)
      run_configure_and_make
    ;;
  esac
  ) &&
  (  # start subshell
  if [[ $GITHUB_ACTIONS = true ]]
  then
    echo "::endgroup::"
  fi
  ) || build_fail
}

Well...

• pargap and simpcomp specific code is not needed anymore
• arguably neither is the xgap specific code
• atlasrep: dealing with the data directory of it is another long term issue... but when installing atlasrep for all users, of course we can't make its package directory world writable! And IMHO it's not a great model in general either... I am going to open an issue about this on the GAP issue tracker later on)
• we don't the '::endgroup::` etc. messages (those are for GitHub Action workflows)
• the NormalizInterface should be made redundant by a new release of NormalizInterface (I'll work on a PR separately), but it shouldn't matter for downstream packager who will surely want it to use a system wide Normaliz installation anyway, and that should work "out of the box" (famous last words 😂)

If someone would like to help make this happen, a good start would be to write a minimal replacement shell script for BuildPackage.sh based on https://github.com/gap-actions/build-pkg/blob/main/action.yml and/or a much simplified version of the BuildPackage.sh code based on my comments above; this should be put into the PackageManager repository and then the GAP code be tweaked to reference that new script, instead of BuildPackage.sh. This should be relatively easy, and requires minimal GAP knowledge and mostly shell scripting capabilities.

In a second update this script could be replaced by equivalent GAP code calling out to subprocesses, but this requires a lot more work; plus I really think we should first extend GAP to support capturing stderr of subprocesses (see #104 and gap-system/gap#4657).

[dimpase]

we don't run in BuildPackages.sh in Sage, we have our own contraption to deal with it.

[dimpase]

we can reuse it to do the simplification you propose.

But now I have to deal with my son getting hold of a non-empty ink bottle and splashing it over himself 🤦

[fingolfin]

Regarding NormalizInterface, of course I was wrong sigh one has to explicitly tell it to use the system wide Normaliz; there is an old issue to improve that, but I just could never be bothered (also, strictly speaking, not every Normaliz version will work, as Normaliz tends to introduce breaking changes to its API quote often).

Oh and I submitted gap-packages/NormalizInterface#109

[fingolfin]

I did not suggest that SageMath should use BuildPackages.sh. Rather I suggested that PackageManager should stop using it 😂

[antonio-rojas]

Hmm, isn't the actual problem that BuildPackages.sh does not get installed by make install, but PackageManager wants to use it? As such, I am surprised to hear that it "works fine" with just creating that bin dir. I mean, I do see that it would fix this error message, but it would still fails later on when trying to install a package that needs compilations...?!?

Indeed, I had only tested non-binary packages.

[mtorpey]

I can definitely see advantages in a PackageManager-specific alternative to BuildPackages.sh. I originally chose to use it to cut down on moving parts that could break, and avoid code duplication. However, this isn't the first time that locating and interacting with that script has caused problems. And as @fingolfin points out, the relevant code would surely not be too complicated.

I've changed the issue title to make the goal more precise. I think it'd be worth doing.

[mtorpey]

Note that the package manager currently has a special bit of code for handling prerequisites.sh files. This could all be rolled into a bespoke script!

[dimpase]

I'm confused - when we e.g. use GAP installed with make install, there is no BuildPackages.sh around any more, as it's not installed anywhere. Yet packagemanager GAP package works.

[antonio-rojas]

I'm confused - when we e.g. use GAP installed with make install, there is no BuildPackages.sh around any more, as it's not installed anywhere. Yet packagemanager GAP package works.

It doesn't work with packages that need compilation

[dimpase]

OTOH, if packagemanager uses a built-in location for BuildPackages.sh, and it happens to be invoked from a GAP source directory, then indeed it has BuildPackages.sh available for it, and this might be the root cause of this bug. Either install it, and use it from the installed location (hoping it does the correct thing, although YMMV), or drop it.