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

cmake flatpak #96

Closed
ozolli opened this issue May 19, 2022 · 82 comments
Closed

cmake flatpak #96

ozolli opened this issue May 19, 2022 · 82 comments

Comments

@ozolli
Copy link

ozolli commented May 19, 2022

I have no idea what the cmake command line parameters have to be set to compile a flatpak plugin...

Rick, can you help me please?

@rgleason
Copy link
Owner

@leamas Good question! I think we need to have Alec Leamas advise.

@ozolli
Copy link
Author

ozolli commented May 19, 2022

And do the Weatherfax 1.9.31 and 1.9.32 flatpak build fail on you? I don't see them on Cloudsmith.

@rgleason
Copy link
Owner

@ozolli @bdbcat was going to take a look at the 4 failing flatpak builds when he had a chance. I also asked @jongough about this because he might know how to fix it. There is some problem with finding the right libs for the ftp feature.

@bdbcat
Copy link
Collaborator

bdbcat commented May 19, 2022

Working on this with Alec. Not trivial....
Hope for progress today.
Dave

@rgleason
Copy link
Owner

Much appreciated. ..Did not realize it was a problem.

@bdbcat
Copy link
Collaborator

bdbcat commented May 19, 2022

PR posted to your repo now.
Fingers crossed....

@leamas
Copy link

leamas commented May 19, 2022

hm... @rgleason: If you got a PR moving weatherfax to the shipdriver templates: would that be a good thing or not?

@rgleason
Copy link
Owner

Alec, that would be ok with me, I am a little nervous about it though.

There are a lot of moving parts right now. We are updating the data xml files (links to MET services), also trying to get the Weatherfax schedules updated and I would like to get the Kap function fixed and confirm that we can decode sound and fix that if necessary.

It would probably be better after we get this done, unless you think it would help.

@rgleason
Copy link
Owner

rgleason commented May 19, 2022

PR posted to your repo now.
Fingers crossed....

It built the flatpaks! now finishing with MacOS. I had one user say MacOS is not working, but no detail. Going to put this up on Beta channel and get it tested by the crew.

Thank you Dave and Alec.
BTW Ozolli did a super job on the WFIR xml files!

@leamas
Copy link

leamas commented May 19, 2022

It would probably be better after we get this done, unless you think it would help.

let's wait then. That said, I actually think it helps in the long run.

@rgleason
Copy link
Owner

Understood Alex.

@ozolli
Copy link
Author

ozolli commented May 19, 2022

It built the flatpaks!

Flatpak ftp download working fine on Rocky Linux 8.6. Well done guys !

@ozolli ozolli closed this as completed May 19, 2022
@jongough
Copy link
Collaborator

Have extended the ability to add extra libs to flatpak, armhf and arm64 builds in frontend2 1.0.205.0 . The method implemented here breaks the ability to use a template build process.

@bdbcat
Copy link
Collaborator

bdbcat commented May 20, 2022

Jon...
I took a pragmatic approach to get the weatherfax_pi to build, so that it might be released. I'd be happy to follow your ideas so that the template stays sound.
Please describe your design, and how it might be used to include libcurl on flatpak.

@jongough
Copy link
Collaborator

This is part of the testplugin_pi that I used for OD development and now for frontend2. If you go to "https://github.com/jongough/testplugin_pi" you will find an extras directory under the ci directory. In this directory there is a README.txt file and an extra_libs.txt file. I created the second form of file name in the readme for the missing ssllib and added the requisite code to the ci script.
This is the code that does it:

