Rework how packages describe how they contain the components of an embedded package

[jrray]

Addresses the question from #742.

The original spec schema had multiple places where an embedded package could be defined:

pkg: demo/1.0.0
install:
  components:
    - name: comp1
      embedded:
        # inside a component
        - pkg: embedded/2.0.0
  embedded:
    # outside a component
    - pkg: embedded/2.0.0

It becomes confusing if the embedded package also has components. Where do those components get defined? How can the parent package say which of its components contains the embedded packages' components?

A motivating example

The package boost contains a lot of headers only needed during build so it benefits from having those files live only in the "build" component. A second package, boost-python, is also a full build of boost (plus extra python things), so it needs to embed boost. Since the boost-python package is a full build of boost, it also benefits from having the headers only live in the "build" component. It needs to be able to say that the boost-python:build component embeds the boost:build component.

pkg: boost-python/1.76.0
install:
  components:
    - name: build
      files:
        - include/
        - lib/cmake/
        - lib/*.a
        - lib/*.so
      uses:
        - run
      embedded_components:    # <-- new
        - boost:build
    - name: run
      files:
        - "*"
      file_match_mode: Remaining
      embedded_components:    # <-- new
        - boost:run
  embedded:
    - pkg: boost/1.76.0
      install:
        components:
          - name: build
            uses:
              - run
          - name: run

This shows how the boost-python spec can now be written. The embedded boost package is defined once under install.embedded and its components are defined here. The new install.components[].embeded_components field is used to specify for each component of boost-python, which components of boost are present.

Solver logic changes

There can be a situation where a certain combination of requests will cause the solver to fail even though there is nothing "wrong" about those requests.

First, there needs to be a request for a specific component of a real package: real-package:comp1. Assuming real-package:comp1 exists, this request will solve successfully.

Then, some other package needs to exist that embeds real-package, let's call it fake-package. Assume fake-package:comp1 embeds real-package:comp1 and other components exist inside fake-package.

Now, add a request for a specific component of fake-package that isn't comp1: fake-package:comp2. As in, spk env real-package:comp1 fake-package:comp2.

The problem arises when fake-package replaces real-package in the solve, but only fake-package:comp2 is part of the solution, because this does not include (embed) real-package:comp1.

The solver now needs to detect that the current set of requests do not bring in all the necessary components of the parent package in order to have the required components of the embedded package satisfied. It effectively needs to translate the real-package:comp1 request into a request for fake-package:comp1. When this situation is detected, the state will be rolled back to just before fake-package is added to the solution, and a request for fake-package:comp1 is added. This combines with the explicit request for fake-package:comp2, making the merged request into fake-package:{comp1,comp2}. Now when fake-package is added to the solution it will include comp1 and the request for real-package:comp1 will be satisfied.

[jrray]

The solve fails if both the components aren't named here, because when it tries to find a build of victim it sees this spec and claims real-pkg doesn't have a component named comp2 ("does not define requested components").

[jrray]

Test output here for reference:

-------------- TEST START --------------
State Requests: 
State Resolved: 
State  VarReqs: ""
State  Options: {}
 INITIAL REQUEST fake-pkg:comp1/* (PreReleasePolicy: ExcludeAll, InclusionPolicy: Always, RequiredCompat: Binary)
 INITIAL REQUEST victim:run/* (PreReleasePolicy: ExcludeAll, InclusionPolicy: Always, RequiredCompat: Binary)
State Requests: fake-pkg:comp1/* (RequiredCompat: Binary), victim:run/* (RequiredCompat: Binary)
State Resolved: 
State  VarReqs: ""
State  Options: {}
> RESOLVE fake-pkg/1.0.0/3I42H3S6  (requested by spk's test suite)
. REQUEST real-pkg/=1.0.0/embedded (RequiredCompat: Binary)
. RESOLVE real-pkg/1.0.0/embedded  (requested by embedded in fake-pkg/1.0.0/3I42H3S6)
. ASSIGN {fake-pkg=~1.0.0}
State Requests: fake-pkg:comp1/* (RequiredCompat: Binary), victim:run/* (RequiredCompat: Binary), real-pkg/=1.0.0/embedded (RequiredCompat: Binary)
State Resolved: fake-pkg/1.0.0/3I42H3S6, real-pkg/1.0.0/embedded
State  VarReqs: ""
State  Options: {fake-pkg=~1.0.0}
>> RESOLVE victim/1.0.0/3I42H3S6  (requested by spk's test suite)
.. REQUEST real-pkg:comp2/1.0.0
.. ASSIGN {victim=~1.0.0}
Installed Packages:
  mem/fake-pkg:comp1/=1.0.0/3I42H3S6 highest (required by spk's test suite) {} 
  real-pkg/=1.0.0/embedded           highest (required by embedded in fake-pkg/1.0.0/3I42H3S6, victim/1.0.0/3I42H3S6) {} 
  mem/victim:run/=1.0.0/3I42H3S6     highest (required by spk's test suite) {} 
 Number of Packages: 3
thread 'solver::solver_test::test_solver_components_interaction_with_embeds' panicked at 'assertion failed: `(left == right)`
  left: `{Named("comp1")}`,
 right: `{Named("comp1"), Named("comp2")}`', crates/spk-solve/src/./solver_test.rs:1891:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
test solver::solver_test::test_solver_components_interaction_with_embeds ... FAILED

[jrray]

So I think what is missing is in two parts:

1. There needs to be a way for the embedd-er to name which components of the embedd-ee are embedded within the components of the embedd-er.
2. From the added info provided by [1], the solver needs to detect that a request for a component hasn't been satisfied and inject the additional components of the embedd_er_ that will satisfy them. Or, this could just be detected as unsatisfiable and require the requests to be changed.

[jrray]

Now I'm thinking the problem runs deeper, and the spec schema has a problem that two incompatible specs can be presented for the same embedded package in different components. Example:

pkg: embedder/1.0.0
install:
  components:
    - name: comp1
      embedded:
        - pkg: embedee/1.0.0
          install:
            components:
              - name: comp1
    - name: comp2
      embedded:
        - pkg: embedee/2.0.0
          install:
            components:
              - name: comp2

The TODOs in the test mention how this causes solver problems, and this goes a step further to show how two different incompatible versions of embedee are described between the two components.

What piece of information that is needed in the install.components[].embedded position is what components of embedded packages are present in that (parent) component. This should refer to an embedded package that has been defined once in the non-component way. Example:

pkg: embedder/1.0.0
install:
  components:
    - name: comp1
      embedded_packages:
        - embedee:comp1
    - name: comp2
      embedded_packages:
        - embedee:{comp2,comp3}
  embedded:
    - pkg: embedee/1.0.0
      install:
        components:
          - name: comp1
          - name: comp2
          - name: comp3

The embedded package embedee is defined once, in full, in the position install.embedded[]. Then, in install.components[].embedded_components there are optionally references to that package to specify which components are present.

Now it is clear what the spec for the embedded stub should be (it only appears once in the parent spec) and it's clearer that this spec needs to define all the components of the embedded package.

[jrray]

I checked in some code while I noodle around with ideas. There definitely isn't good test coverage for this territory because all the tests are passing, but this code is almost certainly still broken.

[codecov]

Codecov Report

Attention: 67 lines in your changes are missing coverage. Please review.

Comparison is base (e8cf549) 53.55% compared to head (1ec935c) 54.50%.
Report is 135 commits behind head on main.

❗ Current head 1ec935c differs from pull request most recent head 80776d3. Consider uploading reports for the commit 80776d3 to get more accurate results

Files	Patch %	Lines
...es/spk-schema/crates/ident/src/ident_optversion.rs	16.00%	21 Missing ⚠️
...spk-schema/crates/foundation/src/version/compat.rs	0.00%	12 Missing ⚠️
...ates/spk-schema/src/component_embedded_packages.rs	75.00%	8 Missing ⚠️
crates/spk-solve/crates/graph/src/graph.rs	87.75%	6 Missing ⚠️
...rates/spk-schema/crates/ident/src/parsing/ident.rs	0.00%	4 Missing ⚠️
crates/spk-schema/src/install_spec.rs	88.23%	4 Missing ⚠️
...ates/validation/src/validators/embedded_package.rs	78.94%	4 Missing ⚠️
crates/spk-solve/src/solver.rs	91.48%	4 Missing ⚠️
...lve/crates/validation/src/validators/components.rs	0.00%	2 Missing ⚠️
...ve/crates/validation/src/validators/pkg_request.rs	0.00%	1 Missing ⚠️
... and 1 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #744      +/-   ##
==========================================
+ Coverage   53.55%   54.50%   +0.95%     
==========================================
  Files         258      254       -4     
  Lines       20466    20107     -359     
==========================================
- Hits        10960    10959       -1     
+ Misses       9506     9148     -358     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[rydrman]

From the meeting today:

• generally a reasonable approach
• consider changing embedded to virtual packages that get embedded into components
  • what do we call packages that embedded other packages? (parent? host?)
• if you have a virt package defined that is not included in a component, is that an error? is that a lint issue? warning?
  • better to be an error at build time, but not at use/run time
• relates to / might conflict with Formalize embedded packages as a generic type #564
• this all might be more for v1, so in this PR the TODO is to flesh out more test cases and capture edge case behaviours

[jrray]

I added support for embedded multiple versions of the same package. It complicated the code by a large factor.

[jrray]

At this stage I think embedded_packages is a better name than embedded_components. It fits better since this is describing what packages are embedded within the component, with the added information of which components of the embedded package are being embedded.

[jrray]

Highlighting this change since it is the only test that had to be modified due to the changes in this PR. The spirit of the test is unchanged but the spec had to be modified.

[jrray]

Another angle to this problem came up today. Although argparse is bundled in the python standard library, it is also sometimes listed as a dependency of packages in pypi and in our environment the use of spk convert pip -- unittest2 ended up creating and depending on a python-argparse package.

I'd say there's a general problem here with respect to packages bundled in the system library that are also in pypi, and there's a category of that problem that argparse falls into where the package is old and not getting updates.

The python package basically needs a way to specify that it contains python-argparse, but the twist is that we don't want it to prevent (in the general sense) allowing a newer version of python-argparse to be brought into the environment. So part of this requires the functionality in this PR to add python-argparse as both a component and embedded, then the expected behavior would be:

1. make the argparse component included in the environment by default (because it is expected to be there via the standard library); but
2. allow a request for python-argparse to overrule the default request for the argparse and conflict with an explicit request for the argparse component; and
3. prevent the external python-argparse package from being a version lower than the version in the argparse component.

I think this might all be possible with this PR and the existing install.requirements functionality.

1. the argparse component will embed python-argparse but not be included by default
2. the top-level python package will have an install.requirement for python-argparse of at least the version that is bundled in python

When solving for python, the install.requirement will force the solver into either picking up a newer "real" python-argparse or it will pick up the argparse component via finding it through an embedded stub.

What I'm not sure already works [with this PR] is the solver finding a stub for an embedded package that comes from a component and having that add a request for that component to an existing request for the provider package.

Edit: I have learned a little more about our situation with argparse, and it involves a DCC that includes python but we're not "embedding" python in this case but allowing the DCC python and "real" python to coexist in the environment. To get things to (mostly) work some $PYTHONPATH shenanigans are making python find the site-package's argparse.py before the standard library's copy. I'm not sure if this fact entirely nullifies the issue being raised here.

[rydrman]

From the meeting today:

• default run components embed all install.embedded entries
• default build components DO NOT, but they do need to use the run component

install:
  components:
  - name: cmpt
    embedded: 
      - name:component
      - name:all # all components
      - name # this is an error, must specify components
  embedded:
  - pkg: name

[jrray]

From the meeting today:

* default `run` components embed all `install.embedded` entries

* default `build` components DO NOT, but they do need to `use` the `run` component

I'm not convinced that the extra complexity of implementing this adds any value over what this PR is already doing. If install.components[].embedded is empty, it is given a "fabricated" value where the components of install.embedded[] are mapped 1:1.

So for example:

pkg: demo/1.0.0
install:
  embedded:
    - pkg: embedded1/1.0.0
    - pkg: embedded2/1.0.0

expands to:

pkg: demo/1.0.0
install:
  components:
    - name: build
      embedded:
        - embedded1:build/1.0.0
        - embedded2:build/1.0.0
    - name: run
      embedded:
        - embedded1:run/1.0.0
        - embedded2:run/1.0.0
  embedded:
    - pkg: embedded1/1.0.0
      install:
        components:
          - name: build
          - name: run
    - pkg: embedded2/1.0.0
      install:
        components:
          - name: build
          - name: run

Changing how the default build and run components are generated feels a bit out of scope for this PR.

[rydrman]

This line seems like it's wrong for this function...

[jrray]

It's not wrong but I changed it a little.