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

extensions: add 'basic-graphics' extension #4288

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mboudr35
Copy link

@mboudr35 mboudr35 commented Jul 21, 2023

This extension provides a shortcut for snaps that make use of the 'mesa-core22' snap
of graphics libraries & resources. It configures the conventional 'graphics-core22'
content plug and provides the 'graphics-launch' helper script for setting-up
the environment.

Those that make frequent use of the 'mesa-core22' snap when building snaps intended for
execution on Ubuntu Core 22 (or eventually later) running ubuntu-frame may especially
appreciate this extension, as this automates much of the boilerplate put in the snapcraft.yaml.

  • Have you followed the guidelines for contributing?
  • Have you signed the CLA?
  • Have you successfully run make lint?
  • Have you successfully run pytest tests/unit?

@mboudr35 mboudr35 force-pushed the graphics-plug branch 2 times, most recently from 690cda4 to 568b634 Compare July 21, 2023 09:25
Copy link
Contributor

@Saviq Saviq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, so as discussed my initial feedback on this:

I don't think we should have setup.sh here — it's our shortcut to not having to wait for store declarations, but it shouldn't proliferate in a Snapcraft extension.

There's no IoT specificity - we'd like for any graphical snap to ultimately move to this approach - rather than shipping Mesa et al. Neither is Wayland the only target, so I wonder if we should go for a more optimistic approach, where a Wayland setup is prepared, but not a requirement.

Here's a simpler version of wayland-launch that does this: https://github.com/MirServer/graphics-test-tools/blob/main/scripts/bin/wayland-launch. It should probably be renamed graphics-launch, too - or even integrated into graphics-core22-wrapper. I would probably import the wrapper and cleanup scripts directly here, rather than dump it in build-time. Snapcraft knows how to clean up what's in the base snap, maybe it's possible to do the same for mesa-coreXX?

extensions/graphics/wayland/bin/wayland-launch Outdated Show resolved Hide resolved
extensions/graphics/wayland/bin/wayland-launch Outdated Show resolved Hide resolved
Comment on lines 31 to 41
"""An extension that provides graphics libraries for snaps that don't need to integrate with a full desktop environment.

This extension is intended to be used with Wayland-ready application snaps that will execute on an
Ubuntu Core 22 (and eventually later) systems with ubuntu-frame,
however it is not exclusively fixated on Ubuntu Core systems - Desktop should work fine
(though you may want to consider one of the gnome/kde extensions instead).

This extension defines a single plug "graphics-core{base version number}" which is a content
interface to the "mesa-core{base version number}" snap that provides the graphics libraries and
other important resources. It also configures apps to use the "opengl" and "wayland" plugs,
which are essential for graphical output.

To assist in the execution of Wayland applications, this extension provides a helper
"wayland-launch" script that can be added to the app's command-chain.
"""
Copy link
Contributor

@Saviq Saviq Jul 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will need a rewrite if my suggestions about making this more generic go through.

Ultimately we'd like the GNOME / KDE extensions replicate this extension's behaviour, or maybe wrap it, if that's possible.

snapcraft/extensions/graphics.py Outdated Show resolved Hide resolved
@tonyespy
Copy link

tonyespy commented Aug 1, 2023

There's no IoT specificity - we'd like for any graphical snap to ultimately move to this approach - rather than shipping Mesa et al. Neither is Wayland the only target, so I wonder if we should go for a more optimistic approach, where a Wayland setup is prepared, but not a requirement.

I'd really hate to see us add this using the name graphics though. We're in a situation right now where Core customers are confused about whether or not to use the gnome extension. Thankfully we're all aligned internally that this approach is not best for people building Core-based graphical IoT snaps. If we think that this extension is useful for people building non IoT graphical snaps too, then lets make sure the name better differentiates it from the existing gnome extension (e.g. gnomeless-graphics?).

@mboudr35
Copy link
Author

mboudr35 commented Aug 1, 2023

[...] lets make sure the name better differentiates it from the existing gnome extension (e.g. gnomeless-graphics?).

basic-graphics, primitive-graphics, mesa-graphics, mesa

Perhaps one of these could be a better fit?

@Saviq
Copy link
Contributor

Saviq commented Aug 2, 2023

[...] lets make sure the name better differentiates it from the existing gnome extension (e.g. gnomeless-graphics?).

