Linutronix / elbe

Embedded Linux Build Environment
https://elbe-rfs.org
GNU General Public License v3.0
163 stars 59 forks source link

preferences: flexible pkg pinning options #401

Closed hexahurz closed 3 months ago

hexahurz commented 4 months ago

This enables the use of apt package pinning options other than the codename. For backwards compatibility, the default option used is still the codename (n=).

Signed-off-by: Franz Heger franz.heger@kuka.com Reviewed-by: Daniel Braunwarth daniel.braunwarth@kuka.com

t-8ch commented 4 months ago

This change leaks the implementation details of the pinning mechanism through to the XML description.

What is your specific usecase?

Maybe we could recognize the Archive names stable, oldstable, etc.. and emit a= stanzas instead for them.

hexahurz commented 4 months ago

Thank you for your feedback!

Our use case would be the use of a (proxied) debian repositories mixed with company-internal debian repositories, with the codename being passed programmatically. To avoid hacky workarounds and due to process requirements, we need to ensure that some specific packages are always taken from the same origin.

This is achieved by specifying the origin (o=, not to be confused with the origin: setting) or combined filtering options (o=... + n=...) for those packages in deviation of the default preferences.

See e.g.:

    <pkg pin="o=KUKA">swupdate</pkg>
    <pkg pin="o=KUKA">lua-swupdate</pkg>
    <pkg pin="o=KUKA">swupdate-www</pkg>
    <pkg pin="a=stable-backports">libubootenv-tool</pkg>
    <pkg pin="a=stable-backports">libubootenv0.1</pkg>
    <pkg pin="a=stable-backports">libzck1</pkg>
t-8ch commented 4 months ago

Thanks for the examples.

For o=KUKA you should already be able to do something like:

<mirror><url-list>
<binary pin="1000" package="swupdate lua-swupdate">$YOUR_REPO_URL</binary>
</url-list></mirror>

This would pin origin against the hostname of the repo URL.

As for stable-backports:

Does it really make sense to pin a backport against a relative archive name? As soon as the image switches to a new stable release whatever was in the backport is now part of the non-backport package.

hexahurz commented 4 months ago

There might be workarounds to this, but we want to be able to specify this on a package level, so that these settings can be put en-bloc into a file context which is then included:

<!-- elbe.xml -->
<xi:include href="./../common/swupdate/pkg-list_swupdate.xml"/>
<!-- pkg-list_swupdate.xml -->
<?xml version="1.0" encoding="UTF-8"?>
<pkg-list>
    <pkg pin="o=KUKA">swupdate</pkg>
    <pkg pin="o=KUKA">lua-swupdate</pkg>
    <pkg pin="o=KUKA">swupdate-www</pkg>
    <pkg pin="a=bookworm-backports">libubootenv-tool</pkg>
    <pkg pin="a=bookworm-backports">libubootenv0.1</pkg>
    <pkg pin="a=bookworm-backports">libzck1</pkg>
</pkg-list>

We'd like to keep this information bundled in one file, not scattered across multiple files.

t-8ch commented 4 months ago

into a file context which is then included

Makes sense.

For the backport pin, are you fine with using the existing pin?

<pkg pin="bookworm-backports">libubootenv-tool</pkg>
d4nuu8 commented 4 months ago

Maybe we could recognize the Archive names stable, oldstable, etc.. and emit a= stanzas instead for them

<pkg pin="bookworm-backports">libubootenv-tool</pkg>

Both statements are conflicting I guess.

I don't think it's a good idea to try to be smart here. I think we can keep backwards-compatibility with this patch and enable advanced use cases at the same time.

Maybe we can extend the documentation and explain both use cases in detail. With a link to man 5 apt_preferences everybody should be able to handle this.

t-8ch commented 4 months ago

Both statements are conflicting I guess.

Yes, the original heuristic a suggested is not good.

I'll see where this can be added to the docs. For backports however I don't see how stable-backports would ever be useful.

But as said, leaking the apt_preferences syntax into the xml attributes is not good. Either would need a new XML attribute or you could do raw apt_preferences via <raw-preference>.

hexahurz commented 4 months ago

Don't get too hung up on the stable-backports part, I only included that for completeness, and it can be easily changed e.g., to "bookworm-backports".

The main aspect here is the use of o=KUKA on package level, independent of codename and global preferences.

d4nuu8 commented 4 months ago

