Skip to content

Migrating installation script to python#121

Closed
leopoldhub wants to merge 14 commits into
TamtamHero:mainfrom
leopoldhub:true-pip-installation
Closed

Migrating installation script to python#121
leopoldhub wants to merge 14 commits into
TamtamHero:mainfrom
leopoldhub:true-pip-installation

Conversation

@leopoldhub
Copy link
Copy Markdown
Collaborator

@leopoldhub leopoldhub commented Mar 12, 2025

The objective is to move most of the installation script logic to Python for several reasons:

  • Improved readability and maintainability
  • Easier for package maintainers to work with
  • Enables standalone uninstallation/reinstallation

Currently, uninstalling the project REQUIRES to have a working version of the repository to run the uninstallation script, which may confuse users wanting to uninstalling it.

This also means that we have to support legacy uninstallation and cleanup processes because we can't be sure which version the user has.

With a standalone uninstallation, users could uninstall whenever, and every version would know how to uninstall itself.

These changes also means that the project could be packaged as a standalone pip package, and be added to pip compatible repositories.

It could then be installed fully using:

  • pip install fw-fanctrl
  • fw-fanctrl-setup run [optional arguments...]

Proposed Solution

We will keep the manual installation script for convenience. However, all installation and uninstallation processes will be managed by a dedicated Python module.

The manual script will serve as an interface to:

  • Inform the user about missing dependencies
  • Build the project
  • Install the Python module
  • Run the Python-based installation script

This makes it easier for package maintainers that wants to do things manually, without running the manual script, while also keeping it simple for others.

The installation script itself will be a separate command (fw-fanctrl-setup) solely responsible for installing and uninstalling dependencies, configuration, and services.

Note: There isn't a solution to selectively disable the Python installation script when using pip install without involving a setup.py. Package maintainers should manually remove or disable the setup script executable if necessary.


At this stage, things are messy or incomplete as this is only a POC.

I would love to hear about your takes and propositions on this!

@leopoldhub leopoldhub self-assigned this Mar 12, 2025
@leopoldhub leopoldhub requested a review from TamtamHero March 12, 2025 03:28
@leopoldhub leopoldhub added documentation Improvements or additions to documentation enhancement New feature or request help wanted Extra attention is needed labels Mar 12, 2025
@leopoldhub leopoldhub linked an issue Mar 12, 2025 that may be closed by this pull request
@leopoldhub leopoldhub force-pushed the true-pip-installation branch from c94b312 to 741d065 Compare April 5, 2025 12:14
@leopoldhub leopoldhub marked this pull request as ready for review April 5, 2025 13:51
@leopoldhub
Copy link
Copy Markdown
Collaborator Author

Hi @Svenum, @icedream,

I have updated and tested these changes and I think it is done for now.

As asked in #122, I would like to get some feedback on how this affects your packages and fix the potential quirks together.

Have a nice day!

@leopoldhub leopoldhub added this to the Standardization milestone Apr 5, 2025
@icedream
Copy link
Copy Markdown

icedream commented Apr 5, 2025

I'll modify the PKGBUILD on my end later to build from this and see what effects it introduces.

@Svenum
Copy link
Copy Markdown
Contributor

Svenum commented Apr 6, 2025

I got this error, when I try to build from your branch:

