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.

Sign up for GitHub

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

Jump to bottom

Move "installed" and "installed_upstream" from PackageBase to Spec #30191

Merged
merged 6 commits into from
Apr 22, 2022
Merged

Move "installed" and "installed_upstream" from PackageBase to Spec #30191

merged 6 commits into from
Apr 22, 2022

Conversation

alalazo
Copy link
Member

@alalazo alalazo commented Apr 20, 2022 •
edited
Loading

fixes #30187

This PR moves the installed and installed_upstream properties from PackageBase to Spec and is a step towards being able to reuse specs for which we don't have a package.py available. It should be sufficient to complete the concretization step and see the spec in the concretized DAG.

To fully reuse a spec without a package.py though we need a way to serialize enough data to reconstruct the results of calls to:

  • Spec.libs, Spec.headers and Spec.ommand
  • Package.setup_dependent_*_environment and Package.setup_run_environment
  • Other methods ?

This came out of a discussion with @becker33 and @scheibelp during one of the weekly telco

@alalazo alalazo marked this pull request as ready for review April 21, 2022 06:14
tgamblin
tgamblin requested changes Apr 22, 2022
View reviewed changes
Copy link
Member

@tgamblin tgamblin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor requests but looks great to me.

The refactored test where we don’t have to make a mock package anymore really shows why this is the right refactor — we shouldn’t have to go through Package when we can just ask the DB.

lib/spack/spack/cmd/fetch.py Show resolved Hide resolved
lib/spack/spack/package.py Show resolved Hide resolved
lib/spack/spack/spec.py Show resolved Hide resolved
@alalazo
Address review comments
f862dce 
- Add stub methods to packages with warnings
- Add a missing "root=False" in cmd/fetch.py
- Assert that a spec is concrete before checking
  installation status
@alalazo alalazo requested a review from tgamblin April 22, 2022 16:53
@tgamblin tgamblin added this to In progress in Spack v0.18.0 release via automation Apr 22, 2022
@tgamblin tgamblin enabled auto-merge (squash) April 22, 2022 18:49
@tgamblin tgamblin disabled auto-merge April 22, 2022 21:46
@tgamblin tgamblin merged commit a9fbc01 into spack:develop Apr 22, 2022
Spack v0.18.0 release automation moved this from In progress to Done Apr 22, 2022
tgamblin added a commit that referenced this pull request Apr 25, 2022
@tgamblin
bugfix: Spec.installed and Spec.installed_upstream should not `as…
ac74a1b 
…sert`

Fix bug introduced in #30191. `Spec.installed` and `Spec.installed_upstream` should
just return `False` for abstract specs, as they can be called in that context.

- [x] `Spec.installed` returns `False` now instead of asserting that the `Spec`
      is concrete.
- [x] `Spec.installed_upstream` returns `False` now instead of asserting that
      the `Spec` is concrete.
- [x] `Spec.installed_upstream` no longer caches its result, as install status
      seems like a bad thing to cache -- it can easily be invalidated. Calling code
      should use transactions if there are peformance issues, as with other places
      in Spack.
@tgamblin tgamblin mentioned this pull request Apr 25, 2022
Merged
4 tasks
tgamblin added a commit that referenced this pull request Apr 25, 2022
@tgamblin
bugfix: installed and installed_upstream should not assert
012f43f 
Fix bug introduced in #30191. `Spec.installed` and `Spec.installed_upstream` should just return
`False` for abstract specs, as they can be called in that context.

- [x] `Spec.installed` returns `False` now instead of asserting that the `Spec`
      is concrete.
- [x] `Spec.installed_upstream` returns `False` now instead of asserting that the `Spec`
      is concrete.
- [x] `Spec.installed_upstream` no longer caches its result, as install status seems
      like a bad thing to cache -- it can easily be invalidated. Calling code should
      use transactions if there are peformance issues, as with other places in Spack.
- [x] add tests for `Spec.installed` and `Spec.installed_upstream`
tgamblin added a commit that referenced this pull request Apr 25, 2022
@tgamblin
bugfix: installed and installed_upstream should not assert
ab6767c 
Fix bug introduced in #30191. `Spec.installed` and `Spec.installed_upstream` should just return
`False` for abstract specs, as they can be called in that context.