# Install extra libs
ME=$(echo ${0##*/} | sed 's/\.sh//g')
EXTRA_LIBS=../ci/extras/extra_libs.txt
if test -f "$EXTRA_LIBS"; then
    while read line; do
        sudo apt-get install $line
    done < $EXTRA_LIBS
fi
EXTRA_LIBS=../ci/extras/${ME}_extra_libs.txt
if test -f "$EXTRA_LIBS"; then
    while read line; do
        sudo apt-get install $line
    done < $EXTRA_LIBS
fi

and the entry in the extras directory is:

circleci_build_flatpak.txt

containing:

libcurl4-openssl-dev

The idea being that a developer can use the template builds and add customisations to the installed packages without modifying the provided templates. The only file that needs to be maintained by the developer is the CMakeLists.txt as all the customisation for cmake is in there. The 'libs' directory contains the extra libs that have to be build with their own CMakeLists.txt file. As we now build for 18 different platforms it is much easier to have one person maintain the build templates and others just use them by upgrading (copying) the new/changed files. The build process can be customised during development to build only those environments required and, with the latest patches, use apt proxies and avoid downloading master.zip through the use of circleci local.

Hope this helps.

@leamas
Copy link

leamas commented May 20, 2022

Here are really two things. The first is about using curl in Flatpak. The simple answer is that curl is included in the flatpak runtime, the flatpak OS so to speak. This means that only thing needed to use curl in flatpak is to let cmake find it and then use the cmake data to add include headers and library. Nothing special is required for this, cmake finds it out of the box.

@jongough
Copy link
Collaborator

I only added the file that Dave added to the flatpak build. The process to do so exists in frontend2, I just updated the process to include a few other scripts that use apt. I made the update to the file that could have been used rather than modifying a standard template, the script file, that would cause this plugin issues in future when changes are made. If it is not needed it is one file to remove, the template will stay the same.

@leamas
Copy link

leamas commented May 20, 2022

The other is the use of EXTRA_LIBS in Flatpak. In general this does not work, but see below on -dev packages

Flatpak runs in a sandbox, and the only libs a flatpak application sees are those in the runtime and possible libraries built from source. In particular, libraries installed using apt-get are not visible in the sandbox.

Building from source could be done as part of the plugin build, or as a separate source in the .yaml flatpak manifest.

For a plugin, the runtime used is OpenCPN. This means that besides the standard libraries like for example curl also all libraries linked by OpenCPN are available. This is the reason for example wxWidgets is visible for plugins. Which means that if a plugin needs to link to a library, the Flatpak build has three alternatives:

  1. The library is available in the Flatpak runtime. All is fine, just let cmake do it's job.
  2. The library is available in the OpenCPN runtime. In this case, the headers are missing which is pretty complicated. The simple solution is to avoid it. The alternative is to somehow provide correct headers (and to maintain them)
  3. Build the library from source. This could be done as part of the plugin build or by adding a new module to the Flatpak .yaml manifest

Added to this is of course MacOS and Windows. We cannot use any external libraries here either, we cannot assume users are using package managers like choco or Homebrew. This basically boils down to that libraries not available in OpenCPN are better off built from source, this is a solution which works everywhere.

For Debian, there is a special case with a library like curl which is available in the runtime because it's linked to OpenCPN. To link against such libraries the build needs the corresponding -dev pack (for curl, it needs libcurl-dev). This is OK to add to the build environment since it does not add a requirement to the plugin runtime. libcurl4-openssl-dev is another example on this. But then again, this does not affect a Flatpak build

As I see it, these are hard facts that a templating system needs to adapt to.

@rgleason
Copy link
Owner

Hmm. It's not simple, but it is. I guess we have to build the curl library in weatherfax?

@rgleason
Copy link
Owner

rgleason commented May 20, 2022

JG has kindly provided a PR for weatherfax which uses his method. I am going to accept it as that will conform with his scripts in TP. Improvement PR's very welcome. @bdbcat Thank you for getting it working, testers are working with it now.

@rgleason
Copy link
Owner

Well now the 4 flatpaks are failing (android usually fails anyway, should comment it out). JG's config has a problem with flatpak as Alec noted. Perhaps I should revert that PR?

@rgleason
Copy link
Owner

I reverted Jon's PR so weatherfax would work with flatpak.

@leamas
Copy link

leamas commented May 20, 2022

Hmm. It's not simple, but it is. I guess we have to build the curl library in weatherfax?

I have yet not looked at the code.Or rather, read but not to the point I understand.

But as for Flatpak, no. It is included in the Flatpak runtime.

@bdbcat
Copy link
Collaborator

bdbcat commented May 20, 2022

Alec...
Just for my clarification:
If we simply install "libcurl4-openssl-dev" in the host build system, how is it that we know that the same API is available for the libcurl.so which is included in the "sandbox" by virtue of being part of OCPN?

@bdbcat
Copy link
Collaborator

bdbcat commented May 20, 2022

Note on Windows:
libcurl.dll, and all required header files, is included in the downloaded build dependency package. So I see no need to rebuild from source in a plugin.

@leamas
Copy link

leamas commented May 20, 2022

@bdbcat: It's the other way around (I think)

  • libcurl.so is available in the Flatpak runtime, this is what OpenCPN links to. As a consequence, the header and unversioned so-lib which is used when compiling is available in the Flatpak SDK. In other words, curl is between the plugin and the Flatpak runtime, OpenCPN is not really involved (besides also using it).
  • libcurl4-openssl-dev is invisible in Flatpak. However, when building Debian plugins it provides headers and unversioned so-lib for the runtime package which is pulled in by OpenCPN. So in this case it makes sense.

@leamas
Copy link

leamas commented May 20, 2022

libcurl.dll, and all required header files, is included in the downloaded build dependency package. So I see no need to rebuild from source in a plugin.

In that case we should probably update the curl library I provided to you, have the Windows headers and dll file(s) there and use them on Windows. It's much easier to make this system modular if we group things in a library which covers all platforms rather than having all windows stuff on one place, macos in another , etc.

Will look into it, but now heading for wine and Friday dinner.

@bdbcat
Copy link
Collaborator

bdbcat commented May 20, 2022

So, can we not get the curl headers from the flatpak SDK? I tried to do this, but cannot find the directory "/app...." in the build system, although flatpak-builder seems to know about it. I poked around using a CCI ssh login.

@leamas
Copy link

leamas commented May 20, 2022

So, can we not get the curl headers from the flatpak SDK?

Yes, cmake finds them with default settings

@rgleason
Copy link
Owner

rgleason commented May 23, 2022

Yes, agreed. I also notice that both macOS fail too.
As an aside some other OS issues with the plugin
OpenCPN/OpenCPN#2616 (reply in thread)

@leamas
Copy link

leamas commented May 25, 2022

Finallly found some time and have something which builds on all platforms besides Android. This is far from completed, the history and partly also the code is a mess, but it works. Notes:

  • rtlsdr is fully supported on Linux and Macos without external packages (built from source).
  • On windows it is possible to build rtlsdr but it creates a dependency on the libusb windows driver which must be installed. Plan is to make this an option for interested devs, by default off.
  • Libaudiofile needs some patching to build on windows. Nothing spectacular, seems safe.
  • The patching of libs/curl/CMakeLists.tx done by Dave works in the testplugin context. However, the need of this patching somehow shows that this context looks complicated when including libraries. Proper done libraries should be possible to reuse in all plugins and also main OpenCPN, but the testplugin framework seemingly does not allow this.
  • Android needs to be fixed, but I see no roadblocks, just some work.

@leamas
Copy link

leamas commented May 25, 2022

I'm making this mostly to apply the templates on a harder usecase. It might end up in a PR or not. So far the templates looks sound, there are no changes besides fixing a nasty bug which hasn't been visible until facing this plugin.

@leamas
Copy link

leamas commented May 25, 2022

No, Android won't fly. Leaving it unimplemented

@bdbcat
Copy link
Collaborator

bdbcat commented May 25, 2022

If the "won't fly" is due solely to curl, this is no problem. We always use Java host side for internet access on this platform. Just need some (more) "#ifdefs" in the source. Let us agree that Android is "pathological" in some respects.

@bdbcat
Copy link
Collaborator

bdbcat commented May 25, 2022

Are you working with Jon's template, or moving to SD?

@leamas
Copy link

leamas commented May 25, 2022

I'm moving to SD. The core reason is that it doesn't seem possible to include libraries in a sane way.

I just found a precompiled android out there. Will give it a try, but not today. Talking about android, there is some discrepancies in Weatherfax.cpp/h. Basically, the cpp file refers to events not defined in the header. BBL

@jongough
Copy link
Collaborator

If there is an issue with the frontend2 build process then it would be helpful for an issue to be raised with an example of where it is failing and, hopefully, a pull request generated to fix it. If there is no issue raised it is very difficult to identify and fix insane issues.

@bdbcat
Copy link
Collaborator

bdbcat commented May 25, 2022

Alec...
It would be useful to describe the troubles encountered while trying to include libraries on the FE2 process, even in schematic form. FE2 is in current production use, and should be maintained to best standards.

@rgleason
Copy link
Owner

See the test reports by ALROCHARARI Raoul Richard for weatherfax in the Testing Spreadsheet
https://docs.google.com/spreadsheets/d/19JvjTHTXqzE-atlgQ_SPdqTaAxfCUAiQ5_1vXa1ENb0/edit#gid=0

Some of these builds are not working with plugins.

@leamas
Copy link

leamas commented May 26, 2022

The Frontend workflow has drawbacks related to build dependencies:

  • In a Flatpak build, libraries are included in the host part of the workflow. This means that the library has to cope with Frontend specific stuff, and it's unclear if it can be applied to the Flatpak build at all.
  • The manifest is generated from a common template. This solution is probably not sustainable when there is a need to add Flatpak dependencies in the form of added modules in the manifest. Since there is no manifest available it is also really hard to make local Flatpak builds.

These were the reasons I dropped the workflow the current Frontend is based upon (which was my first attempt to make a workflow, so I am the one to blame). I see no easy fixes to the FE2 setup which fixes this. Could be wrong, for sure.

OTOH, current FE2 works fine for "most" plugins without complicated build dependencies. We could thus continue to use it in most current of the current plugins, keeping a window open to apply the SD templates instead when the build deps makes it necessary.

Examples: Any library in opencpn-libs. These are written without any assumptions about the framework they are used in, it's basically generic code which works in both main OpenCPN, the SD templates or actually any context I could think of. They should in Frontend as well, but...

@jongough
Copy link
Collaborator

There is no explanation as to what is wrong with FE2. Looking at the flatpak yaml there is little difference in outcomes. The FE2 process is a build template which is maintained to ease the process of building for multiple environments. All customisation is applied in CMakeLists.txt and any extra cmake, directories, libraries, etc, that the developer wants. If this is insufficient then an explanation, possibly with examples, needs to be provided for those who do not intimately understand flatpak and its limitations (there appear to be quite a few looking at the issues people are having with flatpak as a whole and not OCPN in particular).
Raise a pull request against testplugin to help those who do not understand flatpak to implement it.

@leamas
Copy link

leamas commented May 26, 2022

@jongough : Expanding on the build dependencies problem, the basic flow in the FE ci script which builds Flatpak is:

  1. Setup environment + flatpak and flatpak-builder
  2. Install build dependencies
  3. Run cmake
  4. Run make

This flow does not really take the fact that Flatpak is built in a sandbox into account. This sandbox is isolated from the host environment, with it's own set of dependencies known as the Flatpak runtime. This means:

  • That installing the build dependencies is useless, they are not visible in the sandbox.
  • That all cmake code runs outside the sandbox, no configuration is done within it. Which means that using libraries is not really feasible.
  • The part which runs within the sandbox is the make command configured in the yaml manifest.

The ShipDriver (SD) templates works differently. The cmake configuration is split into two parts. The first is done when invoking cmake, and is also done outside the sandbox. However, in the SD case make flatpak invokes a second round of cmake configuration which makes the heavy lifting. This part runs in the sandbox. Hence, libraries are configured as expected also in the Flatpak case.

@leamas
Copy link

leamas commented May 26, 2022

Add to this that some Flatpak dependencies are added as snippets to the manifest. For example, this is the glu dependency:

   - name: glu
      config-opts:
          - --disable-shared
          - --enable-static
          - --prefix=/app/extensions/weatherfax
      sources:
          - type: archive
            url: https://mesa.freedesktop.org/archive/glu/glu-9.0.1.tar.xz
            sha256: fb5a4c2dd6ba6d1c21ab7c05129b0769544e1d68e1e3b0ffecb18e73c93055bc
      cleanup: ["/include", "/lib/*.a", "/lib/*.la", "/lib/pkgconfig"]

The list of such dependencies varies from plugin to plugin. It is of course possible to handle is by templating , but it's non-trivial and likely to fail anyway as soon as a new or updated snippet is required. This is what makes the single template approach in FE somewhat problematic here.

@leamas
Copy link

leamas commented May 26, 2022

But then again, FE works just fine for most plugins:

  • Those without specific build dependencies
  • Those which requires things like curl which a) is pulled in by OpenCPN and b) available in the Flatpak runtime. This works just fine when building Flatpak.

