Skip to content

Conversation

@elsterdev
Copy link

Hi, thank you very much for your work! I am new to flatpak and this repo helped me get familiar with flatpak, as well install packet tracer on my devices. With this pull request I wanted to draw attention to some things I did, I don't expect this to get merged. Feel free to point out individual commits or changes, if you want those merged I can open separate MRs.

I restricted the sandbox permissions, mainly for two reasons:

  • without network access, packet tracer drops the login screen at startup
  • I am paranoid and don't entirely understand what I'm doing x)

The readme still mentions release 8.2.1, do we want to keep the readme up to date, only mention major versions, or remove that part of the sentence? I can write a commit for that too.

Copy link
Owner

@losuler losuler left a comment

Choose a reason for hiding this comment

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

Thanks for all your contributions, they're very much appreciated. Funny to hear it helped you get familiar with Flatpak, because it did the same for me originally too, haha. Sorry I took so long to look at this (working on improving on that!).

I felt like having the exact version in the README to make it easy to tell if this Flatpak was up to date. I just accidentally forgot to update it last time the .deb got updated. Thanks for reminding me! I'll update that now.

3. Install Flatpak runtime dependencies.

```bash
flatpak-builder --install-deps-from=flathub --install-deps-only build com.cisco.PacketTracer.yml
Copy link
Owner

Choose a reason for hiding this comment

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

This is neat, I didn't realise you could do this! It's probably better if you just remove this step and move this command to replace the existing flatpak-builder build command in step 5.

# - --socket=pulseaudio
- --device=dri
- --filesystem=xdg-download
# - --filesystem=xdg-download
Copy link
Owner

Choose a reason for hiding this comment

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

The reason I added was so that I had somewhere I could save the session file. Most Flatpak's are meant to be usable out of the box and if you want it more restricted, I think the best way to do this is to have the user do so from Flatseal before they launch the Flatpak.

# - --share=network
- --socket=x11
- --socket=pulseaudio
# - --socket=pulseaudio
Copy link
Owner

Choose a reason for hiding this comment

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

I don't remember there being any audio output from Packet Tracer lol, so it's fine to remove this.

- --share=ipc
- --share=network
# - --share=ipc
# - --share=network
Copy link
Owner

Choose a reason for hiding this comment

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

I think we should keep this otherwise I don't believe it's possible to login with your Cisco account.

finish-args:
- --share=ipc
- --share=network
# - --share=ipc
Copy link
Owner

Choose a reason for hiding this comment

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

I think removing this could potentially negatively impact performance1.

Footnotes

  1. https://docs.flatpak.org/en/latest/sandbox-permissions.html#f1

# - --filesystem=xdg-download
- --persist=.
- --env=TZ=
- --nofilesystem=xdg-config/kdeglobals
Copy link
Owner

Choose a reason for hiding this comment

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

Fine as long as you can confirm this doesn't break anything on KDE1.

Footnotes

  1. https://gitlab.gnome.org/GNOME/gnome-software/-/merge_requests/1786

- --persist=.
- --env=TZ=
- --nofilesystem=xdg-config/kdeglobals
- --no-talk-name=com.canonical.AppMenu.Registrar
Copy link
Owner

Choose a reason for hiding this comment

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

Fine as long as you can confirm this doesn't remove this functionality.

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