Add reserved commit variant to preserve data by psakievich · Pull Request #48702 · spack/spack

psakievich · 2025-01-23T23:18:40Z

As noted in #48121 when we move to always having commit sha's paired to GitVersions the optional user supplied git ref can get lost. This PR is intended to make space for storing all three components of a GitVersion

User requested git ref
Commit sha
Spack standard version that will be used for version comparisons

We've opted for the following syntax. When a spec has a commit assigned it will go into a reserved variant

foo@master commit=<sha>

GitVersion syntax is then unchanged, but a commit will get added based on the user ref.

foo@git.<user_ref>=<modeled_version> commit=<sha>

This PR also ensures that the fetcher will respect a commit variant. After doing this work I'm not totally sure we need to convert StandardVersions using branch or tag attributes to GitVersions. It's not clear to me what the additional utility that provides over just accounting for the commit sha as a variant.

Using the commit as a proper variant mostly resolves the concerns regarding projections and use inside a package class outlined in #48121. We essentially side step the issue by not changing versions and making commits node attributes rather than version attributes. I believe this eliminates, or at least alleviates, most of the need for using GitVersions in production systems

I will add docs once the basic design is approved.

lib/spack/spack/spec.py

psakievich · 2025-02-24T21:48:50Z

@spackbot fix style

spackbot-app · 2025-02-24T21:48:54Z

Let me see if I can fix that for you!

spackbot-app · 2025-02-24T21:50:00Z

I was able to run spack style --fix for you!

spack style --fix

==> Running style checks on spack
  selected: import, isort, black, flake8, mypy
==> Modified files
  lib/spack/spack/fetch_strategy.py
  lib/spack/spack/solver/asp.py
  lib/spack/spack/spec.py
  lib/spack/spack/test/concretization/core.py
  lib/spack/spack/test/conftest.py
  lib/spack/spack/test/git_fetch.py
  lib/spack/spack/test/versions.py
  lib/spack/spack/variant.py
  lib/spack/spack/version/version_types.py
  var/spack/repos/builtin.mock/packages/git-ref-package/package.py
==> Running import checks
  import checks were clean
==> Running isort checks
  isort checks were clean
==> Running black checks
reformatted lib/spack/spack/solver/asp.py
All done! ✨ 🍰 ✨
1 file reformatted, 9 files left unchanged.
  black checks were clean
==> Running flake8 checks
  flake8 checks were clean
==> Running mypy checks
Success: no issues found in 637 source files
  mypy checks were clean
==> spack style checks were clean

Keep in mind that I cannot fix your flake8 or mypy errors, so if you have any you'll need to fix them and update the pull request. If I was able to push to your branch, if you make further changes you will need to pull from your updated branch before pushing again.

I've updated the branch with style fixes.

psakievich · 2025-03-02T18:13:29Z

@spackbot fix style

spackbot-app · 2025-03-02T18:13:32Z

Let me see if I can fix that for you!

spackbot-app · 2025-03-02T18:15:18Z

I was able to run spack style --fix for you!

spack style --fix

==> Running style checks on spack
  selected: import, isort, black, flake8, mypy
==> Modified files
  lib/spack/llnl/util/lock.py
  lib/spack/spack/detection/common.py
  lib/spack/spack/directives.py
  lib/spack/spack/fetch_strategy.py
  lib/spack/spack/package_base.py
  lib/spack/spack/solver/asp.py
  lib/spack/spack/spec.py
  lib/spack/spack/test/concretization/core.py
  lib/spack/spack/test/conftest.py
  lib/spack/spack/test/git_fetch.py
  lib/spack/spack/test/versions.py
  lib/spack/spack/variant.py
  lib/spack/spack/version/version_types.py
  var/spack/repos/builtin.mock/packages/git-ref-package/package.py
==> Running import checks
  import checks were clean
==> Running isort checks
  isort checks were clean
