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

XDGData: misc fixes #4110

Closed
wants to merge 5 commits into from
Closed

XDGData: misc fixes #4110

wants to merge 5 commits into from

Conversation

eszlari
Copy link
Contributor

@eszlari eszlari commented Dec 5, 2020

Thank you for creating a pull request to contribute to FreeCAD! To ease integration, please confirm the following:

  • Branch rebased on latest master git pull --rebase upstream master
  • Unit tests confirmed to pass by running ./bin/FreeCAD --run-test 0
  • Commit message is well-written

And please remember to update the Wiki with the features added or changed once this PR is merged.
Note: If you don't have wiki access, then please mention your contribution on the 0.19 Changelog Forum Thread.


@luzpaz
Copy link
Contributor

luzpaz commented Dec 5, 2020

@eszlari interesting. Do you mind opening forum thread to discuss your PR?

@eszlari
Copy link
Contributor Author

eszlari commented Dec 6, 2020

I don't think there is anything controversial about these changes. These are just some minor cleanups.

@luzpaz
Copy link
Contributor

luzpaz commented Dec 6, 2020

why rename appdata to metainfo?

@eszlari
Copy link
Contributor Author

eszlari commented Dec 7, 2020

why rename appdata to metainfo?

The link to the spec is in the commit message:
https://www.freedesktop.org/software/appstream/docs/sect-Metadata-Application.html
This was changed some months ago.

@yorikvanhavre
Copy link
Member

This might potentially affect older distributions... Need to have a better look

@luzpaz
Copy link
Contributor

luzpaz commented Dec 7, 2020

I took a cursory look at all the https://www.freedesktop.org/software/appstream/docs/sect-Metadata-Application.html documentation and I couldn't find clear instructions on how to manage backward compatibility.

@eszlari
Copy link
Contributor Author

eszlari commented Dec 7, 2020

According to GNOME developers, this is supported since 2014:

https://gitlab.gnome.org/GNOME/gnome-software/-/issues/1110

@wwmayer
Copy link
Contributor

wwmayer commented Dec 12, 2020

When using the package search engine of Ubuntu then none of the packages of Xenial (2016) have their metainfo.xml file in the metainfo directory, it's still appdata:
https://packages.ubuntu.com/search?searchon=contents&keywords=metainfo.xml&mode=&suite=xenial&arch=any

With Bionic the majority of packages have moved these files to metainfo now:
https://packages.ubuntu.com/search?searchon=contents&keywords=metainfo.xml&mode=&suite=bionic&arch=any

When searching files of the form appdata.xml then they are still used a lot:
https://packages.ubuntu.com/search?searchon=contents&keywords=appdata.xml&mode=&suite=groovy&arch=any

When I got the package instructions of e.g. Fedora right then an appdata.xml is used for the main application and metainfo.xml is used for add-ons: https://docs.fedoraproject.org/en-US/packaging-guidelines/AppData/

So, this means that our appdata.xml file shouldn't be renamed but installed to a different directory on newer systems.

Now we can ignore the default install location for Xenial and thus have the risk that the meta information won't be shown (any more) because it will expire in a few months anyway or we can add a new CMake variable called CMAKE_INSTALL_METAINFO whose default value is "metainfo" but can be overridden by package maintainers of Xenial to be "appdata".

This PR must then be adjusted accordingly:

  • it should use the variable CMAKE_INSTALL_METAINFO if we still want to support Xenial
  • It should revert the change to rename to appdata.xml file (if I got things right)

@wwmayer
Copy link
Contributor

wwmayer commented Dec 12, 2020

Here is how KDE handles it:
https://community.kde.org/Guidelines_and_HOWTOs/AppStream

@luzpaz
Copy link
Contributor

luzpaz commented Dec 12, 2020

In a lot of way we aren't fully leveraging the AppData/MetaInfo aspect of FC. Should I open a ticket to explore this?

@eszlari
Copy link
Contributor Author

eszlari commented Dec 12, 2020

Just in case people are not aware what this file actually does:

There are three applications (that I know of) that use this file to display information to the user:

  1. https://gitlab.gnome.org/GNOME/gnome-software
  2. https://invent.kde.org/plasma/discover
  3. https://flathub.org

In case an app has no such file, GNOME Software and Plasma Discover fallback to the *.desktop file.

I doubt that people who use PPAs / back-port repositories for older distros, install these through such software center applications. And I guess more and more people are moving to AppImage, Flatpak or Snap and are not affected by this change anyway.

This PR must then be adjusted accordingly:

  • it should use the variable CMAKE_INSTALL_METAINFO if we still want to support Xenial
  • It should revert the change to rename to appdata.xml file (if I got things right)

Then I would simply keep the old suffix (appdata.xml), because I think this small change is not worth it to make the build system even more complicated than it already is.

@eszlari
Copy link
Contributor Author

eszlari commented Dec 12, 2020