- [x] `Spec.installed` returns `False` now instead of asserting that the `Spec`
      is concrete.
- [x] `Spec.installed_upstream` returns `False` now instead of asserting that the `Spec`
      is concrete.
- [x] `Spec.installed_upstream` no longer caches its result, as install status seems
      like a bad thing to cache -- it can easily be invalidated. Calling code should
      use transactions if there are peformance issues, as with other places in Spack.
- [x] add tests for `Spec.installed` and `Spec.installed_upstream`
@alalazo alalazo deleted the refactor/move_installed_properties_to_spec branch April 25, 2022 07:04
haampie pushed a commit that referenced this pull request Apr 25, 2022
@tgamblin
bugfix: installed and installed_upstream should not assert (#30271)
d729b4e 
Fix bug introduced in #30191. `Spec.installed` and `Spec.installed_upstream` should just return
`False` for abstract specs, as they can be called in that context.

- [x] `Spec.installed` returns `False` now instead of asserting that the `Spec`
      is concrete.
- [x] `Spec.installed_upstream` returns `False` now instead of asserting that the `Spec`
      is concrete.
- [x] `Spec.installed_upstream` no longer caches its result, as install status seems
      like a bad thing to cache -- it can easily be invalidated. Calling code should
      use transactions if there are peformance issues, as with other places in Spack.
- [x] add tests for `Spec.installed` and `Spec.installed_upstream`
tgamblin added a commit that referenced this pull request Apr 25, 2022
@tgamblin
bugfix: installed and installed_upstream should not assert (#30271)
8c2eb56 
Fix bug introduced in #30191. `Spec.installed` and `Spec.installed_upstream` should just return
`False` for abstract specs, as they can be called in that context.

- [x] `Spec.installed` returns `False` now instead of asserting that the `Spec`
      is concrete.
- [x] `Spec.installed_upstream` returns `False` now instead of asserting that the `Spec`
      is concrete.
- [x] `Spec.installed_upstream` no longer caches its result, as install status seems
      like a bad thing to cache -- it can easily be invalidated. Calling code should
      use transactions if there are peformance issues, as with other places in Spack.
- [x] add tests for `Spec.installed` and `Spec.installed_upstream`
trws pushed a commit to trws/spack that referenced this pull request Apr 29, 2022
@alalazo @trws
Move "installed" and "installed_upstream" from PackageBase to Spec (s…
dd118a3 
…pack#30191)

This PR moves the `installed` and `installed_upstream` properties from `PackageBase` to `Spec` and is a step towards being able to reuse specs for which we don't have a `package.py` available. It _should_ be sufficient to complete the concretization step and see the spec in the concretized DAG.

To fully reuse a spec without a package.py though we need a way to serialize enough data to reconstruct the results of calls to:
- `Spec.libs`, `Spec.headers` and `Spec.ommand`
- `Package.setup_dependent_*_environment` and `Package.setup_run_environment`

- [x] Add stub methods to packages with warnings
- [x] Add a missing "root=False" in cmd/fetch.py
- [x] Assert that a spec is concrete before checking installation status
trws pushed a commit to trws/spack that referenced this pull request Apr 29, 2022
@tgamblin @trws
bugfix: installed and installed_upstream should not assert (spack…
2120e9f 
…#30271)

Fix bug introduced in spack#30191. `Spec.installed` and `Spec.installed_upstream` should just return
`False` for abstract specs, as they can be called in that context.

- [x] `Spec.installed` returns `False` now instead of asserting that the `Spec`
      is concrete.
- [x] `Spec.installed_upstream` returns `False` now instead of asserting that the `Spec`
      is concrete.
- [x] `Spec.installed_upstream` no longer caches its result, as install status seems
      like a bad thing to cache -- it can easily be invalidated. Calling code should
      use transactions if there are peformance issues, as with other places in Spack.
- [x] add tests for `Spec.installed` and `Spec.installed_upstream`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tgamblin tgamblin tgamblin approved these changes

@scheibelp scheibelp Awaiting requested review from scheibelp

@becker33 becker33 Awaiting requested review from becker33

Assignees
No one assigned
Labels
binary-packages commands environments modules tests General test capability(ies) utilities
Projects
No open projects
Spack v0.18.0 release
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cannot query if a concrete spec from an unknown namespace is installed
2 participants
@alalazo @tgamblin