This actually covers the bulk of the available plugins. However, this plugin is seemingly not such a case.

@rgleason
Copy link
Owner

So Jon's efforts 6 days ago jongough/testplugin_pi#263 (comment) to to "add extra libs to flatpack and android builds" is not going to work?

A second cmake command has to be executed so that the necessary libs are packed from inside of the flatpak "sandbox" using the lib packages provided by flatpak?

@jongough
Copy link
Collaborator

jongough commented May 26, 2022

The process is already a two step one with both config and build as a two pass process. The to file is built from a file in 'in-files' and passed to the build process. It looks very similar to the one shipdriver uses except it is generic with Cmake modifying it for the particular plugin. It may need to be modified to allow extra sections which could be provided on the same way as the ci/extras are. It just needs an example of what is needed. Leamas/rasbats or Dave may be able to provide this.

@leamas
Copy link

leamas commented May 26, 2022

So Jon's efforts 6 days ago jongough/testplugin_pi#263 (comment) to to "add extra libs to flatpack and android builds" is not going to work?

Not for Flatpak, unsure about Android. Let's focus on Flatpak for now.

@bdbcat
Copy link
Collaborator

bdbcat commented May 26, 2022

Jon...
Rick's weatherfax_pi master repo does work, using FE2. The required changes were made in CMakeLists.txt, along with the addition of a new directory in libs. The flow recognizes the two step cmake process. See around line 468:

if(NOT OCPN_FLATPAK_CONFIG)
    # Build environment not available when flatpak is being configured so following statements will not work
    message(STATUS "${CMLOC}Adding target link libraries to ${PACKAGE_NAME}")

IF(NOT QT_ANDROID)
    add_subdirectory("libs/curl")
    target_link_libraries(${PACKAGE_NAME} libcurl::libcurl)

    add_subdirectory("libs/wxcurl")
    target_link_libraries(${PACKAGE_NAME} ocpn::wxcurl_src)
ENDIF(NOT QT_ANDROID)

    add_subdirectory(libs/tinyxml)
    target_link_libraries(${PACKAGE_NAME} ocpn::tinyxml)
endif(NOT OCPN_FLATPAK_CONFIG)

As I see it, Leamas' work is to generalize the process, so that arbitrary libraries, even ones which are not available in the flatpak SDK nor in the OCPN core set, may be added to a plugin without hacking a template.
But as far as weatherfax_pi goes, I don't think that there is any immediate problem to be resolved.

@leamas
Copy link

leamas commented May 26, 2022

It looks very similar to the one shipdriver uses except it is generic with Cmake modifying it for the particular plugin

And still, it isn't. It can be demonstrated by snippets like:

if(NOT OCPN_FLATPAK_CONFIG)
    # Build environment not available when flatpak is being configured so following statements will not work
    message(STATUS "${CMLOC}Adding target link libraries to ${PACKAGE_NAME}")

    add_subdirectory(libs/tinyxml)

    target_link_libraries(${PACKAGE_NAME} ocpn::tinyxml)
endif(NOT OCPN_FLATPAK_CONFIG)

Something is broken here: why cannot the configuration use the tinyxml subdirectory (which in this case will pick up tinyxml available in the Flatpak runtime). A well working system should no have such limitations, the configuration should run in the same way be it in the sandbox or not. In the more general case with libraries not present in the runtime like rtlsdr it collapses.

I will try to show an alternative implementation in a day or two, if not else just to show other paths. But a bit short of time these days.

@leamas
Copy link

leamas commented May 26, 2022

@bdbcat. In the basic sense you are right: my work is about generalizing to allow arbitrary libraries to be used, not only those in available in the runtime. And again, this covers most but not all plugins.

In this case it is about the rtlsdr support. This can be made available for all platforms besides Android using a more general approach (although Windows requires a libusb driver installation, a questionable requirement).

If we all could agree that FE has a limitation when it comes to build dependencies all is well. FE works just fine in most but not all cases. As I see it, we could live with this situation as long as the ramifications are known.

EDIT: The FE solution also has a problem since it is built with the librtlsdr-dev package installed on native Linux (?).This means that user must install the corresponding runtime package, without it the plugin won't load.

@bdbcat
Copy link
Collaborator

bdbcat commented May 26, 2022

I think the comment in the code is not instructive, or at least misleading. Might better say something like:
"Wait for second pass of cmake to find dependents, when the flatpak sandbox environment is fully available".

