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

Create org.pitivi.Pitivi.json #52

Closed
wants to merge 1 commit into from
Closed

Create org.pitivi.Pitivi.json #52

wants to merge 1 commit into from

Conversation

thiblahute
Copy link

@thiblahute thiblahute commented Jul 5, 2017

No description provided.

{
"app-id": "org.pitivi.Pitivi",
"runtime": "org.gnome.Platform",
"runtime-version": "3.22",
Copy link
Member

Choose a reason for hiding this comment

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

Any reason why not 3.24?

Copy link
Author

@thiblahute thiblahute Jul 5, 2017

Choose a reason for hiding this comment

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

Yes, this version has been developed and mostly tested on 3.22, only in current master we support 3.24 (fixed minor warning and stuff).

We also provide master (build on every commit) on http://flatpak.pitivi.org/ which is based on Sdk 3.24.

Copy link
Member

Choose a reason for hiding this comment

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

I would strongly recommend 3.24 as it has had numerous improvements like supporting theming, VAAPI support, GStreamer 1.10 and other dependencies you use, etc.

Copy link
Author

Choose a reason for hiding this comment

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

We do not want vaapi (not robust enough for us), our official development environment is flatpak and for a specific release we target a specific runtime/sdk version, changing randomly afterward is like what used to happen with distro, and this is really something I do not want.

"finish-args": ["--command=pitivi",
"--share=ipc",
"--socket=x11",
"--socket=session-bus",
Copy link
Member

Choose a reason for hiding this comment

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

Ideally only grant what you need rather than blanket access.

Copy link
Author

Choose a reason for hiding this comment

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

ipc, is the onlyone that might not be required, am I missing something else?

Copy link
Member

Choose a reason for hiding this comment

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

--socket=session-bus gives you unrestricted access to the dbus session. You should be using --talk-name=foo.bar instead.

},
"modules": [
{
"name": "yasm",
Copy link
Member

Choose a reason for hiding this comment

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

This is in the Sdk.

"build-options": {
"build-args": ["--share=network"],
"cflags": "-O0 -g -L/usr/lib -Lbuild/temp.linux-x86_64-3.4 -I/usr/include -I/usr/include/python3.4m/",
"env": {"ARCHFLAGS": "-arch x86_64"}
Copy link
Member

Choose a reason for hiding this comment

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

Flathub supports multiple arches so hardcoding this is wrong unless you only support that one arch.

Copy link
Author

Choose a reason for hiding this comment

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

Good point!

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

{
"name": "numpy",
"build-options": {
"build-args": ["--share=network"],
Copy link
Member

@TingPing TingPing Jul 5, 2017

Choose a reason for hiding this comment

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

Flathub is sandboxed, you cannot grant network access. You have to fix their build system to work offline.

This applies to many of the other modules too.

Copy link
Author

Choose a reason for hiding this comment

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

Why is that? What does it change to access the network from the host or from withing the sandbox?

Copy link
Member

Choose a reason for hiding this comment

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

This was mentioned on IRC but we want everything to be reproducable and it stops any possibility of any malicious network use.

]
},
{
"name": "meson",
Copy link
Member

@TingPing TingPing Jul 5, 2017

Choose a reason for hiding this comment

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

This is in the 3.24 Sdk.

Copy link
Author

Choose a reason for hiding this comment

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

Right, but we do not use 3.24 :-)

Copy link
Member

Choose a reason for hiding this comment

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

Bump as it was rebased to the 3.24 Sdk.

]
},
{
"name": "gst-plugins-good",
Copy link
Member

@TingPing TingPing Jul 5, 2017

Choose a reason for hiding this comment

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

Gstreamer 1.10 is In the 3.24 Sdk.

If you only need to build a few extra plugins specify so in the config-opts. Same goes for most of the other gstreamer modules.

Copy link
Author

Choose a reason for hiding this comment

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

Same.

Copy link
Member

Choose a reason for hiding this comment

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

Bump as it was rebased to the 3.24 Sdk.

Copy link
Author

Choose a reason for hiding this comment

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

We need our own build of Gst as we have backported patches that are not in the 1.12 branch and we build quite many more plugins than what is provided in the Sdk.

"name": "pitivi",
"buildsystem": "meson",
"builddir": true,
"config-opts": ["--libdir=lib", "--prefix=/app"],
Copy link
Member

Choose a reason for hiding this comment

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

Don't need to pass prefix manually.

Copy link
Author

Choose a reason for hiding this comment

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

True.

@thiblahute
Copy link
Author

Updated with the following changes:

  • Nework grant network access to the module builds directly using setup.py
  • Remove wrong "--share=ipc", "--socket=session-bus" access granding
  • Remove --prefix from pitivi target
  • Add gst-transcoder as a module as using it as a subproject lead to a wrong libdir path installation (ie. it was installing to lib64/
  • Remove archs definition as those were not needed (hopefully Pitivi will build on more arch :-))

@thiblahute
Copy link
Author

Anything missing?

@barthalion
Copy link
Member

No. Looks good to me (although I dislike that you are using 3.22, but I see your point); I scheduled a test build.

@TingPing
Copy link
Member

Build failed since 3.22 isn't available on the builders.

@thiblahute
Copy link
Author

Build failed since 3.22 isn't available on the builders.

I tested with 3.24 and we have regression (that are fixed in master), I will try to get pitivi 0.99 released soon and update that PR.

@purpleidea
Copy link

Build failed since 3.22 isn't available on the builders.

Why not?

@thiblahute
Copy link
Author

thiblahute commented Jul 19, 2017

Why not?

Also a very good question! :-)

@TingPing
Copy link
Member

Probably because nobody needed it yet and 3.24 has a lot of improvements. I imagine Alex isn't against adding it though.

@purpleidea
Copy link

@thiblahute

Also a very good question! :-)

Wasn't meant as sarcasm, but rather, I thought the whole point is that flatpak would allow application authors to target different runtimes, thus not breaking apps if gnome moves faster than they do for example. Personally, I think you should just get on the latest, but I understand both sides :)

@thiblahute
Copy link
Author

@thiblahute

Also a very good question! :-)
Wasn't meant as sarcasm, but rather, I thought the whole point is that flatpak would allow application authors to target different runtimes, thus not breaking apps if gnome moves faster than they do for example.

I was not taking it as sarcasm, just stating that it would also be good to have.

Personally, I think you should just get on the latest, but I understand both sides :)

Well we have but it is not released yet.

@alexlarsson

Probably because nobody needed it yet and 3.24 has a lot of improvements. I imagine Alex isn't against adding it though.

(How) could we get that done?

@TingPing
Copy link
Member

@thiblahute I'm curious where the regression you have is? Because they should have the same Gtk version (3.24 doesn't exist), they should have the same GStreamer since you build it yourself, etc.

@thiblahute
Copy link
Author

@thiblahute I'm curious where the regression you have is? Because they should have the same Gtk version (3.24 doesn't exist), they should have the same GStreamer since you build it yourself, etc.

We have an issue in latest meson (we could workaround that), and in Gtk between 3.22.6 (Sdk 3.22.0) and 3.22.14 in the new Sdk.

@TingPing
Copy link
Member

and in Gtk between 3.22.6 (Sdk 3.22.0) and 3.22.14 in the new Sdk.

Hmm, well it is probably just chance that the old Sdk didn't get those Gtk3 updates anyway. As you mention Meson is trivial to workaround.

@thiblahute
Copy link
Author

OK, 0.99 is out based on the 3.24 runtime, please review again :-)

@@ -0,0 +1,497 @@
{
"app-id": "org.pitivi.Pitivi",
"branch": "1.0",
Copy link
Member

Choose a reason for hiding this comment

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

Set it to stable or remove altogether, our CI sets it on its own.

Copy link
Author

Choose a reason for hiding this comment

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

Done.

"--socket=x11",
"--socket=pulseaudio",
"--socket=wayland",
"--socket=session-bus",
Copy link
Member

Choose a reason for hiding this comment

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

Does it need access to entire session bus?

Copy link
Author

Choose a reason for hiding this comment

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

Well, without it Pitivi doesn't start and we get:

Failed to register: GDBus.Error:org.freedesktop.DBus.Error.ServiceUnknown: org.freedesktop.DBus.Error.ServiceUnknown

but I must be missing something, couldn't figure out what :-)

Copy link
Member

Choose a reason for hiding this comment

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

"branch": "1.0",
"runtime": "org.gnome.Platform",
"runtime-version": "3.24",
"finish-args": ["--command=pitivi",
Copy link
Member

Choose a reason for hiding this comment

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

Use command top level property instead.

Copy link
Author

Choose a reason for hiding this comment

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

Done.

"rename-icon": "pitivi",
"copy-icon": true,
"build-options": {
"cflags": "-O0 -g",
Copy link
Member

Choose a reason for hiding this comment

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

So, a nitpick… Why not -O2?

Copy link
Author

Choose a reason for hiding this comment

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

Removed for releases.

"GST_PLUGIN_SYSTEM_PATH": "/app/lib/gstreamer-1.0/",
"FREI0R_PATH": "/app/lib/frei0r-1/"
},
"strip": false,
Copy link
Member

Choose a reason for hiding this comment

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

It's the default.

Copy link
Author

Choose a reason for hiding this comment

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

Remains from the past :-)

{
"name": "numpy",
"buildsystem": "simple",
"ensure-writable": ["easy-install.pth"],
Copy link
Member

Choose a reason for hiding this comment

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

@TingPing does it look right to you?

Copy link
Author

Choose a reason for hiding this comment

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

He suggested the trick :-)

]
},
{
"name": "sound-theme-freedesktop",
Copy link
Member

Choose a reason for hiding this comment

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

It's in the runtime since a while.

Copy link
Author

Choose a reason for hiding this comment

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

Removed.

},
{
"name": "x265",
"cmake": true,
Copy link
Member

Choose a reason for hiding this comment

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

Please use "buildsystem": "cmake", cmake as key is marked as deprecated.

Copy link
Author

Choose a reason for hiding this comment

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

Done.

]
},
{
"name": "gst-plugins-good",
Copy link
Member

Choose a reason for hiding this comment

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

Bump as it was rebased to the 3.24 Sdk.

]
},
{
"name": "meson",
Copy link
Member

Choose a reason for hiding this comment

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

Bump as it was rebased to the 3.24 Sdk.

@barthalion
Copy link
Member

barthalion commented Sep 24, 2017

Just now when we're about to switch to 3.26! :)

We need some things to be changed, but I started a test build though: https://flathub.org/builds/#/builders/1/builds/451

@thiblahute
Copy link
Author

thiblahute commented Sep 24, 2017

Just now when we're about to switch to 3.26! :)

Well, Pitivi 1.0 is not going to be based on it ;)

