Skip to content

Extract installation of dependencies from PackageBase.do_install#6833

Closed
alalazo wants to merge 2 commits intospack:developfrom
epfl-scitas:refactoring/package_do_install
Closed

Extract installation of dependencies from PackageBase.do_install#6833
alalazo wants to merge 2 commits intospack:developfrom
epfl-scitas:refactoring/package_do_install

Conversation

@alalazo
Copy link
Copy Markdown
Member

@alalazo alalazo commented Jan 5, 2018

Up to this commit PackageBase.do_install was installing the root package of a DAG unconditionally and its dependencies conditionally (depending on the value of the boolean argument install_deps).

However, a sensible operation in many contexts is to install only the dependencies. This was not possible via API, and the code to do that was replicated in the implementation of PackageBase.do_install and of the spack install command. Further, the code in those two places was not doing exactly the same thing.

This commit extracts a new function that is used in both places, and is external to PackageBase. This choice has been preferred over extending PackageBase.do_install API because:

  • the method was already complex enough (10-15 arguments) and needed to be simplified
  • having a method on an instance that installs anything but that instance spec didn't seem a sensible choice

@adamjstewart
Copy link
Copy Markdown
Member

adamjstewart commented Jan 5, 2018 

$ spack configure xz
...
Process Process-1:
Traceback (most recent call last):
  File "/Users/Adam/spack/lib/spack/spack/build_environment.py", line 665, in child_process
    return_value = function()
  File "/Users/Adam/spack/lib/spack/spack/package.py", line 1435, in build_process
    phase(self.spec, self.prefix)
  File "/Users/Adam/spack/lib/spack/spack/package.py", line 121, in phase_wrapper
    self._on_phase_exit(instance)
  File "/Users/Adam/spack/lib/spack/spack/package.py", line 131, in _on_phase_exit
    raise StopIteration('Stopping at \'{0}\' phase'.format(self.name))
StopIteration: Stopping at 'configure' phase

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/Users/Adam/anaconda3/lib/python3.6/multiprocessing/process.py", line 258, in _bootstrap
    self.run()
  File "/Users/Adam/anaconda3/lib/python3.6/multiprocessing/process.py", line 93, in run
    self._target(*self._args, **self._kwargs)
  File "/Users/Adam/spack/lib/spack/spack/build_environment.py", line 670, in child_process
    tty.msg(e.message)
AttributeError: 'StopIteration' object has no attribute 'message'

@alalazo
Copy link
Copy Markdown
Member Author

alalazo commented Jan 5, 2018 •
edited
Loading

@adamjstewart I can't reproduce this locally

$ spack configure xz
==> Checking dependencies for ['xz']
==> Installing xz
==> Using cached archive: /home/mculpo/PycharmProjects/spack/var/spack/cache/xz/xz-5.2.3.tar.bz2
==> Staging archive: /home/mculpo/PycharmProjects/spack/var/spack/stage/xz-5.2.3-dfdm7dysz7f3odzyrtpe6nc65uerxgre/xz-5.2.3.tar.bz2
==> Created stage in /home/mculpo/PycharmProjects/spack/var/spack/stage/xz-5.2.3-dfdm7dysz7f3odzyrtpe6nc65uerxgre
==> No patches needed for xz
==> Building xz [AutotoolsPackage]
==> Executing phase: 'autoreconf'
==> Executing phase: 'configure'
==> Stopping at 'configure' phase

EDIT: I am exactly at 48ef453

@adamjstewart
Copy link
Copy Markdown
Member

adamjstewart commented Jan 5, 2018

@alalazo Looks like it works for Python 2 but crashes for Python 3.

@alalazo
Copy link
Copy Markdown
Member Author

alalazo commented Jan 5, 2018

@adamjstewart Confirmed. The bug though is not specific to this PR, but is also on develop.

@alalazo
Copy link
Copy Markdown
Member Author

alalazo commented Jan 5, 2018

I am looking for a reliable source to check this, but it seems python3 exceptions don't have the message attribute. The solution (I'll prepare a PR for that) is probably to find all the places where we use exc.message and substitute it with str(exc). I'd like to find a PEP or something similar to confirm what I got by induction - philosophers and turkeys say it's safer!

@alalazo
Copy link
Copy Markdown
Member Author

alalazo commented Jan 5, 2018

Ok, seems something quite old - PEP 352

alalazo added a commit to epfl-scitas/spack that referenced this pull request Jan 5, 2018
@alalazo
Fixes the bug in spack configure spotted in spack#6833
0c2b6a6
adamjstewart pushed a commit that referenced this pull request Jan 5, 2018
@alalazo @adamjstewart
Fixes the bug in spack configure spotted in #6833 (#6837)
d978cfb
@alalazo alalazo force-pushed the refactoring/package_do_install branch from 48ef453 to fa6eb40 Compare January 8, 2018 13:54
@alalazo alalazo mentioned this pull request Jan 28, 2018
Merged
alalazo and others added 2 commits February 13, 2018 13:04
@alalazo
Extract installation of dependencies from PackageBase.do_install
45a399d 
Up to this commit PackageBase.do_install was installing the root package
of a DAG unconditionally and its dependencies conditionally (depending
on the value of the boolean argument 'install_deps').

However, a sensible operation in many contexts is to install *only* the
dependencies. This was not possible via API, and the code to do that
was replicated in the implementation of PackageBase.do_install and of
the 'spack install' command. Further, the code in those two places was
not doing *exactly* the same thing.

This commit extracts a new function that is used in both places, and is
external to PackageBase. This choice has been preferred over extending
PackageBase.do_install API because the method was already complex enough
(10-15 arguments) and needed to be simplified + having a method on an
instance that installs anything but that instance spec didn't seem a
sensible choice.
@alalazo
Fixed failing tests from rebase conflict
8df7be1
@alalazo alalazo force-pushed the refactoring/package_do_install branch from fa6eb40 to 8df7be1 Compare February 13, 2018 13:06
ch741 pushed a commit to ch741/spack that referenced this pull request Apr 20, 2018
@alalazo @ChrisHad
Fixes the bug in spack configure spotted in spack#6833 (spack#6837)
4b8f858
@alalazo alalazo closed this Feb 12, 2019
@alalazo alalazo deleted the refactoring/package_do_install branch February 12, 2019 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tgamblin tgamblin Awaiting requested review from tgamblin

@scheibelp scheibelp Awaiting requested review from scheibelp

@adamjstewart adamjstewart Awaiting requested review from adamjstewart

Assignees

No one assigned

Labels

refactoring

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@alalazo @adamjstewart