feat(install): add specific options for packaging environments#138
feat(install): add specific options for packaging environments#138tulilirockz wants to merge 2 commits intoTamtamHero:mainfrom
Conversation
I wanted to use the install script so I didnt need to handle the config files and systemd units, but I ran into a few issues because the install script kept replacing the DEST_DIR with a location in `%{buildroot}` on the RPM build, this should add a few options to make this more seamless for weird aah enviroments like this.
- `no-pip-build`: You'd build the wheel previously on a package
- `no-override-python-installation-location`: This breaks RPM builds cuz it finds %{BUILDROOT} on the script and it craps itself
tulilirockz
changed the title
leopoldhub
left a comment
leopoldhub
left a comment
There was a problem hiding this comment.
Hi, thanks for your contribution!
I realized I did a mistake in a previous PR (more details in the review) causing you and others trouble which I will fix in a separate PR.
In the meantime, here is some feedback.
| ;; | ||
| '--no-pip-build') | ||
| NO_PIP_BUILD=true | ||
| NO_PIP_INSTALL=true |
There was a problem hiding this comment.
Users should be able to install pre-built packages, so I don't think we should set NO_PIP_INSTALL=true
| done | ||
|
|
||
| if ! python -h 1>/dev/null 2>&1; then | ||
| if ! python -h 1>/dev/null 2>&1 && [ "$NO_PIP_BUILD" = false ] ; then |
There was a problem hiding this comment.
same here, python will be necessary when installing without building
if ! python -h 1>/dev/null 2>&1 && { [ "$NO_PIP_BUILD" = false ] || [ "$NO_PIP_INSTALL" = false ]; }
| fi | ||
|
|
||
| if [ "$NO_PIP_INSTALL" = false ]; then | ||
| if [ "$NO_PIP_INSTALL" = false ] && [ "$NO_PIP_BUILD" != false ]; then |
There was a problem hiding this comment.
here too, we should be able to install if NO_PIP_INSTALL=false
if [ "$NO_PIP_INSTALL" = false ];
| if [ "$NO_OVERRIDE_PYTHON_INSTALLATION_PATH" = true ] ; then | ||
| OLD_SYSCONFDIR=$SYSCONFDIR | ||
| OLD_INSTALL_PATH="${PYTHON_SCRIPT_INSTALLATION_PATH}" | ||
| PYTHON_SCRIPT_INSTALLATION_PATH="/usr/bin/fw-fanctrl" | ||
| SYSCONF_DIR="/etc" | ||
| fi |
There was a problem hiding this comment.
The python script installation path deduction by default introduced in #116 was a mistake as it causes the issue you ran into, as well as #129.
I think the correct thing to do is remove the PYTHON_SCRIPT_INSTALLATION_PATH deduction for now and find a proper solution as a replacement (specify the path in an option? deduce only if an option is passed?...)
You can remove this PYTHON_SCRIPT_INSTALLATION_PATH and SYSCONF_DIR manual replacement for now and expect the PYTHON_SCRIPT_INSTALLATION_PATH deduction to be gone for now.
| if [ "$NO_OVERRIDE_PYTHON_INSTALLATION_PATH" = true ] ; then | ||
| OLD_SYSCONFDIR=$SYSCONFDIR | ||
| OLD_INSTALL_PATH="${PYTHON_SCRIPT_INSTALLATION_PATH}" | ||
| PYTHON_SCRIPT_INSTALLATION_PATH="/usr/bin/fw-fanctrl" | ||
| SYSCONF_DIR="/etc" | ||
| fi |
There was a problem hiding this comment.
same here, expect the PYTHON_SCRIPT_INSTALLATION_PATH deduction to be gone
|
Also, could you please document the |
|
I believe #139 will fix the issue with the python script installation path override |
|
#139 merged, |
|
Heyo! Thank you for seeing this PR! I was just upstreaming some fixes I made for the Bazzite package. Ill update this one to fit the things you reviewed here, just a bit busy with other stuff right now! |
2 participants
I wanted to use the install script so I didnt need to handle the config files and systemd units, but I ran into a few issues because the install script kept replacing the DEST_DIR with a location in
%{buildroot}on the RPM build, this should add a few options to make this more seamless for weird enviroments like this.no-pip-build: You'd build the wheel previously on a packageno-override-python-installation-location: This fixes a break on RPM builds cuz it finds %{BUILDROOT} on the script and it craps itself