And thanks for the review!

@TingPing
Copy link
Member

Looks like matplotlib fails.

@thiblahute
Copy link
Author

thiblahute commented Sep 24, 2017

To me the issue is the usage of build-args, which I am removing now. (still testing locally before updating my patches).

@thiblahute
Copy link
Author

Can you trigger a rebuild please?

@TingPing
Copy link
Member

@thiblahute
Copy link
Author

OK, works only on 64bits whch is the only one I care about for now (only tested one), I added a flathub.json so that it is the only one built for now, is that ok?

@AsavarTzeth
Copy link

You would need to fix lame not building on all architectures the same way as com.github.JannikHv.Gydl and org.audacityteam.Audacity.

This will be the third application that has to solve the same issue. I will hence repeat my suggestion to make it a shared module.

@TingPing
Copy link
Member

@AsavarTzeth, Seems acceptable, want to open a PR against flathub/shared-modules

@AsavarTzeth
Copy link

@TingPing Sure, I could do it right away. Should not take long.

@thiblahute
Copy link
Author

thiblahute commented Sep 25, 2017

What about building only x64-64 for now? My problem is that I am not sure how to test other arch (and I am not sure I care so much right now).

@TingPing
Copy link
Member

It is acceptable for now, you probably still want to use the lame version just added to shared-modules though.

"sources": [
{
"type": "git",
"branch": "1.12.3",
Copy link
Member

Choose a reason for hiding this comment

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

If you want 1.12 now the 3.26 runtime is out, so why wouldn't you use that.

Copy link
Author

Choose a reason for hiding this comment

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

"We need our own build of Gst as we have backported patches that are not in the 1.12 branch and we build quite many more plugins than what is provided in the Sdk."

"name": "pitivi",
"buildsystem": "meson",
"builddir": true,
"config-opts": ["--libdir=lib"],
Copy link
Member

Choose a reason for hiding this comment

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

You can drop all of these --libdir=lib now, this was patched in the Sdk.

"--disable-libmfx",
"--disable-libnpp",
"--disable-iconv",
"--disable-jni",
Copy link
Member

Choose a reason for hiding this comment

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

It might be more manageable for you to pass --disable-everything.

Copy link
Author

Choose a reason for hiding this comment

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

I am just copy/pasting what is done in the gst-libav statically build ffmpeg submodule.

},
{
"name": "gsound",
"ensure-writable": ["easy-install.pth"],
Copy link
Member

Choose a reason for hiding this comment

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

I imagine this doesn't need to be here.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed.

"finish-args": ["--socket=x11",
"--socket=pulseaudio",
"--socket=wayland",
"--socket=session-bus",
Copy link
Member

Choose a reason for hiding this comment

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

No reason to pass through entire bus.

Copy link
Author

Choose a reason for hiding this comment

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

@thiblahute
Copy link
Author

Ping?

@AsavarTzeth
Copy link

You made a typo:

Can't parse 'org.pitivi.Pitivi.json': <data>:466:18: Parse error: unexpected character `,', expected character `]'

@thiblahute
Copy link
Author

thiblahute commented Oct 5, 2017

Sorry about that! I am now using lame from the submodule and fixed that issue.

Could you trigger a rebuild?

@barthalion
Copy link
Member

https://flathub.org/builds/#/builders/1/builds/546

@TingPing
Copy link
Member

TingPing commented Oct 5, 2017

Repository has been created: https://github.com/flathub/org.pitivi.Pitivi

@TingPing TingPing closed this Oct 5, 2017
fooishbar pushed a commit to fooishbar/flathub that referenced this pull request Apr 8, 2020
tilosp added a commit to tilosp/flathub that referenced this pull request Aug 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants