yast / yast-yast2

YaST module yast2
http://en.opensuse.org/Portal:YaST
GNU General Public License v2.0
56 stars 44 forks source link

add arch filter class to unify architectures specification #1280

Closed jreidinger closed 1 year ago

jreidinger commented 1 year ago

Problem

There are multiple places where we need to specify in text form condition to apply only to certain hardware architectures and it is not unified so far.

Related to https://trello.com/c/C3apZtHv/3231-d-installer-support-for-dynamic-configuration

Solution

Unified solution is based on specification of subvolume handling https://github.com/yast/yast-storage-ng/blob/master/src/lib/y2storage/subvol_specification.rb#L119

There are some improvements to it:

Testing

Open Questions

There is one more place which define architecture filtering - in workflows https://github.com/yast/yast-yast2/blob/master/library/control/src/modules/WorkflowManager.rb#L209 Question is if we want to use filter also here? If so, then we need to support all keyword to match everything and of course adapt that code.

coveralls commented 1 year ago

Coverage Status

Coverage increased (+0.06%) to 41.778% when pulling 92bd53d2d839dd9659942989da1ab9d2694ed800 on arch_filter into aa302a71e3fb71c23e80b4cc49799ce98b180f46 on master.

ancorgs commented 1 year ago

It generally looks good. Just two notes:

mvidner commented 1 year ago

The main question is: WHO will write the architecture specifications/filters, and where.

I'm asking because currently we delegate the elementary matchers to the implementation of Yast::Arch. That's fine if ArchFilter is internal. But as soon as we tell release managers or even customers to use it, we will have to list the individual recognized architectures authoritatively, and preferably in one place.

jreidinger commented 1 year ago

@mvidner well, I expect it will be as usual us who will write it, but it is also used in control.xml and also d-installer.yml can be modified by users for having their own modified installer. But for list it is always hard to keep it in sync :( Even now in control.xml documentation we do not write all possible values - https://github.com/yast/yast-installation/blob/master/doc/control-file.md#the-volumes-subsection

mvidner commented 1 year ago

Even now in control.xml documentation we do not write all possible values - https://github.com/yast/yast-installation/blob/master/doc/control-file.md#the-volumes-subsection

OK. Now that I've read that file, I'm actually worried about the meaning of the comma , operator:

Normally, architectures are combined with logical OR, i.e.

<archs>i386,x86_64</archs>

means "if architecture i386 or x86_64". If the current architecture is an architecture that was excluded with "!", that subvolume is not used no matter what other architectures are specified that might also apply.

mvidner commented 1 year ago

OK, let's borrow naming from logic: (see DNF)

IMHO this captures the existing usages of ppc,!board_powernv (where, importantly, board_powernv is a subset of ppc) and i386,x86_64

TODO: does this work with all?

mvidner commented 1 year ago

functions (atoms) as of today https://github.com/yast/yast-yast2/blob/aa302a71e3fb71c23e80b4cc49799ce98b180f46/library/general/src/modules/Arch.rb

yast-bot commented 1 year ago

:heavy_check_mark: Public Jenkins job #393 successfully finished :heavy_check_mark: Created OBS submit request #1039367

yast-bot commented 1 year ago

:heavy_check_mark: Internal Jenkins job #216 successfully finished :heavy_check_mark: Created IBS submit request #285639