Improved command line argument parsing (#725)

* An update to PR 715, making the handling of the *--mount-point* parameter less error prone.
I added a synomym (accepting the name both with underscore and dash) and ignoring when no value specified
I added it explicitly to the list to accept both the --parm value and --parm=value syntax
DOES NOT check the contents of the parameter
* Explicitly set all the know parameters
* Define explictly all parameters.
Make all non explicitly defined parameters behave as standard parameters, with on exception, names are not changed
Some cleanup of the code
In guided.py the reference to the dry_run parameter is updated to the standard naming convention for parameters
* Linter with flake8. corrections
* Linter with flake8. corrections (II)
* Linter with flake8. corrections (and III)
* Added --disk_layout argument. Was missing
I moved its loading from guided.py to __init__.py as it happens to the other json related arguments
* Better handling of errors during processing of the --disk_layouts parameter.
I define a routine to read an store a JSON file or stream. Tested on disk_layout
* Expand the former commit to all JSON file arguments
* Moved the function we created to read json files/streams to general.py. Add some comments
* flake8. A reference now unneded
* The merge process for the dry-run argument was causing the issue, not solving it
The del is just a cleanup for version upgrade without consequence (I hope)
* flake8 warning
* Correcting the last correction . Worked for old config files, but only for them
* New parameter parsing algorithm. More flexible and accepts multiple arguments (optionallY)
plus some documentation effort
* flake8 warning. For once is significant ( != None to not None)

2 files changed, 144 insertions, 31 deletions

diff --git a/archinstall/__init__.py b/archinstall/__init__.py
index b0c938ad..865e9844 100644
--- a/archinstall/__init__.py
+++ b/archinstall/__init__.py
@@ -28,64 +28,149 @@ __version__ = "2.3.1.dev0"
 storage['__version__'] = __version__
 
 
-def initialize_arguments():
-	config = {}
+def define_arguments():
+	"""
+	Define which explicit arguments do we allow.
+	Refer to https://docs.python.org/3/library/argparse.html for documentation and
+			https://docs.python.org/3/howto/argparse.html for a tutorial
+	Remember that the property/entry name python assigns to the parameters is the first string defined as argument and
+	dashes inside it '-' are changed to '_'
+	"""
 	parser.add_argument("--config", nargs="?", help="JSON configuration file or URL")
 	parser.add_argument("--creds", nargs="?", help="JSON credentials configuration file")
+	parser.add_argument("--disk_layouts","--disk_layout","--disk-layouts","--disk-layout",nargs="?",
+					help="JSON disk layout file")
 	parser.add_argument("--silent", action="store_true",
 						help="WARNING: Disables all prompts for input and confirmation. If no configuration is provided, this is ignored")
-	parser.add_argument("--dry-run", action="store_true",
+	parser.add_argument("--dry-run","--dry_run",action="store_true",
 						help="Generates a configuration file and then exits instead of performing an installation")
 	parser.add_argument("--script", default="guided", nargs="?", help="Script to run for installation", type=str)
+	parser.add_argument("--mount-point","--mount_point",nargs="?",type=str,help="Define an alternate mount point for installation")
+	parser.add_argument("--debug",action="store_true",help="Adds debug info into the log")
+	parser.add_argument("--plugin",nargs="?",type=str)
+
+def parse_unspecified_argument_list(unknowns :list, multiple :bool = False, error :bool = False) -> dict:
+	"""We accept arguments not defined to the parser. (arguments "ad hoc").
+	Internally argparse return to us a list of words so we have to parse its contents, manually.
+	We accept following individual syntax for each argument
+		--argument value
+		--argument=value
+		--argument = value
+		--argument   (boolean as default)
+	the optional paramters to the function alter a bit its behaviour:
+	* multiple allows multivalued arguments, each value separated by whitespace. They're returned as a list
+	* error. If set any non correctly specified argument-value pair to raise an exception. Else, simply notifies the existence of a problem and continues processing.
+
+	To a certain extent, multiple and error are incompatible. In fact, the only error this routine can catch, as of now, is the event
+	argument value value ...
+	which isn't am error if multiple is specified
+	"""
+	tmp_list = unknowns[:]   # wastes a few bytes, but avoids any collateral effect of the destructive nature of the pop method()
+	config = {}
+	key = None
+	last_key = None
+	while tmp_list:
+		element = tmp_list.pop(0)			  # retreive an element of the list
+		if element.startswith('--'):		   # is an argument ?
+			if '=' in element:				 # uses the arg=value syntax ?
+				key, value = [x.strip() for x in element[2:].split('=', 1)]
+				config[key] = value
+				last_key = key				 # for multiple handling
+				key = None					 # we have the kwy value pair we need
+			else:
+				key = element[2:]
+				config[key] = True   # every argument starts its lifecycle as boolean
+		else:
+			if element == '=':
+				continue
+			if key:
+				config[key] = element
+				last_key = key # multiple
+				key = None
+			else:
+				if multiple and last_key:
+					if isinstance(config[last_key],str):
+						config[last_key] = [config[last_key],element]
+					else:
+						config[last_key].append(element)
+				elif error:
+					raise ValueError(f"Entry {element} is not related to any argument")
+				else:
+					print(f" We ignore the entry {element} as it isn't related to any argument")
+	return config
+
+def get_arguments():
+	""" The handling of parameters from the command line
+	Is done on following steps:
+	0) we create a dict to store the arguments and their values
+	1) preprocess.
+		We take those arguments which use Json files, and read them into the argument dict. So each first level entry becomes a argument un it's own right
+	2) Load.
+		We convert the predefined argument list directly into the dict vía the vars() función. Non specified arguments are loaded with value None or false if they are booleans (action="store_true").
+		The name is chosen according to argparse conventions. See above (the first text is used as argument name, but underscore substitutes dash)
+		We then load all the undefined arguments. In this case the names are taken as written.
+		Important. This way explicit command line arguments take precedence over configuración files.
+	3) Amend
+		Change whatever is needed on the configuration dictionary (it could be done in post_process_arguments but  this ougth to be left to changes anywhere else in the code, not in the arguments dictionary
+	"""
+	config = {}
 	args, unknowns = parser.parse_known_args()
+	# preprocess the json files.
+	# TODO Expand the url access to the other JSON file arguments ?
 	if args.config is not None:
 		try:
 			# First, let's check if this is a URL scheme instead of a filename
 			parsed_url = urllib.parse.urlparse(args.config)
 
 			if not parsed_url.scheme:  # The Profile was not a direct match on a remote URL, it must be a local file.
-				with open(args.config) as file:
-					config = json.load(file)
+				if not json_stream_to_structure('--config',args.config,config):
+					exit(1)
 			else:  # Attempt to load the configuration from the URL.
 				with urllib.request.urlopen(urllib.request.Request(args.config, headers={'User-Agent': 'ArchInstall'})) as response:
-					config = json.loads(response.read())
+					config.update(json.loads(response.read()))
 		except Exception as e:
 			raise ValueError(f"Could not load --config because: {e}")
 
 		if args.creds is not None:
-			with open(args.creds) as file:
-				config.update(json.load(file))
-
-		# Installation can't be silent if config is not passed
+			if not json_stream_to_structure('--creds',args.creds,config):
+				exit(1)
+	# load the parameters. first the known, then the unknowns
+	config.update(vars(args))
+	config.update(parse_unspecified_argument_list(unknowns))
+	# amend the parameters (check internal consistency)
+	# Installation can't be silent if config is not passed
+	if args.config is not None :
 		config["silent"] = args.silent
+	else:
+		config["silent"] = False
 
-	for arg in unknowns:
-		if '--' == arg[:2]:
-			if '=' in arg:
-				key, val = [x.strip() for x in arg[2:].split('=', 1)]
-			else:
-				key, val = arg[2:], True
-			config[key] = val
-
-	config["script"] = args.script
+	# avoiding a compatibility issue
+	if 'dry-run' in config:
+		del config['dry-run']
+	return config
 
-	if args.dry_run is not None:
-		config["dry-run"] = args.dry_run
+def post_process_arguments(arguments):
+	storage['arguments'] = arguments
+	if arguments.get('mount_point'):
+		storage['MOUNT_POINT'] = arguments['mount_point']
 
-	return config
+	if arguments.get('debug',False):
+		log(f"Warning: --debug mode will write certain credentials to {storage['LOG_PATH']}/{storage['LOG_FILE']}!", fg="red", level=logging.WARNING)
 
+	from .lib.plugins import plugins, load_plugin # This initiates the plugin loading ceremony
+	if arguments.get('plugin', None):
+		load_plugin(arguments['plugin'])
 
-arguments = initialize_arguments()
-storage['arguments'] = arguments
-if arguments.get('debug'):
-	log(f"Warning: --debug mode will write certain credentials to {storage['LOG_PATH']}/{storage['LOG_FILE']}!", fg="red", level=logging.WARNING)
-if arguments.get('mount-point'):
-	storage['MOUNT_POINT'] = arguments['mount-point']
+	if arguments.get('disk_layouts', None) is not None:
+		if 'disk_layouts' not in storage:
+			storage['disk_layouts'] = {}
+		if not json_stream_to_structure('--disk_layouts',arguments['disk_layouts'],storage['disk_layouts']):
+			exit(1)
 
-from .lib.plugins import plugins, load_plugin # This initiates the plugin loading ceremony
 
-if arguments.get('plugin', None):
-	load_plugin(arguments['plugin'])
+define_arguments()
+arguments = get_arguments()
+post_process_arguments(arguments)
 
 # TODO: Learn the dark arts of argparse... (I summon thee dark spawn of cPython)
 
diff --git a/archinstall/lib/general.py b/archinstall/lib/general.py
index 7c8f8ea3..cc50e80a 100644
--- a/archinstall/lib/general.py
+++ b/archinstall/lib/general.py
@@ -481,3 +481,31 @@ def run_custom_user_commands(commands, installation):
 		execution_output = SysCommand(f"arch-chroot {installation.target} bash /var/tmp/user-command.{index}.sh")
 		log(execution_output)
 		os.unlink(f"{installation.target}/var/tmp/user-command.{index}.sh")
+
+def json_stream_to_structure(id : str, stream :str, target :dict) -> bool :
+	""" Function to load a stream (file (as name) or valid JSON string into an existing dictionary
+	Returns true if it could be done
+	Return  false if operation could not be executed
+	+id is just a parameter to get meaningful, but not so long messages
+	"""
+	from pathlib import Path
+	if Path(stream).exists():
+		try:
+			with open(Path(stream)) as fh:
+				target.update(json.load(fh))
+		except Exception as e:
+			log(f"{id} = {stream} does not contain a valid JSON format: {e}",level=logging.ERROR)
+			return False
+	else:
+		log(f"{id} = {stream} does not exists in the filesystem. Trying as JSON stream",level=logging.DEBUG)
+		# NOTE: failure of this check doesn't make stream 'real' invalid JSON, just it first level entry is not an object (i.e. dict), so it is not a format we handle.
+		if stream.strip().startswith('{') and stream.strip().endswith('}'):
+			try:
+				target.update(json.loads(stream))
+			except Exception as e:
+				log(f" {id} Contains an invalid JSON format : {e}",level=logging.ERROR)
+				return False
+		else:
+			log(f" {id} is neither a file nor is a JSON string:",level=logging.ERROR)
+			return False
+	return True