Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhacements to scripts for developers (II) #1383

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

wllacer
Copy link
Contributor

@wllacer wllacer commented Jul 18, 2022

This is a followup of PR #1377 (which it of course, includes) I decided for two different PR as i think this one could be a bit more controversial and merits separate (and probably more intense) review.
The most important change is a more or less thoroughly reorder of guided.py code. I've made almost any single action (or a closely related group) as an independent routine ( called set_* for changes at the host level and setup_* for those at the target level, and a series of groups of this activities ( usually perform_*) to manage them as blocks. A lot in the way of classic Pascal code (at least as we were taught in the early 80's)
Why ?

  • Improve readability. YMMV, but the abstraction it provides simplifies the understanding of what the code is doing, and the flow of it
  • Improve internal maintanability. it's easier to locate where is the action we need to modify, or change the order of certain actions
  • Easier patching. guided.py is the area where (at least me) gets the maximum number of collisions during merges, and the most difficult to sort out. I believe this new structure will limit them, or at least make them easier to resolve
  • Improve flexibility (see below)
  • Improve reusability (see below)

Against this advantages, it could have also have disadvantages i've tried to minimize

  • slighter longer code
  • argument handling. Thanks $DEITY we have a "global memory area" (archinstall.arguments and archinstall.storage) and a ll-rounder central object instance ( installation), which makes that in, almost all, cases (the setup_* routines) only need the last as function argument.
  • the intermediate routines (those which direct the workflow of actions) can be controversial in it scope, but now it becomes easier to test new arrangements
  • future maintenance ... as long as we can stay with this structure, it's easy to expand functionally. But, well ... we are only humans
  • the first time merge:. With over 30 PR waiting on the queue, it's very probable that some will impact guided.py code. This have to be ported manually, i'm afraid

Why it is more flexible.

A greater level of modularization allows for easier coding of alternate routes. I had at swiss.py defined a new application argument (--mode) which allows only to execute a subset of functionality. I've ported this over guided.py and this is all the code I needed:

def perform_installation(mountpoint,mode='full'):
	"""
	This is the main installation routine
	Performs the installation steps on a block device.
	Only requirement is that the block devices are
	formatted and setup prior to entering this function.
	Planned modes:
	Full install (default)
	Recover      (pacstrap and minimal setup. No user definition. WIP
	disk         only disk layout(
	software     software installation, not storage management
	"""
	with archinstall.Installer(mountpoint, kernels=archinstall.arguments.get('kernels', ['linux'])) as installation:
		# Mount all the drives to the desired mountpoint
		if mode.lower() in ('full','disk'):
			perform_partition_management(installation)
		# setup host environment
		if mode.lower() != 'disk':
			set_ntp_environment(installation)
			set_pacstrap_mirrors()
			# Retrieve list of additional repositories and set boolean values appropriately
			enable_testing = 'testing' in archinstall.arguments.get('additional-repositories',[])
			enable_multilib = 'multilib' in archinstall.arguments.get('additional-repositories',[])
			if mode.lower() != 'software':
				if not installation.minimal_installation(testing=enable_testing, multilib=enable_multilib):
					return
				if mode.lower() != 'recover':
					perform_basic_setup(installation)
					setup_users(installation)
			if mode.lower() != 'recover':
				perform_additional_software_setup(installation)
			# This step must be after profile installs to allow profiles to install language pre-requisits.
			# After which, this step will set the language both for console and x11 if x11 was installed for instance.
			installation.set_keyboard_language(archinstall.arguments.get('keyboard-layout','us'))
			# If the user provided custom commands to be run post-installation, execute them now.
			if archinstall.arguments.get('custom-commands', None):
				archinstall.run_custom_user_commands(archinstall.arguments['custom-commands'], installation)

			installation.genfstab()

			installation.log("For post-installation tips, see https://wiki.archlinux.org/index.php/Installation_guide#Post-installation", fg="yellow")
			if not archinstall.arguments.get('silent') and mode == 'full':
				prompt = str(_('Would you like to chroot into the newly created installation and perform post-installation configuration?'))
				choice = Menu(prompt, Menu.yes_no(), default_option=Menu.yes()).run()
				if choice == Menu.yes():
					try:
						installation.drop_to_shell()
					except:
						pass

