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 Sanctify #540

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

SubhadeepJasu
Copy link
Contributor

@SubhadeepJasu SubhadeepJasu commented Nov 23, 2024

Screenshot from 2024-11-23 21 40 49

Review Checklist

  • App opens
  • Does what it says
  • Categories match

AppData

  • Name is unique and non-confusing
  • Matches description
  • Matches screenshot
  • Launchable tag with matching ID
  • Release tag with matching version and YYYY-MM-DD date
  • OARS info matches

Flatpak

  • Uses elementary runtime
  • Sandbox permissions are reasonable

@SubhadeepJasu SubhadeepJasu requested a review from a team as a code owner November 23, 2024 17:47
@zeebok
Copy link
Contributor

zeebok commented Nov 30, 2024

Does this work on the final release of OS8 with pipewire as the default audio server? Not sure if that forces any change to the flatpak permissions necessary

@zeebok
Copy link
Contributor

zeebok commented Nov 30, 2024

In your appdata, in the name tag, you misspelled Sanctify as Santify (though that fits the festive time of year here in the US :P)

Since your oars rating is all none I believe if you'd like you can simply use <content_rating type="oars-1.1" /> there

@zeebok
Copy link
Contributor

zeebok commented Nov 30, 2024

You should update the flatpak yml to grab 1.0 instead of 0.9.9!

@SubhadeepJasu
Copy link
Contributor Author

I have been using pipewire for sometime now. I don't have os8 yet but it might work.

Will fix the typo.

I can't set the release to the current one because of how the pipeline works. The flatpak pipeline uses build artifacts from Godot pipeline uploaded in a prior release. So for flatpak pipeline to work there has to be a release containing the files. Essentially here 0.9.9 is the same as 1.0.0

@zeebok
Copy link
Contributor

zeebok commented Nov 30, 2024

I have been using pipewire for sometime now. I don't have os8 yet but it might work.

Okay, mostly just curious if the portals work properly with pipewire. If it works on OS7 it probably works on OS8 as well. And now that I think about it I did swap over to pipewire myself on OS7 and that is what I am testing with lol

Will fix the typo.

Woo!

I can't set the release to the current one because of how the pipeline works. The flatpak pipeline uses build artifacts from Godot pipeline uploaded in a prior release. So for flatpak pipeline to work there has to be a release containing the files. Essentially here 0.9.9 is the same as 1.0.0

I see. Okay! I kind of want to poke at things with flatpak to see if there is something a little simpler you can update the manifest to.

@SubhadeepJasu
Copy link
Contributor Author

SubhadeepJasu commented Nov 30, 2024

Fixed the typo and the oars stuff.

zeebok
zeebok previously approved these changes Dec 1, 2024
@zeebok
Copy link
Contributor

zeebok commented Dec 1, 2024

@danirabbit Do we want to worry about passing ARM builds with games, or bypass that requirement?

@SubhadeepJasu
Copy link
Contributor Author

Dani did mention on Discord that arm is still experimental, it's good to have and not an absolute requirement.

Also currently I dont have the pipeline setup to build arm and I have no way of testing arm builds.

Copy link
Member

@danirabbit danirabbit left a comment

Choose a reason for hiding this comment

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

You should not have - '--system-talk-name=org.freedesktop.Accounts' in your flatpak manifest. This is not required for system wide dark mode. Your app should use the portal for this

Is there a reason you have - '--filesystem=home'? That's a pretty broad permission

I'm also really really hesitant to allow a pre-built binary here. We've always required things to be built on our server to make sure the source code you present and we've reviewed is actually what's in the package. This would break our promise of AppCenter apps being auditable

@SubhadeepJasu
Copy link
Contributor Author

SubhadeepJasu commented Dec 2, 2024

At the moment it's not known how to access portals from the Godot Engine, without upstream support for it. So, that system talk is only thing the engine is recognizing for it's built-in dark mode support.

Building the game in the flatpak pipeline would required either building the engine or having the engine preinstalled in the image. I tried building the Godot engine from source in the flatpak pipeline, but it didn't work. Also, if you want to audit the code, it's all opensource anyway.

File system home is being used for Godot's save game system. https://docs.godotengine.org/en/stable/tutorials/io/data_paths.html

@danirabbit
Copy link
Member

Touching acccountservice here is a hack and will probably break in future versions. Is there really no way to access dbus?

Well I know that for you it's probably true that the source in your GitHub repository is the source used to create your binary. But as a rule for an App Store, how can we verify this for other people? If we set a precedent to allow people to upload binaries I think that's really dangerous. We could maybe make like a rule that you can upload a binary if your app is 100% sandboxed but with broad permissions like home I don't think that's workable. It feels like people could really abuse that to cause big problems

@SubhadeepJasu
Copy link
Contributor Author

Let me try some ways.

@SubhadeepJasu
Copy link
Contributor Author

I have made some changes to the manifest but first we need to add the Godot base app to the repo for it to work.

@SubhadeepJasu
Copy link
Contributor Author

SubhadeepJasu commented Dec 15, 2024

Updated release signature to one which is compatible with AppCenter flow. Thanks @davidmhewitt and Cassidy for helping with this.

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.

3 participants