microsoft / microsoft-r-open

Microsoft R Open Source
212 stars 69 forks source link

Unnecessary post-scripts in RPM packages of MRO #20

Closed iavael closed 6 years ago

iavael commented 7 years ago

In microsoft-r-open-mro-3.3 and microsoft-r-open-mkl-3.3 rpm-s there're unnecessary scripts in pre/post sections, see comments

microsoft-r-open-mro-3.3:

postinstall scriptlet (using /bin/sh):

#!/bin/bash
VERSION="3.3"
INSTALL_PREFIX="${RPM_INSTALL_PREFIX}/microsoft-r/${VERSION}"

mkdir -p ${INSTALL_PREFIX}/lib64/R/backup/lib # Should be done during rpm build, not during install
mv ${INSTALL_PREFIX}/lib64/R/lib/*.so ${INSTALL_PREFIX}/lib64/R/backup/lib # Same here
cp ${INSTALL_PREFIX}/lib64/R/backup/lib/libR.so ${INSTALL_PREFIX}/lib64/R/lib # Same here
cp ${INSTALL_PREFIX}/stage/mkl_install_stage/*.so ${INSTALL_PREFIX}/lib64/R/lib # Same here
preuninstall scriptlet (using /bin/sh):
#!/bin/bash
VERSION="3.3"
DIRECTORY="${RPM_INSTALL_PREFIX}/microsoft-r/${VERSION}/lib64/R"
if [ -d "$DIRECTORY" ]; then
  rm ${DIRECTORY}/lib/*.so # Same here
  mv ${DIRECTORY}/backup/lib/*.so ${DIRECTORY}/lib # Same here
fi

So whole pre and post sections are unnecessary, beacuse everything they do could (moreover should!) be done during build. One mistake in "rm" statement and you destroy user's system like in https://github.com/MrMEEE/bumblebee-Old-and-abbandoned/issues/123

postinstall scriptlet (using /bin/sh):

#!/bin/bash

VERSION="3.3"
INSTALL_PREFIX="${RPM_INSTALL_PREFIX}/microsoft-r/${VERSION}"

ln -s "${INSTALL_PREFIX}/lib64/R/bin/R" /usr/bin/R # Should be done through "alternatives" mechanism
ln -s "${INSTALL_PREFIX}/lib64/R/bin/Rscript" /usr/bin/Rscript # same here

rm /bin/sh # WHAT!!! This is insane! Never touch files of other packages please
ln -s /bin/bash /bin/sh
preuninstall scriptlet (using /bin/sh):
#!/bin/bash

VERSION=3.3
INSTALL_PREFIX="${RPM_INSTALL_PREFIX}/microsoft-r/${VERSION}"

rm /usr/bin/R # See comment about alternatives
rm /usr/bin/Rscript # same here

rm -rf "${INSTALL_PREFIX}/lib64/R/backup" # Should be done during rpm build, not during install
postuninstall scriptlet (using /bin/sh):
#!/bin/bash

# remove if empty, leave if not empty
rmdir "${RPM_INSTALL_PREFIX}/microsoft-r" 2>/dev/null # RPM will do this for you automatically

These pre/post sections can be reduced to couple of update-alternatives calls. And playing with /bin/sh risking of cripple user's system is wrong from any point of view, please never do this.

nathansoz commented 7 years ago

We will look at these comments and make changes. Particularly:

-Adding update-alternatives support instead of symlinking ourselves in -The /bin/sh stuff is due to Ubuntu symlinking dash to /bin/sh and that reacting badly with both the R shell wrapper and some Microsoft R Server features. I believe we fixed the R shell wrapper features last release by explicitly requesting bash and we will be fixing the R server features next release. 3.3.3 should be updated with these fixes.

iavael commented 7 years ago

Adding update-alternatives support instead of symlinking ourselves in

I investigated a bit more. If there's already R package in distro and it doesn't support update-alternatives mechanism (in Fedora/CentOS/RHEL it doesn't), then alternatives won't help anyway, but direct manipulation with /usr/bin/R and /usr/bin/Rscript will break distro packages if they are installed. It's better to pick one of variants:

The /bin/sh stuff is due to Ubuntu symlinking dash to /bin/sh...

Well, in RPM-based systems (at least in RHEL-based like RHEL itself, CentOS and Fedora, I don't remember about SuSE-based) /bin/sh is already symlink to /bin/bash, so you can avoid replacing in in MRO's rpm packages. For debian-based systems there is dpkg-divert mechanism [1][2] which can help you replace /bin/sh without conflicting distro's settings. Fixing R shell wrapper looks as better option but maybe you can use provided info for fastfix.

[1] https://www.debian.org/doc/debian-policy/ap-pkg-diversions.html [2] https://wiki.debian.org/Adding%20and%20removing%20diversions

voronoipotato commented 6 years ago

https://lobste.rs/s/btycfp/microsoft_s_failed_attempt_at_debian

This issue was reffered to on Lobsters which was discussing this article https://www.preining.info/blog/2018/06/microsofts-failed-attempt-on-debian-packaging/ . Perhaps you could reach out to someone for help on this.

richcalaway commented 6 years ago

We have re-released MRO 3.5.0 with new mro postinst scripts that use the alternatives approach; most of the other changes had been made for MRO 3.3.3.