As of now, this is still not a thing you should call directly at guided.py but thru swiss.py (UI is still not ready for this, and when it is done, most of swiss won't be necessary) or other scripts.

How to improve reusability

Developing scripts based on guided.py is a necessity and a nightmare.
A necessity to provide special interfaces (as now minimal, only_hd and swiss, do, or should do) or to create semi automatic test suits for functionality we're developing (as long it's not UI related). Also third party users, could built their own solutions based on it.
But a nightmare, first because knowing exactly and reusing what you need or not is not that easy with the current codebase. And it becomes obsolete or worse at any change at guided as it is not automatically carried over. Only_hd and swiss are constantly playing catch-up and half-broken, and usually involves manual upgrades. Minimal is not working since time immemorial ...
the new structure should allow to minimize this effects, as we can call instead of copying guided.py reference implementation.
To start a script we only need to create a module with the structure

import archinstall
from archinstall.examples.guided import *   # this is the fastest way. only during development. be more explicit 
if __name__ in ('__main__', this_script):
     mode = archinstall.arguments.get('mode',the_mode_I_need_in_this_case)
     # copy the contents of the same section of guided. This is inavoidable
     ...

Then you simply have to adapt (copy and modify) exactly the parts you need to change (well the call hierarchy) (usually ask_questions at least)
All the rest will be maintained thru one single copy. that of guided.py

I've included updated code for the general scripts. minimal isn't exactly what it was in origin, but the new implementation is a very good testing environment and an ideal piece of code to start making your own scripts

@wllacer wllacer requested a review from Torxed as a code owner July 18, 2022 10:46
@wllacer
Copy link
Contributor Author

wllacer commented Jul 18, 2022

Before you start complaining perform_installation could also be written as

def perform_installation(mountpoint,mode='full'):
	"""
	This is the main installation routine
	Performs the installation steps on a block device.
	Only requirement is that the block devices are
	formatted and setup prior to entering this function.
	Planned modes:
	Full install (default)
	Recover      (pacstrap and minimal setup. No user definition
	disk         only disk layout(
	software     software
	"""
	with archinstall.Installer(mountpoint, kernels=archinstall.arguments.get('kernels', ['linux'])) as installation:
		enable_testing = 'testing' in archinstall.arguments.get('additional-repositories', [])
		enable_multilib = 'multilib' in archinstall.arguments.get('additional-repositories', [])
		match mode.lower():
			case 'full':
				perform_partition_management(installation)
				set_ntp_environment(installation)
				set_pacstrap_mirrors()
				if not installation.minimal_installation(testing=enable_testing, multilib=enable_multilib):
					return
				perform_basic_setup(installation)
				setup_users(installation)
				perform_additional_software_setup(installation)
			case 'disk':
				perform_partition_management(installation)
			case 'software':
				set_pacstrap_mirrors()
				if not installation.minimal_installation(testing=enable_testing, multilib=enable_multilib):
					return
				perform_basic_setup(installation)
				setup_users(installation)
				perform_additional_software_setup(installation)
			case 'recover':
				archinstall.log(f"mode {mode} not implemeted")
				exit(1)
				if not installation.minimal_installation(testing=enable_testing, multilib=enable_multilib):
					return
				perform_basic_setup(installation)
			case _:
				archinstall.log(f"mode {mode} not existant")
				exit(1)

		if archinstall.arguments.get('custom-commands', None):
				archinstall.run_custom_user_commands(archinstall.arguments['custom-commands'], installation)
		
		installation.genfstab()

		installation.log("For post-installation tips, see https://wiki.archlinux.org/index.php/Installation_guide#Post-installation", fg="yellow")

		if not archinstall.arguments.get('silent') and mode == 'full':
			prompt = str(_('Would you like to chroot into the newly created installation and perform post-installation configuration?'))
			choice = Menu(prompt, Menu.yes_no(), default_option=Menu.yes()).run()
			if choice == Menu.yes():
				try:
					installation.drop_to_shell()
				except:
					pass

is a bit more verbose and has some duplicate code, but is more clear on what exactly each mode does. Which one do you prefer ?

@wllacer wllacer mentioned this pull request Jul 21, 2022
10 tasks
@Torxed
Copy link
Member

Torxed commented Aug 9, 2022

There's some important changes in here that I want to merge.
But I have to read through everything as it's a bit of a big change.
I'll play around with it (and sync in some changes from master as I fixed some bugs that stopped me from testing)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants