rgleason / weatherfax_pi

Weatherfax Plugin for Opencpn
GNU General Public License v3.0
5 stars 9 forks source link

cmake flatpak #96

Closed ozolli closed 2 years ago

ozolli commented 2 years ago

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?

leamas commented 2 years ago

Sorry for the run-around.

No need, walking through this is good example for more complicated plugins. It's kind of interesting. Besides that it now builds, I see:

I might give this a shot later with the goal to sort this out.

rgleason commented 2 years ago

Thank you Jon, Dave and Alex, for the focused attention to get this working with flatpak. We of course would like to follow best practices with regard to PIM and if the plugin structure can be simplified a little that would be good. We would like to get libaudio and decoding working on all platforms if possible. Also Sean has a convert to Kap feature that is broken, which we would like to expand into an automatic save and read KAP because it incorporates georeferencing, so that georeferencing is not lost after closing the plugin.

Alex hope your boat is launched, we are about 4 days away!

rgleason commented 2 years ago

@bdbcat I think everything is building except Windows I ran a local build and found this. Don't have time to fix it today. Will try later.

C:/Users/fcgle/source/weatherfax_pi/libs/curl/cache/curl-7.83.1.tar.gz: No such file or directory
CMake Error at libs/curl/CMakeLists.txt:89 (add_subdirectory):
  add_subdirectory given source
  "C:/Users/fcgle/source/weatherfax_pi/libs/curl/cache/curl-7.83.1" which is   not an existing directory.
bdbcat commented 2 years ago

I'll take a look, probably send another PR. Meanwhile, the version in the catalogs is usable for testing, now. So no panic.

rgleason commented 2 years ago

Yes, agreed. I also notice that both macOS fail too. As an aside some other OS issues with the plugin https://github.com/OpenCPN/OpenCPN/discussions/2616#discussioncomment-2804895

leamas commented 2 years ago

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:

leamas commented 2 years ago

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 commented 2 years ago

No, Android won't fly. Leaving it unimplemented

bdbcat commented 2 years ago

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 commented 2 years ago

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

leamas commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

The Frontend workflow has drawbacks related to build dependencies:

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 commented 2 years ago

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 commented 2 years ago

@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:

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 commented 2 years ago

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 commented 2 years ago

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

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

rgleason commented 2 years ago

So Jon's efforts 6 days ago https://github.com/jongough/testplugin_pi/pull/263#event-6681654975 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 commented 2 years ago

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 commented 2 years ago

So Jon's efforts 6 days ago https://github.com/jongough/testplugin_pi/pull/263#event-6681654975 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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

@bdbcat :

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

Unless:

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 commented 2 years ago

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

jongough commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

tumblr_a5493b839982c9923122cc19e2e36014_4a161ae4_400

cc https://github.com/rick/you-rang/issues/1