When I got the package instructions of e.g. Fedora right then an appdata.xml is used for the main application and metainfo.xml is used for add-ons: https://docs.fedoraproject.org/en-US/packaging-guidelines/AppData/

This documentation is out of date, please refer to the official specification:
https://www.freedesktop.org/software/appstream/docs/sect-Metadata-Application.html

@yorikvanhavre yorikvanhavre added the ✋ On hold This PR must not be merged before some condition is met label Dec 14, 2020
@eszlari
Copy link
Contributor Author

eszlari commented Dec 19, 2020

ximion/appstream#294

Quote from the author of the spec:

Jup, since AppStream 0.9.4 (since 2016-04-18).
Every LTS distribution, including Debian, Ubuntu (including older ones like18.04 LTS) and AFAIK even RHEL via updates supports this, so it's pretty much safe to be used unconditionally.
In fact, if the AppStream 1.0 release plans had worked out for this year, the support for the old location would have been dropped already.
On my Debian system, /usr/share/appdata is empty (only Chrome puts its file in there for some reason), and last time I looked in all of Debian only 42 components of 2262 were using the old location.
So yeah, it's safe to switch. The .metainfo.xml suffix is ancient too, but you can stick with the appdata one, most apps did that for desktop-application components anyway, as the "you can use .metainfo.xml for any metadata" recommendation was done later. Also, we'll never deprecate the .appdata.xml suffix, unlike the old metadata location.

@luzpaz
Copy link
Contributor

luzpaz commented Dec 20, 2020

I wonder how this impacts snap packages + other ecosystems like Arch Linux and it's derivatives?

@luzpaz
Copy link
Contributor

luzpaz commented Dec 20, 2020

CC Linux distro package managers
@PrzemoF
@hobbes1069
@jsoo1

Edit: sorry to ping. Just wondering you could weigh in on this proposed change. Does it have any impact on downstream package managing for you?

@hobbes1069
Copy link
Contributor

Overall I agree with the changes. The icon names must match the desktop file name to be compliant. I don't know that the desktop file name needs to match the metainfo naming scheme but it doesn't hurt.

It looks like the appdata.xml extension is still supported so that would be "safe" unless there was a good mechanism to know what kind of system is being installed to OR a CMake option for the packager to specify manually.

@eszlari
Copy link
Contributor Author

eszlari commented Jan 4, 2021

I have reverted the rename to *.metainfo.xml.

@eszlari
Copy link
Contributor Author

eszlari commented Jan 25, 2021

ping @wwmayer

@eszlari
Copy link
Contributor Author

eszlari commented Feb 5, 2021

I would really like to get this in 0.19. If there is something else left to do, please let me know.

@wwmayer
Copy link
Contributor

wwmayer commented Feb 8, 2021

I would really like to get this in 0.19. If there is something else left to do, please let me know.

Although it's marked with "For 0.20" now I will have a look before the freeze.

@yorikvanhavre yorikvanhavre removed the ✋ On hold This PR must not be merged before some condition is met label Mar 26, 2021
@yorikvanhavre
Copy link
Member

(Sorry I had marked it automatically for 0.20 without looking well... Sorry about that!)

@yorikvanhavre
Copy link
Member

Leaving for you @wwmayer as you said you would have a look, but this looks good to merge to me

@yorikvanhavre
Copy link
Member

The internet domain of FreeCAD might change soon, so I think it's best to not use org.freecadweb anymore...

@berndhahnebach berndhahnebach added Packaging/building Related to building, compiling or packaging FreeCAD and removed For 0.20 labels Sep 17, 2021
@berndhahnebach
Copy link
Contributor

berndhahnebach commented Sep 24, 2021

Following a link to the branch on the CI-repository:

https://gitlab.com/freecad/FreeCAD-CI/-/commits/PR_4110

The CI-status is available on the latest commit of the branch.
If there is no status available the PR should be rebased on latest master.
Check pipeline branches to see if your PR has been run by the CI.

https://gitlab.com/freecad/FreeCAD-CI/-/pipelines?scope=branches

@luzpaz
Copy link
Contributor

luzpaz commented Nov 9, 2021

Related: 2cc5707 and 5ad8bed

@donovaly
Copy link
Member

donovaly commented Jan 7, 2022

The internet domain of FreeCAD might change soon, so I think it's best to not use org.freecadweb anymore...

@eszlari can you please do so and use "freecad.org". Then the PR can be merged.

@donovaly
Copy link
Member

@eszlari , if you don't reply this week, the PR cannot be merged for FC 0.20

@donovaly
Copy link
Member

donovaly commented Apr 3, 2022

No reply for more than year. Closing for now. Please reopen when the PR could be merged and is updated.
In every case it cannot be merged for FreeCAD 0.20

@donovaly donovaly closed this Apr 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Packaging/building Related to building, compiling or packaging FreeCAD
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants