Add pre-commit and fix findings (#601)

* Add pre-commit

* pre-commit auto-fixes

* Flake8 fixes

* Update docs

* Correctly import mkdir_p_sudo_safe()

* more

* Remove pre-commit action in favour of pre-commit ci

Co-authored-by: Jannis Leidel <jannis@leidel.info>

* Revert "Remove pre-commit action in favour of pre-commit ci"

This reverts commit f42f079.

* Switch to shellcheck-py

* Add badge

* Disable autofix PRs

Co-authored-by: Jannis Leidel <jannis@leidel.info>

.github/workflows/main.yml

@@ -104,7 +104,7 @@ jobs:
         if: startsWith(matrix.os, 'windows')
         shell: cmd
         run: |
-          set "CONSTRUCTOR_SIGNING_CERTIFICATE=${{ runner.temp }}\certificate.pfx" 
+          set "CONSTRUCTOR_SIGNING_CERTIFICATE=${{ runner.temp }}\certificate.pfx"
           set "CONSTRUCTOR_PFX_CERTIFICATE_PASSWORD=1234"
           powershell scripts\create_self_signed_certificate.ps1
           copy /Y "%CONSTRUCTOR_SIGNING_CERTIFICATE%" examples\signing\certificate.pfx

.gitignore

@@ -9,3 +9,4 @@ rever/
 .idea
 docs/build/
 .vscode/
+build/

.pre-commit-config.yaml

@@ -0,0 +1,27 @@
+# disable autofixing PRs, commenting "pre-commit.ci autofix" on a pull request triggers a autofix
+ci:
+    autofix_prs: false
+exclude: ^(versioneer.py|constructor/_version.py|CONSTRUCT.md)
+repos:
+  - repo: https://github.com/pre-commit/pre-commit-hooks
+    rev: v4.4.0
+    hooks:
+      - id: check-added-large-files
+      - id: check-ast
+      - id: fix-byte-order-marker
+      - id: check-case-conflict
+      - id: check-merge-conflict
+      - id: debug-statements
+      - id: detect-private-key
+      - id: mixed-line-ending
+      - id: end-of-file-fixer
+      - id: trailing-whitespace
+  - repo: https://github.com/PyCQA/flake8
+    rev: 6.0.0
+    hooks:
+      - id: flake8
+  - repo: https://github.com/shellcheck-py/shellcheck-py
+    rev: v0.9.0.2
+    hooks:
+      - id: shellcheck
+        exclude: ^(constructor/header.sh|constructor/osx/.*sh)

CONSTRUCT.md

@@ -218,8 +218,8 @@ as `sh` on Linux and `exe` on Windows.
 
 Notes for silent mode `/S` on Windows EXEs:
 - NSIS Silent mode will not print any error message, but will silently abort the installation.
-  If needed, [NSIS log-builds][nsis-log] can be used to print to `%PREFIX%\install.log`, which can be
-  searched for `::error::` strings. Pre- and post- install scripts will only throw an error
+  If needed, [NSIS log-builds][nsis-log] can be used to print to `%PREFIX%\install.log`, which
+  can be searched for `::error::` strings. Pre- and post- install scripts will only throw an error
   if the environment variable `NSIS_SCRIPTS_RAISE_ERRORS` is set.
 - The `/D` flag can be used to specify the target location. It must be the last argument in
   the command and should NEVER be quoted, even if it contains spaces. For example:
@@ -687,7 +687,7 @@ Allowed keys are:
     - `include_text` (optional bool, default=`False`): Whether to dump the license text in the JSON.
       If false, only the path will be included.
     - `text_errors` (optional str, default=`None`): How to handle decoding errors when reading the
-      license text. Only relevant if include_text is True. Any str accepted by open()'s 'errors' 
+      license text. Only relevant if include_text is True. Any str accepted by open()'s 'errors'
       argument is valid. See https://docs.python.org/3/library/functions.html#open.
 
 

README.md

@@ -1,5 +1,7 @@
 # (conda) Constructor
 
+[![pre-commit.ci status](https://results.pre-commit.ci/badge/github/conda/constructor/main.svg)](https://results.pre-commit.ci/latest/github/conda/constructor/main)
+
 ## Description
 
 Constructor is a tool which allows constructing an installer

constructor/conda_interface.py

@@ -9,6 +9,8 @@
 from os.path import join
 import datetime
 
+from conda.gateways.disk import mkdir_p_sudo_safe
+
 from constructor.utils import hash_files
 
 NAV_APPS = ['glueviz', 'jupyterlab', 'notebook',
@@ -75,7 +77,7 @@
     distro = None
     if sys.platform.startswith('linux'):
         try:
-            from conda._vendor import distro
+            from conda._vendor import distro  # noqa
         except ImportError:
             pass
 
@@ -93,7 +95,8 @@ def get_repodata(url):
         else:
             raise NotImplementedError("unsupported version of conda: %s" % CONDA_INTERFACE_VERSION)
 
-        # noarch-only repos are valid. In this case, the architecture specific channel will return None
+        # noarch-only repos are valid. In this case, the architecture specific channel will
+        # return None
         if raw_repodata_str is None:
             full_repodata = {
                 '_url': url,
@@ -159,8 +162,9 @@ def write_repodata(cache_dir, url, full_repodata, used_packages, info):
             fh.write(repodata)
 
         # set the modification time to mod_time. needed for mamba
-        mod_time_datetime = datetime.datetime.strptime(mod_time,
-            "%a, %d %b %Y %H:%M:%S %Z")
+        mod_time_datetime = datetime.datetime.strptime(
+            mod_time, "%a, %d %b %Y %H:%M:%S %Z"
+        )
         mod_time_s = int(mod_time_datetime.timestamp())
         os.utime(repodata_filepath, times=(mod_time_s, mod_time_s))
 

constructor/construct.py

@@ -177,8 +177,8 @@
 
 Notes for silent mode `/S` on Windows EXEs:
 - NSIS Silent mode will not print any error message, but will silently abort the installation.
-  If needed, [NSIS log-builds][nsis-log] can be used to print to `%PREFIX%\\install.log`, which can be
-  searched for `::error::` strings. Pre- and post- install scripts will only throw an error
+  If needed, [NSIS log-builds][nsis-log] can be used to print to `%PREFIX%\\install.log`, which
+  can be searched for `::error::` strings. Pre- and post- install scripts will only throw an error
   if the environment variable `NSIS_SCRIPTS_RAISE_ERRORS` is set.
 - The `/D` flag can be used to specify the target location. It must be the last argument in
   the command and should NEVER be quoted, even if it contains spaces. For example:
@@ -331,20 +331,20 @@
 `${HOME}/${NAME}`. On windows, this is used only for "Just Me" installation;
 for "All Users" installation, use the `default_prefix_all_users` key.
 If not provided, the default prefix is `${USERPROFILE}\${NAME}`.
-'''),
+'''),  # noqa
 
     ('default_prefix_domain_user', False, str, '''
 Set default installation prefix for domain user. If not provided, the
 installation prefix for domain user will be `${LOCALAPPDATA}\${NAME}`.
 By default, it is different from the `default_prefix` value to avoid installing
 the distribution in the roaming profile. Windows only.
-'''),
+'''),  # noqa
 
     ('default_prefix_all_users', False, str, '''
 Set default installation prefix for All Users installation. If not provided,
 the installation prefix for all users installation will be
 `${ALLUSERSPROFILE}\${NAME}`. Windows only.
-'''),
+'''),  # noqa
 
     ('default_location_pkg', False, str, '''
 Default installation subdirectory in the chosen volume. In PKG installers,
@@ -554,7 +554,7 @@
     - `include_text` (optional bool, default=`False`): Whether to dump the license text in the JSON.
       If false, only the path will be included.
     - `text_errors` (optional str, default=`None`): How to handle decoding errors when reading the
-      license text. Only relevant if include_text is True. Any str accepted by open()'s 'errors' 
+      license text. Only relevant if include_text is True. Any str accepted by open()'s 'errors'
       argument is valid. See https://docs.python.org/3/library/functions.html#open.
 '''),
 ]
@@ -613,11 +613,13 @@ def ns_platform(platform):
         win64=bool(p == 'win-64'),
     )
 
+
 # This regex is taken from https://github.com/conda/conda_build/metadata.py
 # The following function "select_lines" is also a slightly modified version of
 # the function of the same name from conda_build/metadata.py
 sel_pat = re.compile(r'(.+?)\s*(#.*)?\[([^\[\]]+)\](?(2)[^\(\)]*)$')
 
+
 def select_lines(data, namespace):
     lines = []
 

constructor/fcp.py

@@ -247,8 +247,8 @@ def _precs_from_environment(environment, download_dir, user_conda):
     return list(dict.fromkeys(PrefixGraph(precs).graph))
 
 
-def _solve_precs(name, version, download_dir, platform, channel_urls=(), channels_remap=(), specs=(),
-                 exclude=(), menu_packages=(), environment=None, environment_file=None,
+def _solve_precs(name, version, download_dir, platform, channel_urls=(), channels_remap=(),
+                 specs=(), exclude=(), menu_packages=(), environment=None, environment_file=None,
                  verbose=True, conda_exe="conda.exe", extra_env=False):
     # Add python to specs, since all installers need a python interpreter. In the future we'll
     # probably want to add conda too.
@@ -303,15 +303,14 @@ def _solve_precs(name, version, download_dir, platform, channel_urls=(), channel
         # the records are already returned in topological sort
         precs = list(solver.solve_final_state())
 
-
     python_prec = next((prec for prec in precs if prec.name == "python"), None)
     if python_prec:
         precs.remove(python_prec)
         precs.insert(0, python_prec)
     elif not extra_env:
         # the base environment must always have python; this has been addressed
-        # at the beginning of _main() but we can still get here through the
-        # environment_file option
+        # at the beginning of _main() but we can still get here through the
+        # environment_file option
         sys.exit("python MUST be part of the base environment")
 
     warn_menu_packages_missing(precs, menu_packages)
@@ -355,8 +354,11 @@ def _fetch_precs(precs, download_dir, transmute_file_type=''):
                 if os.path.exists(new_file_name):
                     continue
                 print("transmuting %s" % dist)
-                conda_package_handling.api.transmute(os.path.join(download_dir, dist),
-                    transmute_file_type, out_folder=download_dir)
+                conda_package_handling.api.transmute(
+                    os.path.join(download_dir, dist),
+                    transmute_file_type,
+                    out_folder=download_dir,
+                )
             else:
                 new_dists.append(dist)
         dists = new_dists
@@ -424,7 +426,7 @@ def _main(name, version, download_dir, platform, channel_urls=(), channels_remap
         print("Info: Skipping duplicate files checks because `extra_envs` in use")
         duplicate_files = "skip"
 
-    all_pc_recs = list({rec: None for rec in all_pc_recs}) # deduplicate
+    all_pc_recs = list({rec: None for rec in all_pc_recs})  # deduplicate
     approx_tarballs_size, approx_pkgs_size = check_duplicates_files(
         pc_recs, platform, duplicate_files=duplicate_files
     )
@@ -439,6 +441,7 @@ def _main(name, version, download_dir, platform, channel_urls=(), channels_remap
         extra_envs_data
     )
 
+
 def main(info, verbose=True, dry_run=False, conda_exe="conda.exe"):
     name = info["name"]
     version = info["version"]
@@ -459,9 +462,9 @@ def main(info, verbose=True, dry_run=False, conda_exe="conda.exe"):
     if not channel_urls and not channels_remap:
         sys.exit("Error: at least one entry in 'channels' or 'channels_remap' is required")
 
-    # We need to preserve the configuration for proxy servers and ssl, otherwise if constructor is running
-    # in a host that sits behind proxy (usually in a company / corporate environment) it will have this
-    # settings reset with the call to conda_replace_context_default
+    # We need to preserve the configuration for proxy servers and ssl, otherwise if constructor is
+    # running in a host that sits behind proxy (usually in a company / corporate environment) it
+    # will have this settings reset with the call to conda_replace_context_default
     # See: https://github.com/conda/constructor/issues/304
     proxy_servers = conda_context.proxy_servers
     ssl_verify = conda_context.ssl_verify
@@ -473,12 +476,34 @@ def main(info, verbose=True, dry_run=False, conda_exe="conda.exe"):
         conda_context.proxy_servers = proxy_servers
         conda_context.ssl_verify = ssl_verify
 
-        (pkg_records, _urls, dists, approx_tarballs_size, approx_pkgs_size,
-        has_conda, extra_envs_info) = _main(
-            name, version, download_dir, platform, channel_urls, channels_remap, specs,
-            exclude, menu_packages, ignore_duplicate_files, environment, environment_file,
-            verbose, dry_run, conda_exe, transmute_file_type, extra_envs, check_path_spaces
-        )
+        (
+            pkg_records,
+            _urls,
+            dists,
+            approx_tarballs_size,
+            approx_pkgs_size,
+            has_conda,
+            extra_envs_info,
+        ) = _main(
+            name,
+            version,
+            download_dir,
+            platform,
+            channel_urls,
+            channels_remap,
+            specs,
+            exclude,
+            menu_packages,
+            ignore_duplicate_files,
+            environment,
+            environment_file,
+            verbose,
+            dry_run,
+            conda_exe,
+            transmute_file_type,
+            extra_envs,
+            check_path_spaces,
+            )
 
     info["_all_pkg_records"] = pkg_records  # full PackageRecord objects
     info["_urls"] = _urls  # needed to mock the repodata cache

constructor/main.py

@@ -7,7 +7,7 @@
 from __future__ import absolute_import, division, print_function
 
 import os
-from os.path import abspath, basename, expanduser, isdir, join
+from os.path import abspath, expanduser, isdir, join
 import sys
 import argparse
 from textwrap import dedent, indent
@@ -123,7 +123,6 @@ def main_build(dir_path, output_dir='.', platform=cc_platform,
                 new_extras.append({orig: dest})
         info[extra_type] = new_extras
 
-
     for key in 'channels', 'specs', 'exclude', 'packages', 'menu_packages':
         if key in info:
             # ensure strings in those lists are stripped
@@ -170,9 +169,10 @@ def main_build(dir_path, output_dir='.', platform=cc_platform,
         info['_outpath'] = abspath(join(output_dir, get_output_filename(info)))
         create(info, verbose=verbose)
         print("Successfully created '%(_outpath)s'." % info)
-    
+
     process_build_outputs(info)
 
+
 class _HelpConstructAction(argparse.Action):
     def __init__(
         self,

constructor/nsis/UAC.nsh

@@ -22,7 +22,7 @@ Interactive User (MediumIL)        Admin user (HighIL)
 !ifndef UAC_HDR__INC
 !verbose push
 !verbose 3
-!ifndef UAC_VERBOSE  
+!ifndef UAC_VERBOSE
 	!define UAC_VERBOSE 3
 !endif
 !verbose ${UAC_VERBOSE}
@@ -195,7 +195,7 @@ _UAC_L_E_${__UAC_L}:
 !endif
 !insertmacro UAC_AsUser_Call Label _UAC_L_F_${__UAC_L} ${UAC_SYNCREGISTERS}|${UAC_SYNCOUTDIR}|${UAC_SYNCINSTDIR} #|${UAC_CLEARERRFLAG}
 !if "${workdir}" != ""
-	pop $outdir 
+	pop $outdir
 	SetOutPath $outdir
 !endif
 !macroend
@@ -263,7 +263,7 @@ pop $_LOGICLIB_TEMP
 !macroend
 !macro _UAC_AsUser_GenOp outvar op opparam1 opparam2
 !define _UAC_AUGOGR_ID _UAC_AUGOGR_OP${outvar}${op}${opparam1}${opparam2}
-!ifndef ${_UAC_AUGOGR_ID} ;Has this exact action been done before? 
+!ifndef ${_UAC_AUGOGR_ID} ;Has this exact action been done before?
 	!if ${outvar} == $0
 		!define ${_UAC_AUGOGR_ID} $1
 	!else
@@ -275,7 +275,7 @@ pop $_LOGICLIB_TEMP
 	!else
 		!define _UAC_AUGOGR_OPP1 ${opparam1}
 		!define _UAC_AUGOGR_OPP2 ${${_UAC_AUGOGR_ID}}
-	!endif	
+	!endif
 	goto ${_UAC_AUGOGR_ID}_C
 	${_UAC_AUGOGR_ID}_F:
 		${op} ${_UAC_AUGOGR_OPP1} ${_UAC_AUGOGR_OPP2}
@@ -294,4 +294,4 @@ pop ${${_UAC_AUGOGR_ID}}
 
 
 !verbose pop
-!endif /* UAC_HDR__INC */
+!endif /* UAC_HDR__INC */