For backports however I don't see how stable-backports would ever be useful.

Ignore this stable-backports we used previously.

Using <raw-preference> conflicts with the use-case:

There might be workarounds to this, but we want to be able to specify this on a package level, so that these settings can be put en-bloc into a file context which is then included:

To be honest I don't understand why this is "leaking" apt_preferences syntaxt. I mean one want to pin something and therefore needs to know what is possible, right? To infantize the user here seems to be wrong to me.

Adding an xml node for each of the possibilities apt_preferences provides is maybe the wrong direction.

t-8ch commented 4 months ago

it can be easily changed e.g., to "bookworm-backports".

Good, I only wanted to make sure to have all your requirements covered.

Using conflicts with the use-case:

You can put multiple different parts into a single XML file and xinclude each part into a different location with xpointer. This is something that can be used already today. See tests/preproc-02.xml.

To be honest I don't understand why this is "leaking" apt_preferences syntaxt. To infantize the user here seems to be wrong to me.

This is not at all about infantizing the users. After all <raw-preferences> is also provided.

It is about having a clear semantic which can be evolved and does not limit us to the current implementation.

Adding an xml node for each of the possibilities apt_preferences provides is maybe the wrong direction.

Could you elaborate?

The proposed solution while more flexible is still is missing functionality: Pin: origin, Pin: version. When the next user needs this, it would be nice not having to add yet another undocumented escape-hatch heuristic.

I'll think about this some more, maybe I can come up with a nicer and easier solution.

hexahurz commented 4 months ago

Pin: origin and Pin: version are handled differently in the current implementation from Pin: release, and maybe not in a good way, see e.g.,:

<pkg pin="codename" version="2022.12*">swupdate</pkg>

leads to

Package: swupdate
Pin: release n=codename
Pin-Priority: 991

Package: swupdate
Pin: version 2022.12*
Pin-Priority: 1001

which I doubt is the intended outcome.

So, if you'd like to be able to use a combination of these, I'd suggest ensuring that the attributes can be mixed properly, but this is not the objective of this pull request.

t-8ch commented 3 months ago

which I doubt is the intended outcome.

That looks indeed wrong, thanks for the report.

So it seems the whole pinning story needs some sort of overhaul.

For now I'm focusing on improving the testing aspect of elbe, so all future changes will be more robust.

Could you try the approach I outlined above using xinclude/xpointer which seems to fulfill our desired goal of modularization?

hexahurz commented 3 months ago

I played around a bit with xinclude/xpointer. It makes some of our inclusions easier/cleaner, but as far as I see, it doesn't facilitate pinning on a package level.

So the use case remains unsatisfied.

t-8ch commented 3 months ago

it doesn't facilitate pinning on a package level.

Did you try <raw-preference>? It is the documented escape-hatch to insert text verbatim into /etc/apt/preferences.

hexahurz commented 3 months ago

Just checked it again today. Working example

<!-- swupdate.xml -->
   <project>
        <raw-preference>
            Package: swupdate lua-swupdate swupdate-www
            Pin: release o=KUKA
            Pin-Priority: 1200
        </raw-preference>
    </project>
    <target>
        <pkg-list>
            <pkg>swupdate</pkg>
            <pkg>lua-swupdate</pkg>
            <pkg>swupdate-www</pkg>
            <pkg pin="bookworm-backports">libubootenv-tool</pkg>
            <pkg pin="bookworm-backports">libubootenv0.1</pkg>
            <pkg pin="bookworm-backports">libzck1</pkg>
        </pkg-list>
    </target>

and

...
    <project>
        ...

        <raw-preference>
            Package: package1 package2
            Pin: release o=KUKA
            Pin-Priority: 1200
        </raw-preference>
        <xi:include xpointer="xpointer(*/project/raw-preference)" href="./swupdate.xml"/>
    </project>
    ...
    <target>
        <pkg-list>
            <pkg>package1</pkg>
            <pkg>package2</pkg>
            <pkg>package3</pkg>
            ...
            <xi:include xpointer="xpointer(*/target/pkg-list/*)" href="./swupdate.xml"/>
        </pkg-list>
    </target>

While this "works", it is too cumbersome, and error handling for faulty xpointer statements feels more like trial-and-error. And once again, it shifts the scope away from the package, requiring a global consistency instead of a package scope, so I would expect errors to pop up in future maintenance when packages are added/removed.