==> Running black checks
reformatted lib/spack/spack/detection/common.py
reformatted lib/spack/llnl/util/lock.py
reformatted lib/spack/spack/directives.py
reformatted lib/spack/spack/spec.py
All done! ✨ 🍰 ✨
4 files reformatted, 10 files left unchanged.
  black checks were clean
==> Running flake8 checks
  flake8 checks were clean
==> Running mypy checks
Success: no issues found in 637 source files
  mypy checks were clean
==> spack style checks were clean

Keep in mind that I cannot fix your flake8 or mypy errors, so if you have any you'll need to fix them and update the pull request. If I was able to push to your branch, if you make further changes you will need to pull from your updated branch before pushing again.

I've updated the branch with style fixes.

psakievich · 2025-03-17T18:53:26Z

@spackbot fix style

spackbot-app · 2025-03-17T18:53:30Z

Let me see if I can fix that for you!

spackbot-app · 2025-03-17T18:55:02Z

I was able to run spack style --fix for you!

spack style --fix

==> Running style checks on spack
  selected: import, isort, black, flake8, mypy
==> Modified files
  lib/spack/spack/fetch_strategy.py
  lib/spack/spack/package_base.py
  lib/spack/spack/solver/asp.py
  lib/spack/spack/spec.py
  lib/spack/spack/test/concretization/core.py
  lib/spack/spack/test/conftest.py
  lib/spack/spack/test/git_fetch.py
  lib/spack/spack/test/versions.py
  lib/spack/spack/variant.py
  lib/spack/spack/version/__init__.py
  lib/spack/spack/version/common.py
  lib/spack/spack/version/version_types.py
  var/spack/repos/builtin.mock/packages/git-ref-package/package.py
==> Running import checks
  import checks were clean
==> Running isort checks
  isort checks were clean
==> Running black checks
reformatted lib/spack/spack/version/common.py
All done! ✨ 🍰 ✨
1 file reformatted, 12 files left unchanged.
  black checks were clean
==> Running flake8 checks
  flake8 checks were clean
==> Running mypy checks
Success: no issues found in 637 source files
  mypy checks were clean
==> spack style checks were clean

Keep in mind that I cannot fix your flake8 or mypy errors, so if you have any you'll need to fix them and update the pull request. If I was able to push to your branch, if you make further changes you will need to pull from your updated branch before pushing again.

I've updated the branch with style fixes.

becker33 · 2025-03-18T17:18:38Z

Following on to our conversation today, some things we need to check:

spack install foo commit=<...> needs to instantiate the version to something reasonable

If there is no reuse, that should concretize to foo@<...> commit=<...>, but this should also be able to reuse a hypothetical foo@develop commit=<...> that was already installed.

spack install [email protected] commit=<...> needs to fail to concretize if <...> is not a valid commit for a ref 4.1
spack find commit=<...> needs to show appropriate packages. I think this probably falls out naturally from your implementation, but we should double-check.
Reserve the names ref, branch, and tag as well as part of this PR.

Future PR will parse them, and will remove the GitVersion class. [email protected]=3.3 will parse as [email protected] ref=develop. Internally, we will need to model ref as branch.value or tag.value or ref.value, so that we can parse the old syntax which is ambiguous to which type of ref it is. [email protected] will then parse as foo ref=develop. This should naturally handle [email protected] parsing as foo ref=abcdef for any partial commit sha that is unambiguous, the same as currently in develop, but I feel less confident of that

Look into whether there is a better way to print these than just leaving them as-is in the variant list

@psakievich am I missing anything from our conversation?

…ctor

psakievich · 2025-05-23T23:50:02Z

@spackbot fix style

spackbot-app · 2025-05-23T23:50:05Z

Let me see if I can fix that for you!

spackbot-app · 2025-05-23T23:51:12Z

I was able to run spack style --fix for you!

spack style --fix

==> Running style checks on spack
  selected: import, isort, black, flake8, mypy