basic-graphics, primitive-graphics, mesa-graphics, mesa

plain-graphics, gl, accelerated-graphics, gpu?

mesa shouldn't be a part of it since it's just one implementation.

@tonyespy
Copy link

tonyespy commented Aug 2, 2023

What about graphics-minimal (or graphics-min) and we push for re-naming the current gnome extension to graphics-gnome in the next version of snapcraft?

@Saviq
Copy link
Contributor

Saviq commented Aug 2, 2023

What about graphics-minimal (or graphics-min) and we push for re-naming the current gnome extension to graphics-gnome in the next version of snapcraft?

That would suggest there can be a non-graphics GNOME?

@tonyespy
Copy link

tonyespy commented Aug 2, 2023

That would suggest there can be a non-graphics GNOME?

Good point. Although that only applies to the last suggestion in my comment.

@mboudr35
Copy link
Author

@Saviq @tonyespy I've pushed some changes following previous discussion, please give a look when able!

@mboudr35 mboudr35 changed the title extensions: add 'graphics' extension extensions: add 'basic-graphics' extension Aug 22, 2023
@mboudr35 mboudr35 force-pushed the graphics-plug branch 3 times, most recently from 2ccd924 to 27222c5 Compare August 23, 2023 15:45
@codecov-commenter
Copy link

codecov-commenter commented Aug 23, 2023

Codecov Report

Merging #4288 (087449f) into main (c6f41da) will increase coverage by 0.02%.
The diff coverage is 100.00%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##             main    #4288      +/-   ##
==========================================
+ Coverage   89.18%   89.20%   +0.02%     
==========================================
  Files         321      322       +1     
  Lines       21614    21650      +36     
==========================================
+ Hits        19276    19313      +37     
+ Misses       2338     2337       -1     
Files Coverage Δ
snapcraft/extensions/basic_graphics.py 100.00% <100.00%> (ø)
snapcraft/extensions/registry.py 92.00% <100.00%> (+0.33%) ⬆️

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@Saviq Saviq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to see Snapcraft's feedback on this, as I feel this pushes things here and there :)

Some remarks inline.

extensions/basic-graphics/bin/graphics-launch Outdated Show resolved Hide resolved
snapcraft/extensions/basic_graphics.py Outdated Show resolved Hide resolved
snapcraft/extensions/basic_graphics.py Outdated Show resolved Hide resolved
snapcraft/extensions/basic_graphics.py Outdated Show resolved Hide resolved
snapcraft/extensions/basic_graphics.py Outdated Show resolved Hide resolved
snapcraft/extensions/basic_graphics.py Outdated Show resolved Hide resolved
Comment on lines +131 to +110
# This should be the last part to prime, so the clean-up can execute properly
"after": list(self.yaml_data["parts"].keys()),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wonder how extension ordering affects this… Maybe just naming the part zzz_ would be safer xD

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That may be the case, though as I understand the naming convention for extension-provided parts the extname/ prefix is required. I've no idea if applying an ordering prefix to the portion after the / would be applied in the same way

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW I mean that there might be an extension processed after yours, in which case the after: will miss any parts introduced there.

It may be needed to document that this extension should be last, if that actually matters.

@mboudr35 mboudr35 force-pushed the graphics-plug branch 2 times, most recently from 537fc86 to 14b36de Compare September 7, 2023 18:38
@cmatsuoka
Copy link
Contributor

Thanks! I think it would be interesting for us to play a bit with it and explore some scenarios, do you have a test project I could use? Also it's important to have tests for this extension before landing, including a unit test in tests/unit/extensions and a package building test in tests/spread/extensions.

Copy link
Collaborator

@mr-cal mr-cal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a useful extension but I'll echo Claudio's comment - a spread test and test project to experiment with would be helpful.

@mboudr35 mboudr35 force-pushed the graphics-plug branch 2 times, most recently from e4af0f5 to 51be364 Compare October 20, 2023 17:54
This extension provides a shortcut for snaps that make use of the 'graphics-core{ver}' interface,
such as the 'mesa-core{ver}' and similar snaps, for consuming graphics libraries & resources.
It configures the conventional 'graphics-core22' content plug and provides the
'graphics-launch' helper script for setting-up the environment.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants