Adding host_compat setting to package BuildSpec #922

dcookspi · 2023-11-25T00:11:52Z

This add the host_compat field a package's BuildSpec, based on the discussion in the related ticket, e.g.

package: my-pkg/1.0
build:
    host_compat: distro
    options:
     -  pkg: some-pkg/2.3.4
....

The value of host_compat the determines which host os related options are automatically added to the package build's options. It also describes the kind of host os compatibility required for builds of this package. The supported values are: distro (default), arch, os, and any.

This also adds a host_compat section with validate field to spk's config file. This contols whether checks are run on a build's option to ensure no incompatible host os related options are also being by a package variant. The default is for these checks to be disabled (should it be that?).

This does not include:

changes to the spk lint command, there'll be another PR for that (DRAFT: Add host_compat lints (requires spk-lint-update) #929 which is based on this and Capture unrecognized fields with lint #851)
configurable host_compat values, automatically added option name, or incompatible option names for host_compat values. But there are todo makers where they could be added later.

Todo:

Decide how to resolve the <distroname> vars comparison issue, e.g. 9.2 vs 9.3 for rocky 9's being treated as equal (in the solver and builds, so any comparisons) if those versions of the distro are application compatible.
- I'm going to leave this as it is right now: capture 9.2 or 9.3, the full number, and leave possible comparison rules/changes for the future.

crates/spk-cli/group2/src/cmd_new.rs

crates/spk-schema/src/build_spec.rs

crates/spk-schema/src/error.rs

dcookspi · 2023-11-30T01:55:25Z

Added a todo for what to do about the os version comparison discussison we had today.

dcookspi · 2023-11-30T18:03:32Z

Added codes to the new errors.

dcookspi · 2023-12-06T20:48:45Z

I've removed the validation check and error, and the config option to enable/disable it. ~~I've commented out but left in the disallowed fields constants and method because they'll get used in the planned host_compat lint warning (in a later PR).~~

Supports these values: distro, arch, os, and any. The value determines which host os related options are automatically added to the package build's options. Adds 'host_compat' section with 'validate' setting to spk's config file to control whether checks are run on a build's option to ensure there no host os related options set that are incopmatible with the package's host_compat value. Signed-off-by: David Gilligan-Cook <[email protected]>

Signed-off-by: David Gilligan-Cook <[email protected]>

dcookspi · 2023-12-07T20:30:53Z

I've fixed an issue with the --no-host option so host_compat works as expected with it. This should be ready for another review now, @rydrman.

crates/spk-cli/group2/src/cmd_new.rs

crates/spk-schema/src/build_spec.rs

rydrman · 2023-12-08T05:14:50Z

crates/spk-schema/src/build_spec.rs

+pub enum HostCompat {
+    /// Package can only be used on the same OS distribution
+    #[default]
+    Distro,
+    /// Package can be used anywhere that has the same OS and cpu type
+    Arch,
+    /// Package can be used on the same OS with any cpu or distro
+    Os,
+    /// Package can be used on any Os
+    Any,
+}


Has @lgritz been able to put eyes on this addition? I am having memories of talking about being able to handle CPU capabilities, etc, and wondering if we should consider making host_compat a list - or at least not lock ourselves into a state where cpu architecture is tied to os/distro. Eg you can't say "any arch but must be the same distro"...

sorry to throw another question into the mix here...

I'm not sure. I think @lgritz might been some of the internal meetings where it was mentioned, @jrray may know more. But I don't think we got into details.

The host_compat: enum values in this change are an initial set of possibilities. They expand into lists of vars, most with known names, one with a dynamic name (the value of the distro host option). We could add other host_compat values in future if we need to, e.g. a 'DistroOnly' value could expand into just the <distroname>=<version> var option (if I understand what you mean by "any arch but the same distro" correctly). We could add other enum values that turn into other, stranger sets of vars, later, as needed.

Eg you can't say "any arch but must be the same distro"...

David already answered this but to put it another way, individual specs can set host_compat: Any and be back to the original behavior, and then add back in whatever combination of host vars they want.

I really hope this PR doesn't get bogged down over perceived limitations of what this is doing, in my mind we're not introducing any new concepts here but just putting some more formal structure around what the existing behavior has been. It's important for us to get these vars added by default ASAP so we can make progress on our new OS testing.

It does seem a little counter-intuitive to read a spec with host_compat: Any that, in fact, has a very specific host compat defined elsewhere.

My goal is never to "bog down" a PR, and I hope you don't see these discussions as purely bureaucratic. My goal is to make sure we are thinking about these things from all angles. It's very hard to go backwards in spk and with every feature we add to fix a specific need we add complexity for new folks to learn and understand. I just want to make sure that the design feels intuitive for newcomers and doesn't shoehorn us into any undesirable futures...

Just trying to reconcile the above in my brain - maybe we are complicating things by abstracting what's going on here? Aka Any actually means inject no vars, but doesn't preclude them existing elsewhere....

As one idea, we could instead be explicit and call the field something like auto_host_vars, changing Any to None.

I can rename the field and value.

I've changed this over to use auto_host_vars and None. The internals haven't changed.

rydrman · 2023-12-08T05:19:14Z

crates/spk-schema/src/build_spec.rs

+                            return Err(Error::HostOptionNotValidDistroNameError(
+                                distro_name.to_string(),
+                                err,
+                            ))


I still have concerns with this being an error case, but curious about your thoughts...

The value that's incorrect is loaded from the host machine for an option that's injected - I think there's a potential for users to have no idea what went wrong or how to fix this and we should at least have a good explanation and possible work-around --- or maybe we need some kind of reliable conversion+fallback for distro names so that it never fails in this way?

The main question is - how do you recover if you run spk on a machine where the distro name is seen as invalid? I know you have "contact admin" now but what would the admin even do?

I think it's worth erroring. But I can see that stopping spk dead isn't good either. I'm not sure there's a good easy solution.

I looked again at what the existing host_options() function does, and updated this code to do the same. The host_options() displays a warning and skips over making the <distroname> var, which is slightly concerning, but its consistent for now. It's something we can also ticket up to look at in-depth in future, if necessary.

Here are some options:

Error and Stop spk, until there's a valid distroname - if that's even posssible for some sysadmin to do sensibly (the original code did this)

Warn and Skip making the <distroname>=<version_id> var opt at all (the current code does this)

Warn and Fallback, use either:

some predefined value (unknown_distro=<version_id>?), or

convert the distroname into a valid opt name, which I think at the moment involves:

lowercasing it,

filtering out all character that aren't letters, numbers, _'s and -'s,

then making sure the leftover string is > 2 and < 64 chars, by either padding or truncating it.

Internally, for name-like types we often add a new_lossy constructor that does the kind of translation that you mention in a predictable and infallible way. That seems ideal to me in this case because it would still give a consistent value on which to check compatibility, but not fail as we're discussing.

In the future, once we allow a custom host options command then we could allow it to yield an alternative name, but the above feels like the best default behavior to me

I can add a constructor that converts the name to a valid one.

I've added a new_lossy() and unknown_distro() methods.

docs/ref/spec.md

Co-authored-by: Ryan Bottriell <[email protected]> Signed-off-by: David Gilligan-Cook <[email protected]>

Signed-off-by: David Gilligan-Cook <[email protected]>

jrray · 2023-12-09T00:03:58Z

crates/spk-schema/src/build_spec.rs

+}
+
+impl HostCompat {
+    pub fn is_default(&self) -> bool {


I recently learned about the variantly crate which unfortunately doesn't generate an is_default method but does generate other useful methods for enums.

jrray · 2023-12-09T02:27:01Z

crates/spk-schema/src/build_spec.rs

+pub enum HostCompat {
+    /// Package can only be used on the same OS distribution
+    #[default]
+    Distro,
+    /// Package can be used anywhere that has the same OS and cpu type
+    Arch,
+    /// Package can be used on the same OS with any cpu or distro
+    Os,
+    /// Package can be used on any Os
+    Any,
+}


Eg you can't say "any arch but must be the same distro"...

David already answered this but to put it another way, individual specs can set host_compat: Any and be back to the original behavior, and then add back in whatever combination of host vars they want.

I really hope this PR doesn't get bogged down over perceived limitations of what this is doing, in my mind we're not introducing any new concepts here but just putting some more formal structure around what the existing behavior has been. It's important for us to get these vars added by default ASAP so we can make progress on our new OS testing.

Signed-off-by: David Gilligan-Cook <[email protected]>

string is not a valid var option name. Signed-off-by: David Gilligan-Cook <[email protected]>

Signed-off-by: David Gilligan-Cook <[email protected]>

crates/spk-cli/group2/src/cmd_new.rs

crates/spk-schema/crates/foundation/src/name/mod.rs

crates/spk-schema/src/build_spec_test.rs

docs/ref/spec.md

crates/spk-schema/src/build_spec.rs

Co-authored-by: Ryan Bottriell <[email protected]> Signed-off-by: David Gilligan-Cook <[email protected]>

Signed-off-by: David Gilligan-Cook <[email protected]>

dcookspi requested review from jrray and rydrman November 25, 2023 00:11

dcookspi self-assigned this Nov 25, 2023

dcookspi linked an issue Nov 25, 2023 that may be closed by this pull request

Host options should be automatic and opt-out #917

Closed

dcookspi added SPI AOI Area of interest for SPI SPI-0.39 enhancement New feature or request labels Nov 27, 2023

rydrman reviewed Nov 28, 2023

View reviewed changes

dcookspi force-pushed the 917-host-options-should-be-automatic-and-opt-out branch from efb0cee to 2087873 Compare November 30, 2023 01:43

dcookspi requested a review from rydrman November 30, 2023 01:55

dcookspi added 7 commits December 6, 2023 12:50

Fix for digest value in import test changing on a different host os

b510e26

Signed-off-by: David Gilligan-Cook <[email protected]>

Updates spk new's starter package spec to include host_compatibility

2eea8b6

Signed-off-by: David Gilligan-Cook <[email protected]>

Updates error message, help, naming, default, constants, and comments

7a99a1a

Signed-off-by: David Gilligan-Cook <[email protected]>

Adds a code for each of the new host_compat related errors.

9d4b430

Signed-off-by: David Gilligan-Cook <[email protected]>

Adding more constants

6f29278

Signed-off-by: David Gilligan-Cook <[email protected]>

Removes host_compat validation check, error, and config option

4287114

Signed-off-by: David Gilligan-Cook <[email protected]>

dcookspi force-pushed the 917-host-options-should-be-automatic-and-opt-out branch from 92d2ec4 to 4287114 Compare December 6, 2023 21:10

Removed commented out code.

665673b

Signed-off-by: David Gilligan-Cook <[email protected]>

dcookspi mentioned this pull request Dec 7, 2023

DRAFT: Add host_compat lints (requires spk-lint-update) #929

Open

1 task

Update to work with --no-host options

20462e2

Signed-off-by: David Gilligan-Cook <[email protected]>

rydrman reviewed Dec 8, 2023

View reviewed changes

dcookspi and others added 2 commits December 8, 2023 14:24

Update comments

d7fce83

Co-authored-by: Ryan Bottriell <[email protected]> Signed-off-by: David Gilligan-Cook <[email protected]>

Updated docs

f5541aa

Signed-off-by: David Gilligan-Cook <[email protected]>

dcookspi force-pushed the 917-host-options-should-be-automatic-and-opt-out branch from ec9f385 to f5541aa Compare December 8, 2023 22:58

Updated error handling to warn and skip instead of erroring

6428d4f

Signed-off-by: David Gilligan-Cook <[email protected]>

dcookspi requested a review from rydrman December 9, 2023 02:08

jrray reviewed Dec 9, 2023

View reviewed changes

dcookspi added 4 commits December 11, 2023 13:14

Changed host_compat: to auto_host_vars: and Any to None.

6131d75

Signed-off-by: David Gilligan-Cook <[email protected]>

Adds new_lossy method to create a valid option name even if the given

2abfb33

string is not a valid var option name. Signed-off-by: David Gilligan-Cook <[email protected]>

Moved OptName's new_lossy to OptNameBuf, updated warning messages

fcd2c25

Signed-off-by: David Gilligan-Cook <[email protected]>

Adds test for new lossy

2d07c49

Signed-off-by: David Gilligan-Cook <[email protected]>

dcookspi requested a review from jrray December 12, 2023 01:06

rydrman reviewed Dec 12, 2023

View reviewed changes

Updates to comments and tests

d5df810

Co-authored-by: Ryan Bottriell <[email protected]> Signed-off-by: David Gilligan-Cook <[email protected]>

dcookspi force-pushed the 917-host-options-should-be-automatic-and-opt-out branch from 8417691 to d5df810 Compare December 12, 2023 18:01

Removes old comments

e1f145a

Signed-off-by: David Gilligan-Cook <[email protected]>

dcookspi requested a review from rydrman December 12, 2023 18:32

rydrman approved these changes Dec 12, 2023

View reviewed changes

dcookspi merged commit 79ad8fe into main Dec 12, 2023

dcookspi deleted the 917-host-options-should-be-automatic-and-opt-out branch December 12, 2023 20:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding host_compat setting to package BuildSpec #922

Adding host_compat setting to package BuildSpec #922

dcookspi commented Nov 25, 2023 •

edited

Loading

dcookspi commented Nov 30, 2023

dcookspi commented Nov 30, 2023

dcookspi commented Dec 6, 2023 •

edited

Loading

dcookspi commented Dec 7, 2023

rydrman Dec 8, 2023

dcookspi Dec 8, 2023 •

edited

Loading

jrray Dec 9, 2023

rydrman Dec 9, 2023

rydrman Dec 9, 2023

dcookspi Dec 11, 2023

dcookspi Dec 12, 2023

rydrman Dec 8, 2023

dcookspi Dec 9, 2023

rydrman Dec 9, 2023

dcookspi Dec 11, 2023

dcookspi Dec 12, 2023

jrray Dec 9, 2023

jrray Dec 9, 2023

Adding host_compat setting to package BuildSpec #922

Adding host_compat setting to package BuildSpec #922

Conversation

dcookspi commented Nov 25, 2023 • edited Loading

dcookspi commented Nov 30, 2023

dcookspi commented Nov 30, 2023

dcookspi commented Dec 6, 2023 • edited Loading

dcookspi commented Dec 7, 2023

Choose a reason for hiding this comment

dcookspi Dec 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcookspi commented Nov 25, 2023 •

edited

Loading

dcookspi commented Dec 6, 2023 •

edited

Loading

dcookspi Dec 8, 2023 •

edited

Loading