warning: ignoring the client-specified setting 'trusted-public-keys', because it is a restricted setting and you are not a trusted user                                 
error: builder for '/nix/store/dn174mid8aqlvcjbnwydxjbkvyrlknjs-python3.12-fw-fanctrl-0-unstable-2025-04-03.drv' failed with exit code 1;                               
       last 25 log lines:                                                                                                                                               
       >     return self._get_build_requires(config_settings, requirements=[])                                                                                          
       >            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^                                                                                          
       >   File "/nix/store/n6hz86ikn0vjf4lpvgh3gjc91djwgc25-python3.12-setuptools-75.8.2/lib/python3.12/site-packages/setuptools/build_meta.py", line 304, in _get_build_requires                                                                                                                                                              
       >     self.run_setup()                                                                                                                                           
       >   File "/nix/store/n6hz86ikn0vjf4lpvgh3gjc91djwgc25-python3.12-setuptools-75.8.2/lib/python3.12/site-packages/setuptools/build_meta.py", line 320, in run_setup
       >     exec(code, locals())                                                                                                                                       
       >   File "<string>", line 1, in <module>                                                                                                                         
       >   File "/nix/store/n6hz86ikn0vjf4lpvgh3gjc91djwgc25-python3.12-setuptools-75.8.2/lib/python3.12/site-packages/setuptools/__init__.py", line 117, in setup      
       >     return distutils.core.setup(**attrs)                                                                                                                       
       >            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^                                                                                                                       
       >   File "/nix/store/n6hz86ikn0vjf4lpvgh3gjc91djwgc25-python3.12-setuptools-75.8.2/lib/python3.12/site-packages/setuptools/_distutils/core.py", line 160, in setup                                                                                                                                                                       
       >     dist.parse_config_files()                                                                                                                                  
       >   File "/nix/store/n6hz86ikn0vjf4lpvgh3gjc91djwgc25-python3.12-setuptools-75.8.2/lib/python3.12/site-packages/setuptools/dist.py", line 652, in parse_config_files                                                                                                                                                                     
       >     pyprojecttoml.apply_configuration(self, filename, ignore_option_errors)                                                                                    
       >   File "/nix/store/n6hz86ikn0vjf4lpvgh3gjc91djwgc25-python3.12-setuptools-75.8.2/lib/python3.12/site-packages/setuptools/config/pyprojecttoml.py", line 72, in apply_configuration                                                                                                                                                     
       >     config = read_configuration(filepath, True, ignore_option_errors, dist)                                                                                    
       >              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^                                                                                    
       >   File "/nix/store/n6hz86ikn0vjf4lpvgh3gjc91djwgc25-python3.12-setuptools-75.8.2/lib/python3.12/site-packages/setuptools/config/pyprojecttoml.py", line 140, in read_configuration                                                                                                                                                     
       >     validate(subset, filepath)                                                                                                                                 
       >   File "/nix/store/n6hz86ikn0vjf4lpvgh3gjc91djwgc25-python3.12-setuptools-75.8.2/lib/python3.12/site-packages/setuptools/config/pyprojecttoml.py", line 61, in validate                                                                                                                                                                
       >     raise ValueError(f"{error}\n{summary}") from None                                                                                                          
       > ValueError: invalid pyproject.toml config: `project`.                                                                                                          
       > configuration error: `project` must not contain {'license-files'} properties                                                                                   
       >                                                                                                                                                                
       > ERROR Backend subprocess exited when trying to invoke get_requires_for_build_wheel                                                                             
       For full logs, run:                                                                                                                                              
         nix log /nix/store/dn174mid8aqlvcjbnwydxjbkvyrlknjs-python3.12-fw-fanctrl-0-unstable-2025-04-03.drv

@leopoldhub
Copy link
Copy Markdown
Collaborator Author

configuration error: project must not contain {'license-files'} properties

The old licence file declaration is now deprecated in recent setuptools versions but I guess it is not yet supported in the version you are using...

I will revert this change for compatibility 👍

@Svenum
Copy link
Copy Markdown
Contributor

Svenum commented Apr 6, 2025

I am using the Version: 75.8.2

@leopoldhub
Copy link
Copy Markdown
Collaborator Author

I am using the Version: 75.8.2

Deprecated from 77.x.x and onward

@leopoldhub
Copy link
Copy Markdown
Collaborator Author

Should be good now

@Svenum
Copy link
Copy Markdown
Contributor

Svenum commented Apr 6, 2025

Works now 👍

@icedream
Copy link
Copy Markdown

icedream commented Apr 7, 2025

I'll modify the PKGBUILD on my end later to build from this and see what effects it introduces.

Will need more time for this later this evening unfortunately.

@leopoldhub
Copy link
Copy Markdown
Collaborator Author

Will need more time for this later this evening unfortunately.

No problem, it is not urgent, take your time ^^

@icedream
Copy link
Copy Markdown

icedream commented Apr 13, 2025

I'm finally able to get to this, currently dealing with this error when I use this branch in PKGBUILD:

Script installation path is '/usr/bin/fw-fanctrl'
which: no fw-fanctrl-setup in (/usr/lib/distcc/bin:/usr/lib/distcc/bin:/home/icedream/.node_modules/bin:/home/icedream/.yarn/bin:/home/icedream/bin:/home/icedream/.local/bin:/opt/google-cloud-cli/bin:/usr/local/bin:/usr/bin:/bin:/usr/local/sbin:/opt/android-sdk/platform-tools:/var/lib/flatpak/exports/bin:/usr/lib/jvm/default/bin:/usr/bin/site_perl:/usr/bin/vendor_perl:/usr/bin/core_perl:/opt/rocm/bin:/home/icedream/.config/composer/vendor/bin:/home/icedream/.local/share/gem/ruby/3.3.0/bin:/usr/lib/ruby/gems/3.3.0/bin:/home/icedream/go/bin)
Script installation setup path is ''
./install.sh: line 201: fw-fanctrl-setup: command not found
  1. Is fw-fanctrl-setup meant to be packaged?
  2. Is it meant to be used when preparing a package?
  3. How does this new setup intend for package maintainers to ship default configuration files?

Comment thread doc/commands.md Outdated
| --no-ectool | yes | | false | disable ectool installation |
| --no-pre-uninstall | yes | | false | disable pre-uninstall process |
| --no-post-install | yes | | false | disable post-install process |
| --no-battery-sensors | yes | | false | disable checking battery temperature sensors |
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Can battery sensor check also be adjusted at runtime instead of install time somehow?

Copy link
Copy Markdown
Collaborator Author

@leopoldhub leopoldhub Apr 13, 2025

Choose a reason for hiding this comment

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

Sure we can consider switching to a runtime configuration instead.

There was a reason at the time we went with a configuration at installation, so I will check it out and open a new PR.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Here is the relevant comment about our choice to use a cli option instead of a configuration:
#69 (comment)