==> Modified files
  lib/spack/spack/fetch_strategy.py
  lib/spack/spack/package_base.py
  lib/spack/spack/solver/asp.py
  lib/spack/spack/spec.py
  lib/spack/spack/test/cmd/find.py
  lib/spack/spack/test/concretization/core.py
  lib/spack/spack/test/conftest.py
  lib/spack/spack/test/git_fetch.py
  lib/spack/spack/test/packages.py
  lib/spack/spack/test/versions.py
  lib/spack/spack/variant.py
  lib/spack/spack/version/__init__.py
  lib/spack/spack/version/common.py
  lib/spack/spack/version/version_types.py
  var/spack/test_repos/spack_repo/builtin_mock/packages/git_ref_commit_dep/package.py
  var/spack/test_repos/spack_repo/builtin_mock/packages/git_ref_package/package.py
  var/spack/test_repos/spack_repo/builtin_mock/packages/git_test_commit/package.py
==> Running import checks
  import checks were clean
==> Running isort checks
Fixing /tmp/tmp2pw76zj9/spack/var/spack/test_repos/spack_repo/builtin_mock/packages/git_ref_commit_dep/package.py
  isort checks were clean
==> Running black checks
All done! ✨ 🍰 ✨
17 files left unchanged.
  black checks were clean
==> Running flake8 checks
  flake8 checks were clean
==> Running mypy checks
Success: no issues found in 611 source files
  mypy checks were clean
==> spack style checks were clean

Keep in mind that I cannot fix your flake8 or mypy errors, so if you have any you'll need to fix them and update the pull request. If I was able to push to your branch, if you make further changes you will need to pull from your updated branch before pushing again.

I've updated the branch with style fixes.

becker33

Ok I tried to be as thorough as possible here so we don't have to go back and forth more than necessary, hopefully this all makes sense and is easy to address.

lib/spack/spack/package_base.py

lib/spack/spack/solver/concretize.lp

lib/spack/spack/test/cmd/find.py

lib/spack/spack/test/concretization/core.py

lib/spack/spack/test/git_fetch.py

lib/spack/spack/fetch_strategy.py

lib/spack/spack/solver/asp.py

…ion-refactor

@spackbot

* Define components for GitVersion provenance Adding binary provenance requires we track the commit. Typically this has been an optional form for the encompassing git ref. Moving towards always defining a commit means we need to have space to store a user requested ref that will then be paired with a commit sha. * Convert internal members to new names * Style fixes * Update lib/spack/spack/spec.py * Update lib/spack/spack/spec.py * commit variant concretizes and check is commit * Add tests * Add version attribute restrictions for commit variant * dev_path and commit mutually exclusive * Have fetcher respect the commit variant Ensure that if a commit variant is on the spec fetch operations use the commit * Add package class hook for SNL work * Fix commit check * Add a more specific function for shas * Handler for overlapping attributes * Fix tests * Add additional reserved variants * Add unit-tests to verify requirements * Write tests and stub out core changes * Attempt to add solver constraint * Fix lp constraint * First new unit test passes * Review fixes * WIP: refactor test * Add lookup check tests * Add package tests * New test on version satisfaction * Rework test * Fix problem * Revert random style changes * Style * Rework concretizer rules still not passing * WIP: More rules * Ideas for facts that will close the loop * Style and fixes * Disable failing tests * Update core.py * Update packages.py * Fix tests and test reuse * Remove Philter * Add docs * Relocate test package * Okay dir * Move test package * Use underscores * Fix package * [@spackbot] updating style on behalf of psakievich * Update path * Respond to review comments * Fix test --------- Co-authored-by: psakievich <[email protected]>

spackbot-app bot added core PR affects Spack core functionality tests General test capability(ies) versions labels Jan 23, 2025

psakievich mentioned this pull request Jan 23, 2025

Resolving Binary Provenance in Spack for Git Ref Versions #48121

Open

9 tasks

psakievich commented Jan 29, 2025

View reviewed changes

lib/spack/spack/spec.py Outdated Show resolved Hide resolved

psakievich commented Jan 29, 2025

