Variant Trait #635

rydrman · 2023-01-24T03:39:30Z

Defines the Variant trait that provides build options as usual, but also has an optional name and set of additional requirements to be used when building. These are both not exercised in this pull request but are required for the v1 package spec features to come.

This pull request relates to #433 as it required updating the binary package builder to take a variant to be built. It takes a slightly different approach, however, requiring that the variant is selected and/or defined before calling BinaryPackageBuilder::build which is so far less intrusive. It may not achieve everything that #278 did, but I don't know that branch well enough to say so maybe @jrray can comment on whether we would hit limitations in this approach.

codecov · 2023-01-24T04:03:26Z

Codecov Report

Merging #635 (960b1b2) into master (cb66714) will increase coverage by 0.30%.
The diff coverage is 75.74%.

@@            Coverage Diff             @@
##           master     #635      +/-   ##
==========================================
+ Coverage   57.17%   57.47%   +0.30%     
==========================================
  Files         222      224       +2     
  Lines       17169    17297     +128     
==========================================
+ Hits         9816     9942     +126     
- Misses       7353     7355       +2

Impacted Files	Coverage Δ
crates/parsedbuf/src/lib.rs	`78.26% <0.00%> (+1.33%)`	⬆️
crates/spk-cli/cmd-build/src/cmd_build.rs	`85.71% <ø> (ø)`
crates/spk-cli/common/src/exec.rs	`0.00% <0.00%> (ø)`
crates/spk-cli/group4/src/cmd_view.rs	`0.00% <0.00%> (ø)`
crates/spk-exec/src/exec.rs	`61.64% <0.00%> (+1.64%)`	⬆️
crates/spk-schema/src/error.rs	`50.00% <ø> (ø)`
crates/spk-solve/crates/solution/src/solution.rs	`70.10% <0.00%> (ø)`
crates/spk-schema/src/recipe.rs	`34.78% <38.46%> (+4.01%)`	⬆️
crates/spk-cli/cmd-test/src/cmd_test.rs	`81.05% <50.00%> (+0.20%)`	⬆️
crates/spk-schema/crates/ident/src/request.rs	`81.69% <60.00%> (-0.38%)`	⬇️
... and 9 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

jrray · 2023-03-07T21:13:39Z

Responding to my being tagged here, getting the tests in #653 to pass would be what I consider required to allow this branch to replace #433.

rydrman · 2023-03-12T03:46:20Z

Thanks, @jrray - I've gotten those tests to pass, with some changes both to my code and the tests. I added the heuristics that you had in #433, with minor tweaks, and run them when the spec is deserialized. Above what I already had this code is now leaning farther into the variant trait, using it in place of an OptionMap for functions like resolve_options and get_build_requirements

jrray

This is nice, I do like the tidiness of the Variant trait.

I didn't spot what you changed about the tests, beyond filling in TODO comments I left in there. Do you make any changes to the expected results of any of the tests?

crates/spk-cli/cmd-build/src/cmd_build_test.rs

jrray · 2023-04-05T22:54:27Z

crates/spk-cli/cmd-make-binary/src/cmd_make_binary.rs

                }

-                tracing::info!("building variant {}", opts.format_option_map());
+                tracing::info!("building variant:\n{variant}");


The variant stringification could use some polish, this is what I see if a variant introduces a novel package dependency:

INFO building variant: Options: {arch=x86_64, centos=7, distro=centos, os=linux, spi-platform=~2023.1.1.2} Additional Requirements: - Pkg(PkgRequest { pkg: RangeIdent { repository_name: None, name: PkgNameBuf("spi-platform"), components: {}, version: VersionFilter { rules: {LowestSpecified(LowestSpecifiedRange { specified: 4, base: Version { parts: VersionParts { parts: [2023, 1, 1, 2], plus_epsilon: false }, pre: TagSet { tags: {} }, post: TagSet { tags: {} } } })} }, build: None }, prerelease_policy: ExcludeAll, inclusion_policy: Always, pin: None, required_compat: Some(Binary), requested_by: {"spi-platform/~2023.1.1.2": [Variant]} })

Thanks, I'm adding the shorter format for this

jrray · 2023-04-05T23:03:45Z

crates/spk-cli/group4/src/cmd_view.rs

-        for (index, variant) in recipe.default_variants().iter().enumerate() {
+        let default_variants = recipe.default_variants();
+        for (index, variant) in default_variants.iter().enumerate() {
            println!("{index}: {variant}");


The multiple line stringification output for variants pose a compatibility/usability concern for scripts that just want to count the number of lines to get the answer of how many variants exist in a package.

In reality we haven't stopped using spk num-variants <filename> yet, which prints a number. Automation wants to easily get a number and generate CI jobs per index.

$ ~/git/spk/target/debug/spk num-variants package.spk.yaml 2

$ ~/git/spk/target/debug/spk view --variants package.spk.yaml 0: options: {spi-platform: ~2023.1.1.2} additional requirements: - Pkg(PkgRequest { pkg: RangeIdent { repository_name: None, name: PkgNameBuf("spi-platform"), components: {}, version: VersionFilter { rules: {LowestSpecified(LowestSpecifiedRange { specified: 4, base: Version { parts: VersionParts { parts: [2023, 1, 1, 2], plus_epsilon: false }, pre: TagSet { tags: {} }, post: TagSet { tags: {} } } })} }, build: None }, prerelease_policy: ExcludeAll, inclusion_policy: Always, pin: None, required_compat: Some(Binary), requested_by: {"spi-platform/~2023.1.1.2": [Variant]} }) 1: options: {maya2023-platform: ~2023.1.1.2} additional requirements: - Pkg(PkgRequest { pkg: RangeIdent { repository_name: None, name: PkgNameBuf("maya2023-platform"), components: {}, version: VersionFilter { rules: {LowestSpecified(LowestSpecifiedRange { specified: 4, base: Version { parts: VersionParts { parts: [2023, 1, 1, 2], plus_epsilon: false }, pre: TagSet { tags: {} }, post: TagSet { tags: {} } } })} }, build: None }, prerelease_policy: ExcludeAll, inclusion_policy: Always, pin: None, required_compat: Some(Binary), requested_by: {"maya2023-platform/~2023.1.1.2": [Variant]} })

I'll use a one-line version for this as well - not perfect but at least more predictable

$ spk view --variants test.spk.yaml 0: options: {python: 3, stdfs: 1.0} additional requirements: python/3 stdfs/1.0

crates/spk-schema/src/v0/spec.rs

crates/spk-schema/src/v0/variant.rs

jrray · 2023-04-05T23:19:56Z

crates/spk-schema/src/v0/variant.rs

+            f.write_str("\nadditional requirements:\n")?;
+            for r in self.requirements.iter() {
+                f.write_str(" - ")?;
+                std::fmt::Debug::fmt(r, f)?;


The debug output is ugly to display to users. Did you have a specific reason to use it? I put some example output in other comments.

jrray · 2023-04-05T23:21:53Z

crates/spk-schema/src/variant.rs

+            for request in requirements.iter() {
+                f.write_char('\n')?;
+                f.write_str(" - ")?;
+                f.write_fmt(format_args!("{request:?}"))?;


... or I guess this is where the debug output is coming from.

I don't think that we have Display implemented for a full request - but maybe that has changed, or I suppose there's likely something in the solver formatting that I can use though I'm not sure off the top of my head if that shows additional bits above the version range

we did not, but it was easy to add as the var and pkg types already did

Signed-off-by: Ryan Bottriell <ryan@bottriell.ca>

These have no purpose yet, but will be used by v1 spec variants to add additional requirements to the package that are not present for other variants. Signed-off-by: Ryan Bottriell <ryan@bottriell.ca>

Extract tests from #433 to test alternate implementations for this behavior. Signed-off-by: J Robert Ray <jrray@jrray.org> Signed-off-by: Ryan Bottriell <ryan@bottriell.ca>

This provides access to the additional requests insode the version-specific generation code so that the generated binary build can include additional options which represent those added options Signed-off-by: Ryan Bottriell <ryan@bottriell.ca>

Signed-off-by: Ryan Bottriell <ryan@bottriell.ca>

This allows for the additional requirements to be considered, and for the recipe to understand how build opts might be inserted for a build of that variant. There's some anooying overlap of work now where the same variant ends up being calculated multiple times in the process of running a build, but there doesn't seem to be any great ways to add efficiency without re-exposing particualr aspects of the v0 spec Signed-off-by: Ryan Bottriell <ryan@bottriell.ca>

Signed-off-by: Ryan Bottriell <ryan@bottriell.ca>

rydrman added the Rust API Pertaining to the public rust API of the library label Jan 24, 2023

rydrman added this to the V1 Spec milestone Jan 24, 2023

rydrman requested review from dcookspi and jrray January 24, 2023 03:39

rydrman self-assigned this Jan 24, 2023

rydrman mentioned this pull request Jan 24, 2023

296 v1alpha1 #598

Closed

11 tasks

rydrman force-pushed the 296-variant-trait branch from 27ba68d to 06690c0 Compare February 23, 2023 04:11

jrray mentioned this pull request Mar 2, 2023

Tests from pkg-req-from-variant-redux branch #653

Closed

rydrman force-pushed the 296-variant-trait branch from 0f662dc to 3856e52 Compare March 12, 2023 03:42

jrray requested changes Apr 5, 2023

View reviewed changes

This was referenced Apr 6, 2023

Extend "@stage" syntax to allow variant to be specified #493

Merged

variant-aware builds (take two) #433

Closed

rydrman and others added 8 commits April 7, 2023 15:05

Add a Variant trait that's not just an option map

3529fe3

Signed-off-by: Ryan Bottriell <ryan@bottriell.ca>

Implement additional requirements for v1 variants

0958d51

These have no purpose yet, but will be used by v1 spec variants to add additional requirements to the package that are not present for other variants. Signed-off-by: Ryan Bottriell <ryan@bottriell.ca>

Tests from pkg-req-from-variant-redux branch

6ab0023

Extract tests from #433 to test alternate implementations for this behavior. Signed-off-by: J Robert Ray <jrray@jrray.org> Signed-off-by: Ryan Bottriell <ryan@bottriell.ca>

Define a v0::Variant type that loads from an option map

c55bb7e

Signed-off-by: Ryan Bottriell <ryan@bottriell.ca>

Use Ext trait pattern instead of ExtensionVariant type

54e5ccd

Signed-off-by: Ryan Bottriell <ryan@bottriell.ca>

Add one-line formatting for variants

960b1b2

Signed-off-by: Ryan Bottriell <ryan@bottriell.ca>

rydrman force-pushed the 296-variant-trait branch from dc11e70 to 960b1b2 Compare April 7, 2023 22:08

rydrman requested a review from jrray April 7, 2023 22:19

jrray approved these changes Apr 7, 2023

View reviewed changes

rydrman merged commit b10c261 into master Apr 8, 2023

rydrman deleted the 296-variant-trait branch April 8, 2023 01:44

jrray mentioned this pull request Apr 26, 2023

Require specific build when testing in install phase #583

Merged

Variant Trait #635

Variant Trait #635

Uh oh!

Conversation

rydrman commented Jan 24, 2023

Uh oh!

codecov bot commented Jan 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jrray commented Mar 7, 2023

Uh oh!

rydrman commented Mar 12, 2023

Uh oh!

jrray left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Jan 24, 2023 •

edited

Loading