Skip to content

Conversation

@jrray
Copy link
Collaborator

@jrray jrray commented Dec 7, 2025

Given a package with an embedded package that defines a var with a
default value, such as in:

install:
  embedded:
    - pkg: python/3.10.8
      build:
        options:
          - var: abi/3.10.8

Until the fix in #1302, these vars were not getting pinned
when the package is built and the stub created:

pkg: python/3.10.8/embedded[some-parent-pkg:run/3.10.8/P7SZECZD]
api: v0/package
build:
  options:
  - var: python.abi/cp310

This created a problem for solving correctly because
Satisfy<VarRequest> would incorrectly claim that this package is compatible
with any value for python.abi.

In order to fix solves for the existing embedded stubs, these missing
pinned values are pinned as the package specs are read from storage, so
they end up with the intended content:

pkg: python/3.10.8/embedded[some-parent-pkg:run/3.10.8/P7SZECZD]
api: v0/package
build:
  options:
  - var: python.abi/cp310
    static: cp310

Now Satisfy<VarRequest> will correctly reject this package as not
being compatible with, e.g., python.abi/cp311.

The best we can do is assume python.abi was not overridden to some
other value at the time the parent package was built and that the
default on the option is the appropriate value for the pin.

@jrray jrray self-assigned this Dec 7, 2025
@jrray jrray added bug Something isn't working SPI AOI Area of interest for SPI labels Dec 7, 2025
@codecov
Copy link

codecov bot commented Dec 7, 2025

Codecov Report

❌ Patch coverage is 77.77778% with 6 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
crates/spk-storage/src/storage/spfs.rs 71.42% 6 Missing ⚠️

📢 Thoughts on this report? Let us know!

@jrray
Copy link
Collaborator Author

jrray commented Dec 7, 2025

Some extra commentary... In the spirit of #1298 and friends, IMO it will help with bugs like this if/when an embedded package has a type that mandates that build options are pinned.

You could take things further since "build options" are arguably meaningless for something that can never be built: an embedded package.

You can also argue that a package, embedded or not, should not have a "build" section at all anymore, and anything from the "build" section of a recipe that is needed at runtime should be populated into the "install" section of the package at build time.

Similarly, an embedded package could arguably not have an "install" section and any of its runtime requirements should live in the runtime requirements of the component in the parent package that embeds this package.

Changes like these would go a long way towards simplifying how package dependencies are calculated and reduce the opportunity for bugs to sneak in.

For example, Package::runtime_requirements() returns a reference to the install.requirements field, The name might suggest that's all you need to figure out the runtime requirements of a package, however you still need to look at [some of] the build.options, components, embedded packages...

Handling components could be made easier if we baked out package specs for each component and treated them like separate packages. The InstallSpec of a built package would not have a components field anymore. It could also not have an embedded section anymore, just pkg requests for the stub package. The end goal being that install.requirements does become all you need to know the runtime requirements.

.await
.map_err(|err| Error::FileReadError(filename, err))?;
Spec::from_yaml(&yaml)
.map(|spec| match spec {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can understand that this might be undesirable, but it is needed at least in the short term to fix bad solves. With the other fix in this PR in place, we can work on republishing packages to fix their stubs (or write some fixer script), and then I would be good with reverting this.

@jrray jrray requested a review from rydrman December 7, 2025 02:22
@jrray jrray changed the base branch from main to embed-pin-var-on-build December 8, 2025 17:34
@jrray jrray added the pr-chain This PR doesn't target the main branch, don't merge! label Dec 8, 2025
@jrray jrray force-pushed the embed-stub-pin-var branch from 22f72dc to aaa800c Compare December 8, 2025 19:04
@jrray jrray force-pushed the embed-pin-var-on-build branch 2 times, most recently from 39271cf to 6a5854c Compare December 8, 2025 19:13
@jrray jrray force-pushed the embed-stub-pin-var branch from aaa800c to 25a9bc9 Compare December 8, 2025 19:13
@jrray jrray force-pushed the embed-pin-var-on-build branch from 6a5854c to e7dc7f2 Compare December 10, 2025 23:37
@jrray jrray force-pushed the embed-stub-pin-var branch from 25a9bc9 to ce42d94 Compare December 10, 2025 23:37
Base automatically changed from embed-pin-var-on-build to main December 10, 2025 23:52
Given a package with an embedded package that defines a var with a
default value, such as in:

```yaml
install:
  embedded:
    - pkg: python/3.10.8
      build:
        options:
          - var: abi/3.10.8
```

Until the fix in the previous commit, these vars were not getting pinned
when the package is built and the stub created:

```yaml
pkg: python/3.10.8/embedded[some-parent-pkg:run/3.10.8/P7SZECZD]
api: v0/package
build:
  options:
  - var: python.abi/cp310
```

This created a problem for solving correctly because
`Satisfy<VarRequest>` would incorrectly claim that this package is compatible
with _any_ value for `python.abi`.

In order to fix solves for the existing embedded stubs, these missing
pinned values are pinned as the package specs are read from storage, so
they end up with the intended content:

```yaml
pkg: python/3.10.8/embedded[some-parent-pkg:run/3.10.8/P7SZECZD]
api: v0/package
build:
  options:
  - var: python.abi/cp310
    static: cp310
```

Now `Satisfy<VarRequest>` will correctly reject this package as not
being compatible with, e.g., `python.abi/cp311`.

The best we can do is assume `python.abi` was not overridden to some
other value at the time the parent package was built and that the
default on the option is the appropriate value for the pin.

Signed-off-by: J Robert Ray <jrray@imageworks.com>
@jrray jrray force-pushed the embed-stub-pin-var branch from ce42d94 to b59386d Compare December 11, 2025 00:01
@jrray jrray merged commit 7ce3fd7 into main Dec 11, 2025
9 checks passed
@jrray jrray deleted the embed-stub-pin-var branch December 11, 2025 00:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working pr-chain This PR doesn't target the main branch, don't merge! SPI AOI Area of interest for SPI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants