Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Host options should be automatic and opt-out #917

Closed
jrray opened this issue Nov 21, 2023 · 5 comments · Fixed by #922 · May be fixed by #929
Closed

Host options should be automatic and opt-out #917

jrray opened this issue Nov 21, 2023 · 5 comments · Fixed by #922 · May be fixed by #929
Assignees
Labels
enhancement New feature or request package recipe Packaging recipes QoL quality of life fixes SPI AOI Area of interest for SPI

Comments

@jrray
Copy link
Collaborator

jrray commented Nov 21, 2023

Problem

I have started experimenting with using spk with two different OSes sharing the same origin repo and it has come to my attention that some of our packages, and perhaps a lot of them, are written like this:

build:
  options:
    - var: os
    - var: arch
    - var: centos
    # ... other non-host options

There are two problems here. First off, this is unfortunately missing - var: distro which means that when solving on a different linux distro, the solver finds no reason to reject builds for centos.

 INITIAL REQUEST {arch=x86_64}
 INITIAL REQUEST {distro=rocky}
 INITIAL REQUEST {os=linux}
 INITIAL REQUEST {rocky=9.2}

arch and os still have the same values as on centos. No centos host option is present when on rocky, so it doesn't reject builds with centos=7. And crucially, there is no distro=centos to cause a conflict with distro=rocky. I can't prevent the solver from picking centos builds of this package when running on rocky.

The second problem is the fact that the recipe has - var: centos hard-coded into it. It is desirable to have this var in my package, but it is undesirable to have to add it manually, or have to edit it to be "rocky" when building on rocky, or maybe even just adding all the distros to the list:

build:
  options:
    - var: centos
    - var: rocky
    # predict the future...
    - var: distro3
    - var: distro4

Proposal

It is safer and better if these vars are automatically added to the build options at build time, unless the package author elects to opt-out of this behavior using some new build property to control that.

  • Package authors may not know when they need these vars.
  • Package authors may make mistakes and not include all the vars they should.
  • Adding the vars when they are not needed is "harmless" whereas not adding them when they are needed is dangerous.
  • Experienced package authors who know what they are doing can opt-out.

Automatically adding the host vars has the added bonus that the appropriate -var: <distro name> var can be added without having to spell it out in the recipe. For those experienced package authors that want control, perhaps there is already a way with the templating mechanism to substitute in the detected distro name?

We need a new build section to opt-out to auto-adding these vars. This needs more thought but the goal is to have a way in the recipe to turn it off:

See later comment for revised proposal

build:
  settings:
    AutomaticHostOptions: false

The default would be true and the build options would get the [usually] four options that host_options calculates injected into the build.options list. Just the var names, with no values, like we usually write into our recipes (when we remember to!).

@rydrman
Copy link
Collaborator

rydrman commented Nov 21, 2023

History

There is a tangential conversation here that never happened in #37, and also a long-standing desire from ILM to add a configurable external executable and/or static config file values for gathering these host options - for studio-specific additions.

I'd like to think about this a little more, but I will add the historical context here:

There either was a moment when this was the default behavior, or we discussed it but, in either case, I remember the outcome. We decided that adding host options by default was equally unexpected for python developers, who's packages could fail to resolve because of these options. They key point of discussion was that it was more reasonable to expect cpp developers (or other maintainers who care about these options) to include them rather than ask python developers to understand the context enough to disable all or some of them.

Validation?

As an alternative idea (and as a man with a hammer) maybe we can leverage the upcoming #911 changes to attempt to steer packages that install suspect files to include relevant host options.

@jrray
Copy link
Collaborator Author

jrray commented Nov 22, 2023

After a review of our repo, only 13% of builds have the distro var defined.

42% of builds have no host vars defined at all. Without a manual review of each package, it is unknown if they were omitted on purpose or left out by accident. 46% of builds have some host vars defined but not distro. 😞

@jrray
Copy link
Collaborator Author

jrray commented Nov 22, 2023

We decided that adding host options by default was equally unexpected for python developers, who's packages could fail to resolve because of these options. They key point of discussion was that it was more reasonable to expect cpp developers (or other maintainers who care about these options) to include them rather than ask python developers to understand the context enough to disable all or some of them.

I definitely have a different stance today, now that I've had a chance to understand the stakes. I already made my case above, safety first is more important than inconveniencing a group of developers.

What bothers me the most about what we have now is it is possible to leave out the host vars and there is no way to know if it was intentional or accidental. At least if there is an opt-out mechanism you know someone either thought about it, or copied from an example that had it and didn't understand what it was and left it in.

@jrray
Copy link
Collaborator Author

jrray commented Nov 22, 2023

Revising my proposal a bit, I would like there to be a new property to specify if a package is architecture/os/distro specific or not.

build:
  # Set what level of cross-platform compatibility the built package should have.
  # - Full: package can be used on any OS
  # - Arch: package can be used anywhere that has the same cpu type
  # - Distro: package can only be used on the same OS distribution
  package_binary_compatibility: Distro

spk lint should print a lint for packages that have not defined build.package_binary_compatibility.

Distro would be the hard-coded default if unspecified and is the most restrictive. We could make the default configurable via the spk config file, though in my view it is dangerous to have anything besides Distro be the default.

At build time...

Distro would inject the following vars:

  • os
  • arch
  • distro
  • <distro_name> (like "centos")

Arch would inject the following vars:

  • os
  • arch

[Optional] It would be a build error if the distro var is manually defined in the recipe when the mode is Arch.

Full would not inject any vars.

[Optional] It would be a build error any of os, arch, or distro vars are manually defined in the recipe when the mode is Full.


SPI needs a feature like this to prevent the continued publishing of packages that are incorrectly falling into the Full or Arch categories due to user error, as there are currently no safeguards in place to stop this kind of mistake.

We might later swap out the list of generated host var names and their collected values with a custom script, per @rydrman 's comment.

We can add a new validation rule that checks the types of files being created and compares that with the configured setting. For example, if an ELF binary is present and the package is configured as Full.

@jrray jrray added enhancement New feature or request package recipe Packaging recipes QoL quality of life fixes labels Nov 22, 2023
@dcookspi dcookspi added the SPI AOI Area of interest for SPI label Nov 22, 2023
@rydrman
Copy link
Collaborator

rydrman commented Nov 22, 2023

from the meeting today:

  • naming, maybe something more like host_compatilibity to go along with the existing host options language and not get confused with other concepts
  • ideally when we extend host options via a subcommand, we would also want to be able to potentially extend which options appear in these groups based on studio/context. (or maybe that's a bad idea, but worth considering)
  • Any feels more in line with compiler and other packaging tools like pip/gcc
pkg: ...
compat: ...
host_compat: Any | Os | Arch | Distro # here?
build:
  host_compat: Any | Os | Arch | Distro # here or maybe at the root
  options:
    - host: Any # is this too involved?
    - var: os
  variant:
    # variants should be allowed to override vars, so that we can support
    # cross-compilation scenarios
    - {os: }

@dcookspi dcookspi linked a pull request Nov 25, 2023 that will close this issue
1 task
@dcookspi dcookspi linked a pull request Dec 7, 2023 that will close this issue
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request package recipe Packaging recipes QoL quality of life fixes SPI AOI Area of interest for SPI
Projects
None yet
3 participants