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 missing QML dependencies #27

Merged
merged 1 commit into from
Dec 7, 2023
Merged

Conversation

dcaliste
Copy link
Collaborator

@dcaliste dcaliste commented Dec 6, 2023

Following @nephros suggestion, I'm adding Requires: qml(org.nemomobile.mpris) but also for org.nemomobile.policy and for org.nemomobile.thumbnailer which are used in various QML files.

I'm hesitating to also add Requires: qml(com.jolla.mediaplayer) which is imported by the main flowplayer.qml file. What do you think ?

@dcaliste
Copy link
Collaborator Author

dcaliste commented Dec 6, 2023

Post scriptum, I didn't pay attention, but I targetted this for master. If an issue, I can rebase it and target devel instead...

@nephros
Copy link

nephros commented Dec 6, 2023

I think some of those imports names are deprecated , having been replaced by Nemo.Foo aliases.

Now, if that is the case the qml imports should be changed.

But, then we need to check that the RPMs containing those have proper Provides:.

If not, the spec should use the rpm names instead of the qml() variant.

I am not at a SFOS device right now but can check this later.

@nephros
Copy link

nephros commented Dec 6, 2023

Thumbnailer:

rpm -qf /usr/lib64/qt5/qml/Nemo/Thumbnailer/qmldir
nemo-qml-plugin-thumbnailer-qt5-1.0.5-1.6.3.jolla.aarch64
nemo@PGXperiiia10:~/tmp $ rpm -qP nemo-qml-plugin-thumbnailer-qt5
libnemothumbnailer-qt5.so.1()(64bit)
libnemothumbnailer.so()(64bit)
nemo-qml-plugin-thumbnailer-qt5 = 1.0.5-1.6.3.jolla
nemo-qml-plugin-thumbnailer-qt5(aarch-64) = 1.0.5-1.6.3.jolla
qml(Nemo.Thumbnailer)
qml(org.nemomobile.thumbnailer)

@nephros
Copy link

nephros commented Dec 6, 2023

Policy :

rpm -qf /usr/lib64/qt5/qml/Nemo/Policy/
nemo-qml-plugin-policy-qt5-0.1.3-1.6.2.jolla.aarch64
nemo@PGXperiiia10:~/tmp $ rpm -qP nemo-qml-plugin-policy-qt5/qmldir
libnemopolicy.so()(64bit)
nemo-qml-plugin-policy-qt5 = 0.1.3-1.6.2.jolla
nemo-qml-plugin-policy-qt5(aarch-64) = 0.1.3-1.6.2.jolla
nemo-qml-plugins-policy-qt5
qml(Nemo.Policy)
qml(org.nemomobile.policy)

@nephros
Copy link

nephros commented Dec 6, 2023

Mpris and mediaplayer have no aliased names, and have the proper Requires: in the rpm, so these are fine.

@Olf0 Olf0 changed the base branch from master to devel December 6, 2023 20:53
@Olf0
Copy link
Contributor

Olf0 commented Dec 6, 2023

@dcaliste,

What do you think ?

Honestly, I have no idea. I am not a good programmer by any means (well, maybe except for UNIX shell code) and I really do not want to dive into coding aspects (here: code dependencies).

My basic concept for this "GitHub organisation" is to provide the roles as release maintainer, repo maintainer, webpages maintainer, and (very) basic reviewer, so people who want to contribute code (changes) can focus exactly on this.

But I can ask (and may be even good at this, sometimes) checks & balances questions:
Why are you "hesitating to also add Requires: qml(com.jolla.mediaplayer)", as it seems the right thing to do (with my limited understanding)?

Side note: While CepiPerez's code appears to be basically sound and comprehensible, he seems to have missed some infrastructure aspects, such as dependencies of various kinds etc.; which is understandable, because he was authoring and maintaining all his software alone and never had someone to review this, AFAIK. Hence having to add and rectify a couple of these infrastructure aspects is somewhat expected.


AFAIU @nephros's two comments WRT proper dependency statements ([1], [2]), your extant changeset uses the deprecated dependency statements, which should better be replaced by:
Requires: qml(Nemo.Policy)
Requires: qml(Nemo.Thumbnailer)

Unfortunately he did not comment your statement:

I'm hesitating to also add Requires: qml(com.jolla.mediaplayer) which is imported by the main flowplayer.qml file.

Maybe @nephros would like to comment, if he also thinks this should be added. It is not part of this PR, currently.


P.S.: Rebased this PR to the devel branch and updated to the latest commit there.

@dcaliste
Copy link
Collaborator Author

dcaliste commented Dec 7, 2023

Thank you @nephros for your reviews. Indeed, I know that now some nemo QML plugins are using the Nemo.Xxx syntax, instead of the deprecated org.nemomobile.xxx one. I didn't put the requires with the new naming scheme to be consistent with the fact that the code is still using the old naming scheme, and also because like that, it may still be valid for older SailfishOS versions. Maybe a separated commit to migrate the code would better separate the fix of the RPM dependencies issues and the migration requirement.

About the Requires: syntax, I think, I remembered having read on the forum an advice from one of the sailor not to use the package name (i.e. Requires: nemo-qml-plugin-mpris-qt5-devel) but instead to use the qml() syntax. Do you suggest to use the devel package name, or I misunderstand your remark ?

@nephros
Copy link

nephros commented Dec 7, 2023

I didn't put the requires with the new naming scheme to be consistent with the fact that the code is still using the old naming scheme, and also because like that, it may still be valid for older SailfishOS versions

Good point! All good.

About the Requires: syntax, I think, I remembered having read on the forum an advice from one of the sailor not to use the package name (i.e. Requires: nemo-qml-plugin-mpris-qt5-devel) but instead to use the qml() syntax. Do you suggest to use the devel package name, or I misunderstand your remark ?

No, that's fine and recommended. What I meant is that sometimes the official Jolla packages do not have the required Provides: sections for things like qml() (or pkgconfig() or cmake()) to work. In those cases the -devel variant must be used.
But that is not the case here.

It's adding:
- org.nemomobile.policy
- org.nemomobile.thumnailer
- com.jolla.mediaplayer
@dcaliste
Copy link
Collaborator Author

dcaliste commented Dec 7, 2023

Ok, @nephros, thank you for the explanation. I agree with you then !

I've rebased the commit on devel, removed already included qml(org.nemomobile.mpris) and added qml(com.jolla.mediaplayer). I've checked on device that the jolla-mediaplayer RPM is providing this qml() "feature".

Copy link
Contributor

@Olf0 Olf0 left a comment

Choose a reason for hiding this comment

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

Thank you very much, this is looking fine.

@Olf0
Copy link
Contributor

Olf0 commented Dec 7, 2023

I didn't put the requires with the new naming scheme to be consistent with the fact that the code is still using the old naming scheme, and also because like that, it may still be valid for older SailfishOS versions

Good point! All good.

Thanks to both of you: This is exactly what I figured this morning, too.

In general, SailfishOS:Chum supports all SFOS ≥ 3.1.0, the compilation via GitHub-CI-workflow works fine for SFOS ≥ 3.0.2 and works for SFOS ≥ 2.2.0 (but produces ballooned binaries, for some reason). Hence I would like to keep the code supporting old releases as long as feasible.

@Olf0 Olf0 merged commit bdccc23 into sailfishos-applications:devel Dec 7, 2023
1 check passed
@Olf0 Olf0 changed the title Add missing QML dependency. Add missing QML dependencies Dec 7, 2023
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