zhangyiss/spack
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

hooks: remove 7 unused hooks (#42575)

Browse Source 
These 7 hooks were not used.

- Six of them related to install phases were unused after `spack`
  `monitor` was removed, and the code seems to have bit rotten as there
  were reports they were not (always?) triggered when they should.
- The post environment one was made redundant after spack install for
  environment started following the common code path for generating
  module files in #42147.

It should not be a breaking change to remove, since users cannot define
hooks in extensions, they would have to fork Spack.

If we ever _were_ to make those hooks extendable outside of core Spack,
it would also be better to start with fewer rather than more, cause
everything you expose gets relied upon...

Removing those also allows us to rethink what hooks we really need, and
in particular it seems like we need a hook that runs post install also when
the spec is inserted into the database.
This commit is contained in:
Harmen Stoppels 2024-02-09 20:52:09 +01:00 committed by GitHub
parent 7ff3b17f14
commit 4f0a8fce52
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
4 changed files with 12 additions and 120 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

92
lib/spack/docs/developer_guide.rst
View File

@ -357,91 +357,23 @@ If there is a hook that you would like and is missing, you can propose to add a
``pre_install(spec)``
"""""""""""""""""""""
A ``pre_install`` hook is run within an install subprocess, directly before
the install starts. It expects a single argument of a spec, and is run in
a multiprocessing subprocess. Note that if you see ``pre_install`` functions associated with packages these are not hooks
as we have defined them here, but rather callback functions associated with
a package install.
A ``pre_install`` hook is run within the install subprocess, directly before the install starts.
It expects a single argument of a spec.
""""""""""""""""""""""
``post_install(spec)``
""""""""""""""""""""""
"""""""""""""""""""""""""""""""""""""
``post_install(spec, explicit=None)``
"""""""""""""""""""""""""""""""""""""
A ``post_install`` hook is run within an install subprocess, directly after
the install finishes, but before the build stage is removed. If you
write one of these hooks, you should expect it to accept a spec as the only
argument. This is run in a multiprocessing subprocess. This ``post_install`` is
also seen in packages, but in this context not related to the hooks described
here.
A ``post_install`` hook is run within the install subprocess, directly after the install finishes,
but before the build stage is removed and the spec is registered in the database. It expects two
arguments: spec and an optional boolean indicating whether this spec is being installed explicitly.
""""""""""""""""""""""""""""""""""""""""""""""""""""
``pre_uninstall(spec)`` and ``post_uninstall(spec)``
""""""""""""""""""""""""""""""""""""""""""""""""""""
""""""""""""""""""""""""""
``on_install_start(spec)``
""""""""""""""""""""""""""
This hook is run at the beginning of ``lib/spack/spack/installer.py``,
in the install function of a ``PackageInstaller``,
and importantly is not part of a build process, but before it. This is when
we have just newly grabbed the task, and are preparing to install. If you
write a hook of this type, you should provide the spec to it.
.. code-block:: python
def on_install_start(spec):
"""On start of an install, we want to...
"""
print('on_install_start')
""""""""""""""""""""""""""""
``on_install_success(spec)``
""""""""""""""""""""""""""""
This hook is run on a successful install, and is also run inside the build
process, akin to ``post_install``. The main difference is that this hook
is run outside of the context of the stage directory, meaning after the
build stage has been removed and the user is alerted that the install was
successful. If you need to write a hook that is run on success of a particular
phase, you should use ``on_phase_success``.
""""""""""""""""""""""""""""
``on_install_failure(spec)``
""""""""""""""""""""""""""""
This hook is run given an install failure that happens outside of the build
subprocess, but somewhere in ``installer.py`` when something else goes wrong.
If you need to write a hook that is relevant to a failure within a build
process, you would want to instead use ``on_phase_failure``.
"""""""""""""""""""""""""""
``on_install_cancel(spec)``
"""""""""""""""""""""""""""
The same, but triggered if a spec install is cancelled for any reason.
"""""""""""""""""""""""""""""""""""""""""""""""
``on_phase_success(pkg, phase_name, log_file)``
"""""""""""""""""""""""""""""""""""""""""""""""
This hook is run within the install subprocess, and specifically when a phase
successfully finishes. Since we are interested in the package, the name of
the phase, and any output from it, we require:
- **pkg**: the package variable, which also has the attached spec at ``pkg.spec``
- **phase_name**: the name of the phase that was successful (e.g., configure)
- **log_file**: the path to the file with output, in case you need to inspect or otherwise interact with it.
"""""""""""""""""""""""""""""""""""""""""""""
``on_phase_error(pkg, phase_name, log_file)``
"""""""""""""""""""""""""""""""""""""""""""""
In the case of an error during a phase, we might want to trigger some event
with a hook, and this is the purpose of this particular hook. Akin to
``on_phase_success`` we require the same variables - the package that failed,
the name of the phase, and the log file where we might find errors.
These hooks are currently used for cleaning up module files after uninstall.
^^^^^^^^^^^^^^^^^^^^^^

1
lib/spack/spack/environment/environment.py
View File

@ -2090,7 +2090,6 @@ def write(self, regenerate: bool = True) -> None:
if regenerate:
self.regenerate_views()
spack.hooks.post_env_write(self)
self.new_specs.clear()

19
lib/spack/spack/hooks/__init__.py
View File

@ -15,13 +15,6 @@
* post_install(spec, explicit)
* pre_uninstall(spec)
* post_uninstall(spec)
* on_install_start(spec)
* on_install_success(spec)
* on_install_failure(spec)
* on_phase_success(pkg, phase_name, log_file)
* on_phase_error(pkg, phase_name, log_file)
* on_phase_error(pkg, phase_name, log_file)
* post_env_write(env)
This can be used to implement support for things like module
systems (e.g. modules, lmod, etc.) or to add other custom
@ -78,17 +71,5 @@ def __call__(self, *args, **kwargs):
pre_install = _HookRunner("pre_install")
post_install = _HookRunner("post_install")
# These hooks are run within an install subprocess
pre_uninstall = _HookRunner("pre_uninstall")
post_uninstall = _HookRunner("post_uninstall")
on_phase_success = _HookRunner("on_phase_success")
on_phase_error = _HookRunner("on_phase_error")
# These are hooks in installer.py, before starting install subprocess
on_install_start = _HookRunner("on_install_start")
on_install_success = _HookRunner("on_install_success")
on_install_failure = _HookRunner("on_install_failure")
on_install_cancel = _HookRunner("on_install_cancel")
# Environment hooks
post_env_write = _HookRunner("post_env_write")

20
lib/spack/spack/installer.py
View File

@ -1705,7 +1705,6 @@ def _install_task(self, task: BuildTask, install_status: InstallStatus) -> None:
except spack.build_environment.StopPhase as e:
# A StopPhase exception means that do_install was asked to
# stop early from clients, and is not an error at this point
spack.hooks.on_install_failure(task.request.pkg.spec)
pid = f"{self.pid}: " if tty.show_pid() else ""
tty.debug(f"{pid}{str(e)}")
tty.debug(f"Package stage directory: {pkg.stage.source_path}")
@ -2011,7 +2010,6 @@ def install(self) -> None:
if task is None:
continue
spack.hooks.on_install_start(task.request.pkg.spec)
install_args = task.request.install_args
keep_prefix = install_args.get("keep_prefix")
@ -2037,9 +2035,6 @@ def install(self) -> None:
tty.warn(f"{pkg_id} does NOT actually have any uninstalled deps left")
dep_str = "dependencies" if task.priority > 1 else "dependency"
# Hook to indicate task failure, but without an exception
spack.hooks.on_install_failure(task.request.pkg.spec)
raise InstallError(
f"Cannot proceed with {pkg_id}: {task.priority} uninstalled "
f"{dep_str}: {','.join(task.uninstalled_deps)}",
@ -2062,11 +2057,6 @@ def install(self) -> None:
tty.warn(f"{pkg_id} failed to install")
self._update_failed(task)
# Mark that the package failed
# TODO: this should also be for the task.pkg, but we don't
# model transitive yet.
spack.hooks.on_install_failure(task.request.pkg.spec)
if self.fail_fast:
raise InstallError(fail_fast_err, pkg=pkg)
@ -2169,7 +2159,6 @@ def install(self) -> None:
tty.error(
f"Failed to install {pkg.name} due to " f"{exc.__class__.__name__}: {str(exc)}"
)
spack.hooks.on_install_cancel(task.request.pkg.spec)
raise
except binary_distribution.NoChecksumException as exc:
@ -2188,7 +2177,6 @@ def install(self) -> None:
except (Exception, SystemExit) as exc:
self._update_failed(task, True, exc)
spack.hooks.on_install_failure(task.request.pkg.spec)
# Best effort installs suppress the exception and mark the
# package as a failure.
@ -2372,9 +2360,6 @@ def run(self) -> bool:
_print_timer(pre=self.pre, pkg_id=self.pkg_id, timer=self.timer)
_print_installed_pkg(self.pkg.prefix)
# Send final status that install is successful
spack.hooks.on_install_success(self.pkg.spec)
# preserve verbosity across runs
return self.echo
@ -2453,15 +2438,10 @@ def _real_install(self) -> None:
# Catch any errors to report to logging
self.timer.start(phase_fn.name)
phase_fn.execute()
spack.hooks.on_phase_success(pkg, phase_fn.name, log_file)
self.timer.stop(phase_fn.name)
except BaseException:
combine_phase_logs(pkg.phase_log_files, pkg.log_path)
spack.hooks.on_phase_error(pkg, phase_fn.name, log_file)
# phase error indicates install error
spack.hooks.on_install_failure(pkg.spec)
raise
# We assume loggers share echo True/False