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

Add Fleet Desktop support for Wayland display sessions #25998

Merged
merged 9 commits into from
Feb 5, 2025

Conversation

lucasmrod
Copy link
Member

@lucasmrod lucasmrod commented Feb 3, 2025

For #19043.

See the versions and distributions tested during development on the QA notes of #19043.


  • Changes file added for user-visible changes in changes/, orbit/changes/ or ee/fleetd-chrome/changes.
    See Changes files for more information.
  • A detailed QA plan exists on the associated ticket (if it isn't there, work with the product group's QA engineer to add it)
  • Manual QA for all new/changed functionality
  • For Orbit and Fleet Desktop changes:
    • Orbit runs on macOS, Linux and Windows. Check if the orbit feature/bugfix should only apply to one platform (runtime.GOOS).
    • Manual QA must be performed in the three main OSs, macOS, Windows and Linux.
    • Auto-update manual QA, from released version of component to new version (see tools/tuf/test).

Copy link

codecov bot commented Feb 3, 2025

Codecov Report

Attention: Patch coverage is 0.74627% with 133 lines in your changes missing coverage. Please review.

Project coverage is 63.59%. Comparing base (2b9e19f) to head (9b70063).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
orbit/pkg/execuser/execuser_linux.go 1.11% 89 Missing ⚠️
pkg/open/open_linux.go 0.00% 44 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #25998      +/-   ##
==========================================
- Coverage   63.64%   63.59%   -0.05%     
==========================================
  Files        1631     1631              
  Lines      156293   156402     +109     
  Branches     4088     4088              
==========================================
- Hits        99470    99467       -3     
- Misses      48985    49096     +111     
- Partials     7838     7839       +1     
Flag Coverage Δ
backend 64.43% <0.74%> (-0.06%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

sgress454
sgress454 previously approved these changes Feb 5, 2025
Copy link
Contributor

@sgress454 sgress454 left a comment

Choose a reason for hiding this comment

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

Have not tested e2e but code LGTM, couple of comments around getting sockets. I did run some of the various commands (like loginctl show-user and loginctl show-session) with my Ubuntu VM set to use Wayland and got the results expected by this code.


// getUserWaylandDisplay returns the value to set on WAYLAND_DISPLAY for the given user.
func getUserWaylandDisplay(uid string) (string, error) {
matches, err := filepath.Glob("/run/user/" + uid + "/wayland-*")
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we can't use the value of $XDG_RUNTIME_DIR rather than hardcoding /run/user because we're running this as root? Probably not an issue on the majority of systems, but if there was an easy way to get the value of $XDG_RUNTIME_DIR for the user that'd be 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree.

Something to improve as we go AFAICS (given it seems to be an edge case and we will see it on the logs if something like that happens on some system).

Comment on lines +335 to +338
if strings.HasSuffix(match, ".lock") {
continue
}
return filepath.Base(match), nil
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems slightly brittle, in the edge case where anything else besides sockets and lockfiles ends up in here, so we could take the extra step of looking at the file stats to make sure it's a socket, but again probably an edge case.

@lucasmrod lucasmrod merged commit c81117b into main Feb 5, 2025
46 checks passed
@lucasmrod lucasmrod deleted the 19043-fleet-desktop-support-wayland branch February 5, 2025 17:00
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.

2 participants