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

refact fix: Flatpak #705

Merged
merged 7 commits into from
Nov 1, 2024
Merged

Conversation

jeevithakannan2
Copy link
Contributor

@jeevithakannan2 jeevithakannan2 commented Sep 30, 2024

Chris read before merging !!

Type of Change

  • New feature
  • Bug fix
  • Documentation update
  • Refactoring
  • Hotfix
  • Security patch
  • UI/UX improvement

Description

  • For the scripts in office suits to work fix: Common script paths #717 must be merged.
  • Moved flatpak handling to common-script due to path conflicts when setup-flatpak is called from different tabs.
  • Added apk package manager in case feat: Alpine linux support  #814 gets merged. If not remove apk package manager before merging
  • This PR marks as a first step if flatpak was decided to be used as a secondary package manager in linutil
  • The following error was reproduced by removing the pacman installation section, forcing the script to use flatpak for installation:

Note

This issue was originally identified on another laptop running openSUSE. Since no screenshots were captured in openSUSE, the workaround was applied to reproduce the error.

flatpak-error

Testing

  • Tested on arch and openSUSE, works as expected.

Additional Description

  • This implementation is basic and can be improved in future.
  • There is a lot of room for improvement here which can be implemented later in the future.

The idea mentioned in #705 (comment) was not implemented because of the following error

image

It occurs because setup-flatpak is also sourcing common-script and the root sourcing happens in the communication dir (zoom). It will try to access common-script in the application setup folder instead of trying to source the common-script from the root directory

Checklist

  • My code adheres to the coding and style guidelines of the project.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no errors/warnings/merge conflicts.

@nnyyxxxx

This comment was marked as resolved.

@nnyyxxxx

This comment was marked as resolved.

nnyyxxxx

This comment was marked as resolved.

Copy link
Contributor

@nnyyxxxx nnyyxxxx left a comment

Choose a reason for hiding this comment

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

if any other scripts use flatpak be sure to include these changes

@nnyyxxxx
Copy link
Contributor

DONT merge this yet, needs more discussing, this implementation is a bit dirty and i feel like we can just combine the flatpak command_exists with the reg one

@jeevithakannan2 jeevithakannan2 marked this pull request as draft September 30, 2024 14:03
@jeevithakannan2 jeevithakannan2 marked this pull request as ready for review September 30, 2024 16:31
@jeevithakannan2 jeevithakannan2 marked this pull request as ready for review October 9, 2024 14:39
@jeevithakannan2
Copy link
Contributor Author

jeevithakannan2 commented Oct 9, 2024

DONT merge this yet, needs more discussing, this implementation is a bit dirty and i feel like we can just combine the flatpak command_exists with the reg one

Combined both flatpak command_exists ready to be merged !!

Copy link
Collaborator

@adamperkowski adamperkowski 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 wait with this until other PRs that may conflict are resolved.

@ChrisTitusTech ChrisTitusTech merged commit d4eacae into ChrisTitusTech:main Nov 1, 2024
3 of 4 checks passed
@jeevithakannan2 jeevithakannan2 deleted the flatpak branch November 1, 2024 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants