many: add cuda-driver-libs interface #15063
Conversation
|
Wed Feb 12 20:19:24 UTC 2025 Failures:Executing:
Restoring:
|
alfonsosanchezbeato
force-pushed
the
add-cuda-driver-libs
branch
from
d09bff4 to
fd9d5c0
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #15063 +/- ##
=========================================
Coverage ? 77.98%
=========================================
Files ? 1170
Lines ? 157034
Branches ? 0
=========================================
Hits ? 122467
Misses ? 26963
Partials ? 7604
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Grow test for interfaces that export libraries to the rootfs, like cuda-driver-libs.
Make sure that Setup touches the ldconfig configuration only for the system snap, as it is the owner of it. Otherwise the configuration would be destroyed when the other snaps are configured.
alfonsosanchezbeato
force-pushed
the
add-cuda-driver-libs
branch
from
fd9d5c0 to
540bc1d
Compare
| if len(fields) != 1 && len(fields) != 3 { | ||
| return fmt.Errorf("wrong format for api-version: %q", versRange) | ||
| } | ||
| switch len(fields) { |
There was a problem hiding this comment.
A comment here with examples of the expected input might be good. I think something like this?
// valid input examples:
// 1.2.3
// 1.2.3 .. 2.3.4
There was a problem hiding this comment.
Good point, I've added a comment
| // For the moment only the system snap is supported - the | ||
| // snap.system.conf file is owned by it and the set-up of other snaps | ||
| // must not affect it. | ||
| if !interfaces.IsTheSystemSnap(appSet.InstanceName()) { |
There was a problem hiding this comment.
we discussed before that this doesn't make a lot of sense because of the two of the interface, why is it needed?
There was a problem hiding this comment.
It is actually needed, otherwise when the slot side builds its profiles after, say, a disconnection, it overrides the content of snap.system.conf. That file is owned by the system plug and no other plug/slot should touch it.
For instance, this is seen when there are 2 snaps providing slots, if one is removed, when reconstructing the slot profiles it sees no spec.libDirs and it removes the file, overriding what the plug profile wrote just before.
There was a problem hiding this comment.
let's talk about this in our 1-1, I'm really worried that these are the only interfaces that need this kind of behavior and they assume things about two levels up in the code from them
| } | ||
| // Validate directories | ||
| for _, dir := range libDirs { | ||
| if !strings.HasPrefix(dir, "$SNAP") && !strings.HasPrefix(dir, "${SNAP}") { |
There was a problem hiding this comment.
shouldn't this also include the / ?
There was a problem hiding this comment.
Good point, changed
|
@alfonsosanchezbeato maybe it would make sense to make the changes to support implicit plugs their own PR? |
There was a problem hiding this comment.
@pedronis thanks for the review, comments addressed. Regarding configfile, no, for th cuda interface we do not have configuration files that we need to take care of.
| // For the moment only the system snap is supported - the | ||
| // snap.system.conf file is owned by it and the set-up of other snaps | ||
| // must not affect it. | ||
| if !interfaces.IsTheSystemSnap(appSet.InstanceName()) { |
There was a problem hiding this comment.
It is actually needed, otherwise when the slot side builds its profiles after, say, a disconnection, it overrides the content of snap.system.conf. That file is owned by the system plug and no other plug/slot should touch it.
For instance, this is seen when there are 2 snaps providing slots, if one is removed, when reconstructing the slot profiles it sees no spec.libDirs and it removes the file, overriding what the plug profile wrote just before.
| } | ||
| // Validate directories | ||
| for _, dir := range libDirs { | ||
| if !strings.HasPrefix(dir, "$SNAP") && !strings.HasPrefix(dir, "${SNAP}") { |
There was a problem hiding this comment.
Good point, changed
Add cuda-driver-libs interface. This is the first interface using the ldconfig backend. Additionally, it is the first example of an implicit plug, so some changes in ifacestate are also part of this change.