Remove v0::Opt from the Package/Recipe Traits #563

rydrman · 2022-11-23T03:21:31Z

This changes the current inheritance mechanism from being calculated based on the specific Opt::inheritance field, instead expecting the package itself to identify downstream requests that need to be present.

codecov · 2022-11-23T03:45:10Z

Codecov Report

Merging #563 (d07e319) into main (e175bbc) will increase coverage by 0.06%.
The diff coverage is 67.15%.

@@            Coverage Diff             @@
##             main     #563      +/-   ##
==========================================
+ Coverage   53.93%   53.99%   +0.06%     
==========================================
  Files         248      248              
  Lines       19102    19241     +139     
==========================================
+ Hits        10302    10389      +87     
- Misses       8800     8852      +52

Impacted Files	Coverage Δ
crates/progress_bar_derive_macro/src/lib.rs	`0.00% <ø> (ø)`
crates/spk-cli/common/src/flags.rs	`22.22% <0.00%> (ø)`
crates/spk-schema/src/install_spec.rs	`100.00% <ø> (ø)`
crates/spk-schema/src/recipe.rs	`34.78% <0.00%> (ø)`
crates/spk-solve/src/io.rs	`46.49% <0.00%> (ø)`
crates/spk-solve/src/solver.rs	`85.46% <14.28%> (-0.85%)`	⬇️
crates/spk-schema/src/package.rs	`17.14% <30.00%> (+3.34%)`	⬆️
crates/spk-schema/src/v0/spec.rs	`77.27% <42.59%> (-7.15%)`	⬇️
crates/spk-schema/src/spec.rs	`76.59% <44.44%> (-2.82%)`	⬇️
...ates/validation/src/validators/var_requirements.rs	`78.94% <50.00%> (+1.16%)`	⬆️
... and 8 more

... and 2 files with indirect coverage changes

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

rydrman · 2022-12-17T22:09:04Z

I have found a way to maintain the old behavior and not need to error on missing requirements for either v1 or v0 (largely thanks to the fact that #594 negates the need to inject downstream build requirements)

I should be able to backport it to this branch in the future

jrray · 2023-01-31T00:11:17Z

This one needs a rebase.

rydrman · 2023-02-01T00:47:47Z

Thanks, rebased just now

rydrman · 2023-06-13T17:20:54Z

crates/progress_bar_derive_macro/src/lib.rs

                let renderer = Some(std::thread::spawn(move || {
                    if let Err(err) = bars.join() {
-                        tracing::error!("Failed to render commit progress: {err}");
+                        tracing::error!("Failed to render commit progress: {}", err);


this was causing errors for me in cargo 1.68 and seems harmless while we finish updating everything else internally

jrray · 2023-06-13T17:36:26Z

crates/spk-schema/crates/ident/src/request.rs

+            ));
+        }
+        if self.pin || other.pin {
+            // we cannot consider an unpinned request to contain any


pin == true is "unpinned"?

ya this seems to just be a typo

Actually, no - they are unpinned because the request is set as needing to be pinned still - but I'll clarify the comment

crates/spk-schema/crates/ident/src/request.rs

jrray · 2023-06-13T17:40:37Z

crates/spk-schema/crates/ident/src/request.rs

+        if !value.is_empty() {
+            f.write_char('/')?;
+            value.fmt(f)?;
+        } else if *pin {
+            f.write_str("/<fromBuildEnv>")?;
+        }


Should VarRequest::pin and VarRequest::value be combined into an enum (mutually exclusive)?

possibly, but I think that would greatly increase the size of this PR

I am going to give this a try, actually, as it might reduce some of the ambiguity you noted earlier. I'll give up if it get's too involved, though...

Okay, I added a separate commit that introduces an enum for this. I made the VarRequest generic over this type thinking that we could have an instance of it for packages that can only ever have a string value and no pin (since only recipes can pin during build) but it started getting too involved and I left it as-is which is still a bit of an improvement for rusty-ness

jrray · 2023-06-13T17:51:47Z

crates/spk-schema/src/requirements_list.rs

+                    return ours.contains(theirs);
+                }
+                (Request::Var(ours), Request::Var(theirs)) => {
+                    return ours.contains(theirs);


The return here means it will give up on the first thing it checks, compatible or incompatible. That doesn't seem right. Shouldn't this be looking to see if any element in self.iter() is compatible?

The requirements list is not supposed to allow multiple requests with the same name. I'll add a comment and also button up the FromIterator implementations that break this invariant.

Turns out we had some bugs-incognito here, but I've buttoned up the RequirementsList type and tried to make the interaction methods clearer.

What I'm still confused about is if self is something like:

- var: a/1.0.0 - var: b/1.0.0 - var: c/1.0.0

and theirs is var: b/1.0.0, won't this return incompatible when comparing the first element a/1.0.0 <> b/1.0.0?

I see what you are saying now, ya it looks like there are some additional checks for the name here

crates/spk-schema/crates/ident/src/request.rs

jrray · 2023-06-19T18:34:14Z

crates/spk-schema/crates/ident/src/request.rs

+#[derive(Clone, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)]
+pub enum PinnableValue {
+    FromBuildEnv,
+    Pinned(String),


Suggested change

Pinned(String),

Pinned(Arc<str>),

Don't need mutability here.

I saw https://youtu.be/A4cKi7PTJSs last night and I think it could help this project conserve memory and maybe improve performance. I'm interested in trying out this kind of change in a few areas, like for PkgNameBuf. A combination of this change and some kind of string interning mechanism for common strings like package names.

Interesting video, definitely could be super beneficial to the name bufs - I'll take a stab at the change here

jrray · 2023-06-19T18:38:03Z

crates/spk-schema/crates/ident/src/request.rs

A lot of missing coverage in this file.

crates/spk-schema/src/requirements_list.rs

jrray · 2023-06-19T18:48:46Z

crates/spk-schema/src/requirements_list.rs

+                    return ours.contains(theirs);
+                }
+                (Request::Var(ours), Request::Var(theirs)) => {
+                    return ours.contains(theirs);


What I'm still confused about is if self is something like:

- var: a/1.0.0 - var: b/1.0.0 - var: c/1.0.0

and theirs is var: b/1.0.0, won't this return incompatible when comparing the first element a/1.0.0 <> b/1.0.0?

jrray · 2023-06-27T21:02:19Z

crates/spk-schema/src/requirements_list.rs

+/// Requirements lists cannot contain multiple requests with the
+/// same name, requiring instead that they be combined into a single
+/// request as needed.


Anecdotally, we have some packages published that have multiple install requirements specified for the same package. This generates a warning when the solver walks over them. We've deprecated the packages to in theory make the solver not consider them, but the solver still has to read the packages to find out they are deprecated... which generates the warning.

This changes the current inheritance mechanism from being calculated based on the specific v0::Opt::inheritance field, instead expecting the package itself to identify downstream requests that need to be present. Signed-off-by: Ryan Bottriell <rbottriell@ilm.com>

Signed-off-by: Ryan Bottriell <rbottriell@ilm.com>

The VarRequest had a pin field used to denote when fromBuildEnv was given and this field was mutually exclusive to a provided value. An enum works better in this case. Ideally, the package would not allow this field to be pinned, only the recipe. For now, I have not made that change only this first one to introduce an enum. Signed-off-by: Ryan Bottriell <rbottriell@ilm.com>

Signed-off-by: Ryan Bottriell <rbottriell@ilm.com>

rydrman added agenda item Items to be brought up at the next dev meeting Rust API Pertaining to the public rust API of the library labels Nov 23, 2022

rydrman added this to the V1 Spec milestone Nov 23, 2022

rydrman requested review from dcookspi and jrray November 23, 2022 03:21

rydrman self-assigned this Nov 23, 2022

rydrman changed the title ~~Remove use of v0::Opt from binary builder~~ Remove v0::Opt from the Package/Recipe Traits Nov 23, 2022

rydrman mentioned this pull request Dec 23, 2022

296 v1alpha1 #598

Closed

11 tasks

rydrman force-pushed the 256-ident-types branch from db5bceb to 3043738 Compare December 24, 2022 17:34

rydrman force-pushed the 256-ident-types branch 2 times, most recently from 741933c to f4ee76d Compare January 4, 2023 23:57

Base automatically changed from 256-ident-types to master January 5, 2023 23:28

rydrman force-pushed the 296-embedded-stub-trait branch from 658caf7 to e59695f Compare February 1, 2023 00:47

rydrman force-pushed the 296-embedded-stub-trait branch from e59695f to 4173e28 Compare February 23, 2023 04:19

rydrman force-pushed the 296-embedded-stub-trait branch 2 times, most recently from f84f53e to 07942f4 Compare June 13, 2023 17:16

rydrman commented Jun 13, 2023

View reviewed changes

jrray reviewed Jun 13, 2023

View reviewed changes

crates/spk-schema/crates/ident/src/request.rs Outdated Show resolved Hide resolved

jrray reviewed Jun 13, 2023

View reviewed changes

rydrman force-pushed the 296-embedded-stub-trait branch from 07942f4 to 541c7bd Compare June 14, 2023 16:00

rydrman requested a review from jrray June 14, 2023 18:36

jrray requested changes Jun 19, 2023

View reviewed changes

jrray reviewed Jun 27, 2023

View reviewed changes

jrray approved these changes Jun 27, 2023

View reviewed changes

rydrman added 7 commits June 28, 2023 18:25

Remove unused function from install spec

70c7040

Signed-off-by: Ryan Bottriell <rbottriell@ilm.com>

Revamp RequirementsList to ensure no duplicate request names invariant

ee89d8f

Signed-off-by: Ryan Bottriell <rbottriell@ilm.com>

Fix typos in old doc comments

3ec323e

Signed-off-by: Ryan Bottriell <rbottriell@ilm.com>

Use Arc<str> rather than String for pinned value

8d05327

Signed-off-by: Ryan Bottriell <rbottriell@ilm.com>

Make Compatibility enums comparable

8aaeee2

Signed-off-by: Ryan Bottriell <rbottriell@ilm.com>

rydrman force-pushed the 296-embedded-stub-trait branch 2 times, most recently from fdc3eea to 7c08bd3 Compare June 29, 2023 02:15

Check names in ReqList::contains_request

d07e319

Signed-off-by: Ryan Bottriell <rbottriell@ilm.com>

rydrman force-pushed the 296-embedded-stub-trait branch from 7c08bd3 to d07e319 Compare June 29, 2023 02:43

jrray approved these changes Jun 29, 2023

View reviewed changes

rydrman merged commit ea43485 into main Jun 29, 2023

rydrman deleted the 296-embedded-stub-trait branch June 29, 2023 03:32

rydrman mentioned this pull request Jul 26, 2023

Implement "Strong" inheritance #811

Merged

Remove v0::Opt from the Package/Recipe Traits #563

Remove v0::Opt from the Package/Recipe Traits #563

Uh oh!

Conversation

rydrman commented Nov 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Nov 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

rydrman commented Dec 17, 2022

Uh oh!

jrray commented Jan 31, 2023

Uh oh!

rydrman commented Feb 1, 2023

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

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!

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

rydrman commented Nov 23, 2022 •

edited

Loading

codecov bot commented Nov 23, 2022 •

edited

Loading