But anyway, the flag "OCPN_FLATPAK_CONFIG)" signals that this is the first run of cmake, setting up the environment. At this point in the flow, the sandbox is not available. So there is no possibility to get tinyxml from the SDK or OCPN core. So, it is skipped in this first pass.

Different devs, different styles...
Once again, nothing need be done to weatherfax_pi right now. Nor to FE2, for those plugins which currently use it. This discussion is more in the direction of "future-proofing" the templates.

@leamas
Copy link

leamas commented May 26, 2022

@bdbcat :

Once again, nothing need be done to weatherfax_pi right now.

Unless:

  • You want to be able to load the plugin on Linux without the libsdrtl package installed.
  • You want the librtlsdr support on MacOS
  • You wan a window for using librtlsdr also on Windows.

EDIT: but then again, the most important part here is to create a common understanding of the limitations of the current FE framework w r t build dependencies.

@bdbcat
Copy link
Collaborator

bdbcat commented May 26, 2022

Agreed. All about rtlsdr.
@rick: Practically speaking, is the user demand for OOB rtlsdr support high now?

@jongough
Copy link
Collaborator

It looks like a change is needed to CMakeLists.txt to add a test for OCPN_FLATPAK_BUILD and apply the libraries there and a change to PluginConfigure.cmake to handle setting up the flatpak plugin yaml file to handle the required libraries. May be one or two other changes to generalise the process. Am away at the moment so wont be able to do anything until at least Monday.

@rgleason
Copy link
Owner

rgleason commented May 27, 2022

Perhaps we should be clear about this and there should be two versions weatherfax and weatherfax-rtlsdr (for OS where it will work)? For the weatherfax-rtlsdr version a message to install additional drivers could come up.

To answer Dave's question more directly, I don't think rtlsdr is used that much.

I think the templates are getting much better. I particularly like the separation of opencpn-libs and libs. I wish I was skilled enough to improve all Sean's plugins to use the these improvements.

I will need to reread this closed issue several times. Thank you all.

@leamas
Copy link

leamas commented May 27, 2022

Let's wait for @jongough and see if he can generalize the FE template first. If this succeeds, it would be a good outcome of this discussion.

For the weatherfax-rtlsdr version a message to install additional drivers could come up.

Actually, it can't. For a message to come up the plugin must be loaded so it can display a message. But if it cannot be loaded (which is the situation on Linux today) it cannot execute anything, not even displaying a message.

there should be two versions weatherfax and weatherfax-rtlsdr

Different plugin versions IMHO probably just isn't worth it. If we can provide it with a reasonable effort, we probably should, otherwise not. Plugins with external dependencies should target power users which can recompile and import the plugin locally. This means a need for documentation, but that should be it.

Note that if Jon gets the FE template in shape we could just use the libraries in my PR and use them as-is also in FE. This would make the support for rtlsdr simple on Linux and MacOS, no reason to omit it.

I particularly like the separation of opencpn-libs and libs. I

This is also something that could be used in the FE template. The libraries are generic, it's just about adding the git submodule to the template, update the references and not least applicable docs.

This discussion could perhaps continue in #110

@rick
Copy link

rick commented Jun 11, 2022

tumblr_a5493b839982c9923122cc19e2e36014_4a161ae4_400

cc rick/you-rang#1

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

No branches or pull requests

6 participants