View reviewed changes

lib/spack/spack/spec.py Outdated Show resolved Hide resolved

psakievich added the snl-core-team Issue for SNL Spack developers label Feb 4, 2025

psakievich force-pushed the psakiev/f/git-version-refactor branch from 0aba4db to 257a9a1 Compare February 8, 2025 06:37

spackbot-app bot added the new-version label Feb 8, 2025

psakievich force-pushed the psakiev/f/git-version-refactor branch from 59ab524 to ab1b64d Compare February 15, 2025 14:13

spackbot-app bot added the fetching label Feb 19, 2025

psakievich marked this pull request as ready for review February 19, 2025 20:16

psakievich requested review from becker33 and tgamblin February 19, 2025 20:20

psakievich self-assigned this Feb 19, 2025

psakievich changed the title ~~Refactor GitVersions to preserve data~~ Add reserved commit variant to preserve data Feb 19, 2025

psakievich mentioned this pull request Feb 23, 2025

depends_on git commit hashes in package recipe? #49145

Open

spackbot-app bot added directives locking utilities labels Feb 28, 2025

psakievich mentioned this pull request May 16, 2025

Extract commit sha from mirrors, turn on auto-commit population #50516

Closed

psakievich added 5 commits May 16, 2025 15:36

Merge branch 'develop' into psakiev/f/git-version-refactor

9cf1f5a

Merge remote-tracking branch 'origin' into psakiev/f/git-version-refa…

9ee2697

…ctor

Move test package

66bb5de

Use underscores

b54a72b

Fix package

3654d4d

[@spackbot] updating style on behalf of psakievich

08edd47

psakievich added 2 commits May 26, 2025 10:22

Merge branch 'develop' into psakiev/f/git-version-refactor

845d0d3

Update path

47017e3

psakievich added this to the v1.0.0 stretch goals milestone May 27, 2025

becker33 requested changes Jun 2, 2025

View reviewed changes

psakievich added 3 commits June 2, 2025 10:36

Merge remote-tracking branch 'origin/develop' into psakiev/f/git-vers…

19609ad

…ion-refactor

Respond to review comments

25a19b3

Fix test

eb2502f

psakievich requested a review from becker33 June 2, 2025 23:35

tgamblin modified the milestones: v1.0.0 stretch goals, v1.0.0 Jun 3, 2025

This was referenced Jun 3, 2025

Fix installable_main to reset spack module for fresh import #49109

Closed

Features/pip install draft 2 #50238

Closed

Merge branch 'develop' into psakiev/f/git-version-refactor

f6ce766

becker33 approved these changes Jun 4, 2025

View reviewed changes

psakievich enabled auto-merge (squash) June 4, 2025 11:50

psakievich disabled auto-merge June 4, 2025 12:35

psakievich enabled auto-merge (squash) June 4, 2025 16:10

psakievich merged commit 4127514 into develop Jun 4, 2025
36 checks passed

psakievich deleted the psakiev/f/git-version-refactor branch June 4, 2025 16:12

Conversation

psakievich commented Jan 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

psakievich commented Feb 24, 2025

Uh oh!

spackbot-app bot commented Feb 24, 2025

Uh oh!

spackbot-app bot commented Feb 24, 2025

Uh oh!

psakievich commented Mar 2, 2025

Uh oh!

spackbot-app bot commented Mar 2, 2025

Uh oh!

spackbot-app bot commented Mar 2, 2025

Uh oh!

psakievich commented Mar 17, 2025

Uh oh!

spackbot-app bot commented Mar 17, 2025

Uh oh!

spackbot-app bot commented Mar 17, 2025

Uh oh!

becker33 commented Mar 18, 2025

Uh oh!

psakievich commented May 23, 2025

Uh oh!

spackbot-app bot commented May 23, 2025

Uh oh!

spackbot-app bot commented May 23, 2025

Uh oh!

becker33 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

psakievich commented Jan 23, 2025 •

edited

Loading