[...] At the moment, when we deal with instance configurations, we do send them via the run command (e.g. hardware-controller, socket-controller, silent...).
I am open to imagining a new standard way of configuring this via the configuration file, but to ensure uniformity, I think we should do it this way for now. (feel free to discuss this in a new issue 😉) [...]

I believe it still is relevant, is there no way to add configuration options in the package?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

What won't work is if the install.sh script, which is called into during package build time to generate and install config files, hard-switches the feature. Some users may not have a battery installed in their system (I own such a Framework 13 system) and some do, and respectively just expect the package to work with that. Either version of the systemd unit will break for one of the cases. A user isn't expecting that they have to override a systemd unit to enable/disable a feature when there's already a configuration file in place to do it conveniently in. They'll look at the configuration file, try to figure out an option, and find that none exists. Even with it being documented, they have to still go out of their way to at least look up the error message and somehow find to some piece of documentation that explicitly points this out - which as far as I can tell doesn't exist yet either. The worst case is that they don't immediately notice and run into the effect of #154 once more and overheat their system with a failing loop.

Many package managers, including the one Arch uses, have no intended interface at all to ask the user "what features do you want enabled?" or "what version of the systemd unit file do you want to have?" when installing the package. They either install a package, or they don't - the files will then be installed, or they aren't. Then they run some install/uninstall hooks - they may print a message to hint at a thing, and otherwise run non-interactively. The most I can do at that point is make the install hook print a big warning about the systemd unit needing adjustment depending on whether a battery is in your system or not, but there's a chance users still miss it if they just run the package manager in non-interactive/batch mode (e.g. yay --no-confirm) and then step away from the computer to let it do its thing.

In cases where additional files need to be installed to make use of a feature (e.g. via a separate module) packages are split into multiple, e.g. to provide additional localization like LibreOffice does (libreoffice-fresh would then be the main package and libreoffice-fresh-de would then be the German files package) - but one package can't just override the files (including the systemd units) of another package, that'd be a conflict; and it's also additional complexity for the package as now combinations of package installs need to be tested. So I'd rather stay away from using this module approach unless there's a really good reason to do it. I don't see this being a legitimate approach for fw-fanctrl.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the clarification!

Sure then, I can either add it as a runtime configuration, or simply detect automatically if the battery is present or not and act accordingly.

Comment thread pyproject.toml
@leopoldhub
Copy link
Copy Markdown
Collaborator Author

Hi, thanks for finding the time to work on this project ^^

  1. Is fw-fanctrl-setup meant to be packaged?
  2. Is it meant to be used when preparing a package?
  3. How does this new setup intend for package maintainers to ship default configuration files?

1 & 2

It is meant to be used by the package to install and uninstall necessary files.

In a package installation context, it makes no sense to give the end user access to it, so the executable can be excluded.

However, the package manager will need to call the module to uninstall the existing version.

I do not know much about package lifecycles, so if it is not coherent, please let me know.

3

Using the fw-fanctrl-setup run command with the relevant options to your context

@leopoldhub leopoldhub force-pushed the true-pip-installation branch from 081151b to 92bdadf Compare April 16, 2025 14:51
# Conflicts:
#	README.md
#	install.sh
#	services/system-sleep/fw-fanctrl-suspend
@leopoldhub leopoldhub requested a review from icedream April 30, 2025 08:45
@icedream
Copy link
Copy Markdown

icedream commented Jul 5, 2025

In a package installation context, it makes no sense to give the end user access to it, so the executable can be excluded.

However, the package manager will need to call the module to uninstall the existing version.

To be clear, we're talking about the package installation/uninstallation process calling into the script, not the package build process having to call into the script, as far as I understand you?

Any script or executable that has to be called on the system of a user installing/uninstalling the package effectively has to become part of the package one way or another. It can't be excluded from installation because then any installation hook trying to call it won't be able to find the file.

Thanks for clarifying!

@leopoldhub
Copy link
Copy Markdown
Collaborator Author

Hi @icedream , thanks you for your time.

To be clear, we're talking about the package installation/uninstallation process calling into the script, not the package build process having to call into the script, as far as I understand you?

Yes, we do not touch the the build process.

Any script or executable that has to be called on the system of a user installing/uninstalling the package effectively has to become part of the package one way or another

Thank you for the explanation!
So, I cannot reserve the installation/uninstallation script for package managers...

Do you have any suggestions on how to include the installation and uninstallation processes while avoiding accidental user interaction?

Or is the idea of letting the package determine its own uninstallation process a bad idea?

@leopoldhub
Copy link
Copy Markdown
Collaborator Author

After much consideration and lack of free time, I took the decision to not proceed further with those changes.

It was more a quality of life ("it would be neat") than a necessity, and the current installation process is quite straightforward and simple.

If someone want to take back from where I left, feel free to do so, and I will happily welcome your contribution into the project 😉

Have a nice day!

@leopoldhub leopoldhub closed this Sep 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation enhancement New feature or request help wanted Extra attention is needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[PROPOSAL] Migrating installation script to python

3 participants