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

Switching systray dependency to one without glibc requirements #14197

Merged
merged 23 commits into from
Nov 2, 2023

Conversation

georgekarrv
Copy link
Member

@georgekarrv georgekarrv commented Sep 28, 2023

Checklist for submitter

  • Changes file added for user-visible changes in changes/ or orbit/changes/.
    See Changes files for more information.
  • Manual QA for all new/changed functionality
    • For Orbit and Fleet Desktop changes:
      • 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).

mna
mna previously approved these changes Oct 2, 2023
Copy link
Member

@lucasmrod lucasmrod left a comment

Choose a reason for hiding this comment

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

Let's remove the copying of libraries in the Makefile:

fleet/Makefile

Lines 427 to 447 in d108147

# Build desktop executable for Linux.
#
# Usage:
# FLEET_DESKTOP_VERSION=0.0.1 make desktop-linux
#
# Output: desktop.tar.gz
desktop-linux:
docker build -f Dockerfile-desktop-linux -t desktop-linux-builder .
docker run --rm -v $(shell pwd):/output desktop-linux-builder /bin/bash -c "\
mkdir /output/fleet-desktop && \
go build -o /output/fleet-desktop/fleet-desktop -ldflags "-X=main.version=$(FLEET_DESKTOP_VERSION)" /usr/src/fleet/orbit/cmd/desktop && \
cp /usr/lib/x86_64-linux-gnu/libayatana-appindicator3.so.1 \
/usr/lib/x86_64-linux-gnu/libayatana-ido3-0.4.so.0 \
/usr/lib/x86_64-linux-gnu/libayatana-indicator3.so.7 \
/lib/x86_64-linux-gnu/libm.so.6 \
/usr/lib/x86_64-linux-gnu/libcairo.so.2 \
/usr/lib/x86_64-linux-gnu/libdbusmenu-gtk3.so.4 \
/usr/lib/x86_64-linux-gnu/libdbusmenu-glib.so.4 \
/output/fleet-desktop && cd /output && \
tar czf desktop.tar.gz fleet-desktop && \
rm -r fleet-desktop"

@lucasmrod
Copy link
Member

Have we defined our minimum versions of OSs we support for fleetd (with Fleet Desktop enabled)?

  • macOS
  • Windows
  • Ubuntu
  • Fedora/CentOS?

Asking this because IIRC this change didn't work on CentOS 7, but maybe we are a-ok with this (and this change makes the linux support much more maintainable than current state - e.g. glibc issues).

@zhumo @zayhanlon @noahtalerman

@noahtalerman
Copy link
Member

this change didn't work on CentOS 7

Does this include CentOS 7.1+?

Our docs say we support CentOS 7.1+: https://fleetdm.com/docs/using-fleet/supported-host-operating-systems#supported-host-operating-systems

Not saying this can't or shouldn't change. I'm not sure what version our users/customers are on...

@georgekarrv
Copy link
Member Author

I was still attempting to setup testing on physical devices w/ these changes just to be sure it worked for all 3 before removing the packages and testing again.

Yes I will test on CentOS too and figure that out.

@georgekarrv georgekarrv temporarily deployed to Docker Hub October 16, 2023 15:22 — with GitHub Actions Inactive
@georgekarrv georgekarrv temporarily deployed to Docker Hub October 16, 2023 15:22 — with GitHub Actions Inactive
@codecov
Copy link

codecov bot commented Oct 16, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (3de7a0b) 58.83% compared to head (41a3067) 58.84%.

❗ Current head 41a3067 differs from pull request most recent head ad2a09c. Consider uploading reports for the commit ad2a09c to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #14197   +/-   ##
=======================================
  Coverage   58.83%   58.84%           
=======================================
  Files         954      954           
  Lines       80174    80187   +13     
  Branches     2215     2215           
=======================================
+ Hits        47169    47183   +14     
+ Misses      29331    29329    -2     
- Partials     3674     3675    +1     
Flag Coverage Δ
backend 59.48% <ø> (+<0.01%) ⬆️

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

see 1 file with indirect coverage changes

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

@georgekarrv georgekarrv temporarily deployed to Docker Hub October 26, 2023 20:31 — with GitHub Actions Inactive
@georgekarrv georgekarrv temporarily deployed to Docker Hub October 26, 2023 20:31 — with GitHub Actions Inactive
@Sampfluger88
Copy link
Member

@georgekarrv The handbook pages in this PR are duplicative and already merged.

Check the files changed (55?) is that correct?

image

image

@georgekarrv georgekarrv temporarily deployed to Docker Hub October 27, 2023 18:49 — with GitHub Actions Inactive
@georgekarrv
Copy link
Member Author

@Sampfluger88 fixed

@Sampfluger88 Sampfluger88 removed the request for review from mikermcneil October 27, 2023 18:56
lukeheath
lukeheath previously approved these changes Oct 27, 2023
@georgekarrv georgekarrv marked this pull request as draft October 27, 2023 20:22
@georgekarrv georgekarrv temporarily deployed to Docker Hub November 2, 2023 17:04 — with GitHub Actions Inactive
Copy link
Member

@lucasmrod lucasmrod left a comment

Choose a reason for hiding this comment

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

LGTM!

(I haven't reviewed the Helm chars but they were used only for testing so we are g2g IMO.)

@georgekarrv georgekarrv temporarily deployed to Docker Hub November 2, 2023 18:10 — with GitHub Actions Inactive
@lukeheath
Copy link
Member

Looks good to me. @rfairburn take a look when you have a moment?

Copy link
Contributor

@rfairburn rfairburn left a comment

Choose a reason for hiding this comment

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

I haven't had time to fully test the helm chart, but looking at it everything appears 100% good to me. No concerns here from what I see.

@Sampfluger88 Sampfluger88 removed the request for review from mikermcneil November 2, 2023 19:13
@georgekarrv
Copy link
Member Author

This fixes #11958

@georgekarrv georgekarrv temporarily deployed to Docker Hub November 2, 2023 19:40 — with GitHub Actions Inactive
@georgekarrv georgekarrv merged commit 970854e into main Nov 2, 2023
20 of 23 checks passed
@georgekarrv georgekarrv deleted the gkarr-fix-linux-desktop branch November 2, 2023 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

8 participants