Overall it feels like a hard-to-read workaround, while the approach proposed in this pull request would make this functionality way more straightforward, easier to read and use.

t-8ch commented 3 months ago

While this "works", it is too cumbersome

I agree (also to your other points)

would make this functionality way more straightforward

Here I still disagree. Can we find a solution that does not mix apt_preferences syntax with XML syntax? (<raw-preferences> does this mix but it's an escape hatch and it is explicitly named "raw")

We talked about this internally and the plan is to have new attributes ob <pkg>. origin, release-name (alias for the current pin), release-archive, release-version, release-origin and version. In addition the schema should forbid invalid combinations. (Like the current pin/version combination you reported, for which I'll send a patch shortly)

Is this something you want to work on? If not I can work on it in the near future.

hexahurz commented 3 months ago

Sounds good, I'll try to draft something up. (Saw the patch, looking good)

t-8ch commented 3 months ago

I'll try to draft something up

Thanks!

hexahurz commented 3 months ago

Updated with a new proposal.

hexahurz commented 3 months ago

This would also remove the need for your mutual-exclusion patch.

t-8ch commented 3 months ago

This would also remove the need for your mutual-exclusion patch.

That is nice. It's a bit weird because apt_preferences(5) does not mention the possibility of having multiple Pin: entries per record at all. I want to investigate a bit if it is intentional that it works and maybe get it documented.

hexahurz commented 3 months ago

That is nice. It's a bit weird because apt_preferences(5) does not mention the possibility of having multiple Pin: entries per record at all.

Yes, I wondered the same thing, but it also does not mention to not use it :D.

t-8ch commented 3 months ago

Yes, I wondered the same thing, but it also does not mention to not use it :D.

There is https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=868276 which is our exact usecase. It mentions that only the last Pin: entry is used. This is what I can reproduce on bookworm:

$ cat /etc/apt/preferences
Package: curl
Pin: version invalid
Pin: release o=Debian
Pin-Priority: 1001

# the pin matches
$ apt-cache policy curl
curl:
  Installed: (none)
  Candidate: 7.88.1-10+deb12u5
  Version table:
     7.88.1-10+deb12u5 1001
        500 http://deb.debian.org/debian stable/main amd64 Packages
        500 http://deb.debian.org/debian-security stable-security/main amd64 Packages

# swap Pin: lines
$ cat /etc/apt/preferences
Package: curl
Pin: release o=Debian
Pin: version invalid
Pin-Priority: 1001

# the pin does not match
$ apt-cache policy curl
curl:
  Installed: (none)
  Candidate: 7.88.1-10+deb12u5
  Version table:
     7.88.1-10+deb12u5 500
        500 http://deb.debian.org/debian stable/main amd64 Packages
        500 http://deb.debian.org/debian-security stable-security/main amd64 Packages

Another comment reports that joining entries with commas would work correctly but for me that also only evaluates the first entry.

hexahurz commented 3 months ago

It mentions that only the last Pin: entry is used. This is what I can reproduce on bookworm:

Thank you for the investigation. What would be the takeaway? Provide an xsd rule that ensures only either of version, release-* and origin is used?

t-8ch commented 3 months ago

What would be the takeaway? Provide an xsd rule...

I fear such an exclusion will be necessary. It would have been great to handle this like you proposed, but alas... Can you add a comment to the exclusion to point to the upstream bug report and our discussion?

hexahurz commented 3 months ago

Overhauled, and included your suggestions!

t-8ch commented 3 months ago

Thanks! It it intentional that the validation is in code and not in the XSD? The XSD would give people using XML editors instant feedback.

Also the git history of the PR should be cleaned up.

hexahurz commented 3 months ago

If you have a working solution in XSD, that would be welcome. My XSD skills seemed to be too limited to come to a fruitful end.

Will clean up the history...

t-8ch commented 3 months ago

If you have a working solution in XSD, that would be welcome.

Doing it in XSD 1.0, which we are limited to with lxml seems to be a big pain, so let's keep your logic.

With XSD 1.1 it would be nicer but for that we would need to move to xmlschema or a similar library. That should be useful in general as it would get rid of binary dependencies. But that's completely out of scope for this PR.

t-8ch commented 3 months ago

Thanks! I'll run this through CI and pick it up when that succeeds.

t-8ch commented 3 months ago

Committed as 99e9c4b43454d61d9d56cc1234666dc0a1f27742 Thanks for your work!