search

mauroc / squiddio_pi

squiddio_pi
3 stars 13 forks source link

Revert "Cmake flatpak" #91

Closed mauroc closed 4 years ago

mauroc commented 4 years ago

Reverts mauroc/squiddio_pi#88

got 3 errors in circleci

image

leamas commented 4 years ago

Yes, this needs to be reverted. Partly due the build failures caused by not creating pkg_version.sh. But also due to the fix which adds the linesudo dnf builddep -y ci/opencpn-fedora.spec to docker-build-flatpak.sh. This basically loads all dependencies required to build a fedora plugin. This is a major problem -- these dependencies are not used to build anything. See https://github.com/mauroc/squiddio_pi/issues/86#issuecomment-546598151

leamas commented 4 years ago

BTW: @mauroc: Did you forget to push the merge button here to actually perform the revert? Or, is the PR open to be reviewed?

jongough commented 4 years ago

Yes it does load all the dependencies as this allows cmake to run. If you don't want to do that then you need to get flatpak build to use cmake to load the dependencies you want and then use the makefile generated from cmake to do the build within flatpak. Without this there is just a mess of confusing and conflicting configurations. I am not saying this is the real solution but it is an attempt at moving forward. However, if these changes are not wanted then remove them. But unless this process gets greatly simplified it will not work. It is too difficult and too error prone to be used currently. This is at best an alpha release and at worst a proof of concept.

jongough commented 4 years ago

Why are you not using the 'buildsystem cmake' option of flatpak-builder? Surely this would help sort out some of the problems and if the requisite parts are not installed then get them installed. The compile requires wxWidgets and OpenGL or OCPN does not work. So not using cmake because it wants these packages seems very strange.

leamas commented 4 years ago

If you don't want to do that then you need to get flatpak build to use cmake to load the dependencies you want

No. Again: The libraries loaded by cmake (for example wxWidgets) are not used when building the flatpak plugin. The libraries used are loaded when flatpak installs the sdk and the main binary org.opencpn.OpenCPN. In the squiddio case (actually, most cases) these contains all required deps.

You could see this by looking at the current code. It builds the plugin successfully without loading any dependencies such as wxWidgets in the "normal" way. Also note that if there actually are dependencies (as in the oesenc example at https://github.com/leamas/oesenc_pi/blob/master/flatpak/org.opencpn.OpenCPN.Plugin.oesenc.yaml) these must be rebuilt from source in the sandbox rather than loaded as a regular dependency.

Which in the end raises the basic question: besides that the current code clutters CMakeLists.txt (it does, agreed), what exactly is the problem you are trying to solve?

jongough commented 4 years ago

I want to have one place to configure all the source trees, extra requirements, levels of widgets, etc. I do not want to have to configure 2 or more different build environments. It was painful enough making travis work, but once it did then everything was managed in cmake. When I change my plugin I don't want to have to remember to modify multiple files due to the builds being different. Up to the point of using flatpak once I had the secure keys and environments (travis, launchpad, appveyor) configured to talk I did not have to do anything else.

So, I don't care whether we use cmake or something else, I just want it to be one tool/config file where I make the changes and the builds all work it out themselves. My plugin has multiple source and include directories as well as requiring, for linux at least, special code that has to run before the build library is packaged. It was difficult enough getting it working in cmake without having to make it work in multiple other tools.

To help move the build/packaging process forward I want it to be as simple as possible, hence the single file to change. I want all the other files (config, cmake, shell scripts, yaml, xml, etc) to be configured by one tool, then most of the build/packaging code can be copy pasted into all the plugins without change. As all the current plugins use cmake it would seem sensible to continue to use this. The idea was to make CMakeLists.txt contain all the 'user' modifiable code and all the other stuff would be handled by the process. Hence the use, in cmake, of all the different variables and their use in the other config files.

leamas commented 4 years ago

Well, that's a good goal. But given that there actually are no dependencies to configure in the flatpak case, why mess with this particular code?

And to be honest: I see no way to configure possible flatpak dependencies in a central cmake setup. I'd suggest that you leave this the way it is (in flatpak/*yaml) and focus on other areas where I think your approach would be successful. I share your view that we need to consolidate builders and deploying stuff. I just don't think the flatpak parts are that important in this respect.

jongough commented 4 years ago

Because you have to hand create the makefiles for flatpak, or does your process create this? I have not found how to automate the creation of the makefiles for flatpak.

leamas commented 4 years ago

There is no reason to think the Makefile will change. In fact, it's generic and should be possible to use in any plugin besides a simple variable definition (just look at it and you'll understand). It's the same even in the oesenc case which is as messy as anything gets

In fact, it could be avoided by making cmake invoke flatpak-builder directly. But I'm fond of the simple interface the Makefile provides to the flatpak build process.

In short: there is no need to generate it, just use it as-is.

EDIT: And yes, contrary to other Makefiles generated by cmake, flatpak/Makefile is checked into the git tree.

jongough commented 4 years ago

So where, for flatpak, are the source and include directories defined? How do I provide post processing of the executable (lib) to change the OS/ABI? How do I configure wxWTranslateCatalog.h.in and version.h.in? I have not seen how any of this hangs together? I am new to it but....

leamas commented 4 years ago

So where, for flatpak, are the source and include directories defined

See below (configuration)

How do I provide post processing of the executable (lib) to change the OS/ABI.

You don't do that. A flatpak extension is tightly coupled to the main binary it is built against.

How do I configure wxWTranslateCatalog.h.in and version.h.in? I have not seen how any of this hangs together?

Inside the sandbox, where the plugin is built using cmake. You can see the cmake invocation in flatpak/*yaml. Is this is the missing piece for you?

leamas commented 4 years ago

Hm... I maybe should expand a little here...

How do I provide post processing of the executable (lib) to change the OS/ABI.

You don't do that. A flatpak extension is tightly coupled to the main binary it is built against.

And flatpak is OS/ABI agnostic, it's the very point. A flatpacked Opencpn can basically run on any linux distro.

How do I configure wxWTranslateCatalog.h.in and version.h.in? I have not seen how any of this hangs together?

Inside the sandbox, where the plugin is built using cmake. You can see the cmake invocation in flatpak/*yaml. Is this is the missing piece for you?

Also note that the plugin is not built from the git tree where it's invoked. It downloads it's own sources (as defined in *yaml, here from github) and builds from that. So, if you want to change anything this needs to be done by patches, as in the oesenc example (which has quite a few of them).

rgleason commented 4 years ago

Could cmake generate org.opencpn.OpenCPN.Plugin.squiddio.yaml ?

leamas commented 4 years ago

Of course. But for what purpose?

rgleason commented 4 years ago

Then Jon's goal and my hope of configuring from one place (CMakeLists.txt) might be improved. Later: Or perhaps use variables? Later: I suppose this is no different than having to configure appveyor.yml and travis.yml with an encrypted key. However it is somewhat hidden. Could be part of the instructions however.

leamas commented 4 years ago

IMHO, it just won't work in a meaningful way. If you look at org.opencpn.OpenCPN.Plugin.squiddio.yaml, what could be configured there in a meaningful way using cmake? OK, perhaps url and/or commit, but does it make sense to take this configuration out of it's context?

rgleason commented 4 years ago

Alec, I don't know that answer. I am just thinking of how to make it easier for PI developers to adopt and deploy this Plugin Manager system!

--I wasn't aware of the flatpak files that had to be configured until Jon started asking questions and making suggestions. You've done a great deal here that is valuable, we are in the process of learning.

leamas commented 4 years ago

Adding to above: the flatpak build does see CMakeLists.txt when it runs cmake in the sandbox. So, anything you do in CMakeLists.tx and friends will affect the build (as long as the build sees right version of it). But practically speaking, flatpak/*yaml isn't much to configure.

rgleason commented 4 years ago

Is there anything else requiring configuration by the PI Maintainer?

leamas commented 4 years ago

flatpak/org.opencpn.Opencpn.Plugin.squiddio.yaml # Needs config of filename, id, name, config opts, URL, commit

"Config" in the sense that they are likely to change? I'd say no. The exception is the commit. However, changing the commit is tightly related to the patches applied. Definining patches in cmake will just be a mess, and moving the definition of the commit to one place (CMakeLists) while having the patch definitions in the *yaml file is IMHO a recipe for disaster.

In short: it's a good goal to have as much as possible in CMakeLists. But the flatpak configuration is very low on the list of things you need to move to it. I'd suggest you just drop it for now, at least until you get better understanding of the flatpak build process.

jongough commented 4 years ago

So where, for flatpak, are the source and include directories defined

See below (configuration)

How do I provide post processing of the executable (lib) to change the OS/ABI.

You don't do that. A flatpak extension is tightly coupled to the main binary it is built against.

How do I configure wxWTranslateCatalog.h.in and version.h.in? I have not seen how any of this hangs together?

Inside the sandbox, where the plugin is built using cmake. You can see the cmake invocation in flatpak/*yaml. Is this is the missing piece for you?

This yaml file gives the OCPN_FLATPAK=ON flag so bypasses most of the cmake process. The build options that are being used are too simplistic, there need to be more, they could be added but why not use the ones in the PluginConfig.cmake? The yaml file also contains a git commit, this would then restrict the build to that commit not the most current one or a tagged one, unless this is not used. If it is not used why is it in the file? If it is used who/what is changing this to reflect the current git state?

I need to change the OS/ABI for linux builds as the gcc compiler makes bad decisions based on the say that std-c++ is used. There is no flag in for gcc which will allow the setting to remain stable. So unless OCPN is compiled with exactly the same includes I am using in ODraw we will get different OS/ABI's. I handle this in the OD CMakeLists.txt file with the following code

IF(UNIX AND NOT APPLE)
  IF(OD_JSON_SCHEMA_VALIDATOR)
    # Change OS/ABI to be UNIX - System V to make it run with OCPN
    SET(lib_name lib${PACKAGE_NAME}.so)
    MESSAGE(STATUS "lib-name: ${lib_name}")
    FIND_FILE(elfedit elfedit) 
    IF(NOT (${elfedit} STREQUAL "elfedit-NOTFOUND"))
      MESSAGE(STATUS "Will ensure OS/ABI is set to 0 to match OCPN")
      ADD_CUSTOM_COMMAND(
        TARGET ${PACKAGE_NAME}
        POST_BUILD
        WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
        DEPENDS ${PACKAGE_NAME}
        COMMENT " Running post build action on ${lib_name}."
        COMMAND sh -c 'elfedit --input-osabi=Linux --output-osabi=none ${lib_name} || (echo 0) '
      )
    ELSE(NOT (${elfedit} STREQUAL "elfedit-NOTFOUND"))
      MESSAGE(STATUS "Cannot correct OS/ABI to match OCPN if it is generated incorrectly. Package binutils needed")
    ENDIF(NOT (${elfedit} STREQUAL "elfedit-NOTFOUND"))
  ENDIF(OD_JSON_SCHEMA_VALIDATOR)
ENDIF(UNIX AND NOT APPLE)

This does need to be executed for ODraw.

leamas commented 4 years ago

but why not use the ones in the PluginConfig.cmake?

Because this is the very file which looks for libraries such as OpenGL and wxWidgets -- which not are used to build the (flatpak) plugin. You should not load those.

Change OS/ABI to be UNIX - System V to make it run with OCPN

I don't follow the details, but would definitely think twice before I did anything like this on the flatpak plugins. In particular, the flatpak plugin is (for better or worse) more tightly coupled to the main opencpn code than in the "normal" case, so to speak. FWIW my git instinct says that you shouldn't have to do anything like this.

The yaml file also contains a git commit, this would then restrict the build to that commit not the most current one or a tagged one

It's actually more complicated. flatpak-builder downloads fresh sources from the url in the yaml file and then uses the commit. You can specify a branch or a tag as well. However, to get reproducible builds you need to apply your changes as patches against the given commit as in the oesenc link above, acting as a downstream packager.

In the end, flatpak-builder can rebuild the plugin with nothing but the yaml file. It's a strange beast in this context, but there are IMHO good design decisions behind this.

jongough commented 4 years ago

The issue with OS/ABI is that to use JSON Draft 7 schemas which ODraw does needs the inclusion of the schema validation hpp file. Something in this causes gcc to change the default value to a linux specific value. Linux will not load a library with a different OS/ABI than the main module. As the value is determined by gcc at compile time and cannot be changed by any flag it has to be changed after the module is built, hence the use of 'elfedit'. Other compilers do not demonstrate this issue, so Visual Studio works fine. This issue is mentioned in a few places on the internet and I have contacted directly the people who built the hpp file, but there is no resolution except the one I am using. So this needs to be included, if needed, in any flatpak build. You can try on a clone of ODraw to implement your flatpak build and see if it works, but I don't have enough of an environment or understanding to try it out.

jongough commented 4 years ago

The current build processes, travis and appveyor, both get copies of the repository from git and build these in their respective docker environments, so flatpak should be nothing different to this. wxWidgets is required for the whole of OCPN, so it must be installed or the build will fail. If it is installed then cmake should be able to find it. This would suggest that wxWidgets is being installed after cmake is run but before the make process. Is this true?

jongough commented 4 years ago

Your suggestion of using patches would be a nightmare. Every change would need to be applied as a patch which would require modifying the yaml file each time. This is anything but automated and will definitely reduce the reliability of any plugin. This whole thing needs to be automated and the current branch (should be able to specify this easily) tip should be used, or tags used, but it should happen without user intervention. Both travis and appveyor manage this, why does flatpak not?

jongough commented 4 years ago

How well does flatpak work on micro systems, i.e. Raspberry Pi and other similar systems? Flatpak files are large and seem to take quite a bit of processing power to work. Yes it is a common complete environment, but...

leamas commented 4 years ago

How well does flatpak work on micro systems, i.e. Raspberry Pi and other similar systems

Not very well. This is the main reason I think we still need to make .deb packages -- raspberry and similar platforms could need something smaller.

That said, most Opencpn installations has a lot of (Gb) pf charts. But there are other reasons as wll to keep the .deb

leamas commented 4 years ago

Your suggestion of using patches would be a nightmare.

This is not a suggestion, this is how to work downstream with upstream sources. I assure it's a well established best practice in many contexts. That said, feel free to use the current git tree -- it is possible to do so (just set the git repo to ..) It's the kind of patches one could make if required. However, I don't see the need for now.

leamas commented 4 years ago

This would suggest that wxWidgets is being installed after cmake is run but before the make process. Is this true?

No. I'd suggest you just read the build logs.

BTW: contrary to normal plugins, flatpak plugins are linked against OpenCPN (since they are flatpak extensions, they consider OpenCPN as their runtime). This means that whatever OpenCPN is linked with is visible in the flatpak case.

leamas commented 4 years ago

The issue with OS/ABI is that to use JSON Draft 7 schemas which ODraw

But then again, this is about squiddio... Let's return to more complicated plugins when the issues in this one are settled.

rgleason commented 4 years ago

These lines will need to be changed to Plugin Installer enable a new plugin

Any others for flatpak?

rgleason commented 4 years ago

Alec, You mentioned combining your scripts into one script to simplify. I actually like the separate scripts because each one has a particular job, it is understandable, and certainly some of the "retired" scripts should remain somewhere (in my opinion), as the process gets refined, in case that job needs to be used again for some reason.

jongough commented 4 years ago

I thought this was about more than just squiddio, I thought this was about making a generalised installation process that could be used with all other plugins.

I can add a header file which will cause the OS/ABI to change for squiddio and then you can see if the flatpak linker will work and also if the generated flatpak will actually run.

The header is for using JSON Schemas (Draft 7) which should be used by all plugins particularly if we are going to allow multiple instances of OCPN to communicate via JSON of if we are going to allow other programs to send/receive OCPN JSON messages. I didn't implement this into Squiddio as I was trying it in my plugin first. It works on windows and Linux but macos (the build environment I have anyway) does not like something in the code so macos versions of ODraw do not use the schema.

leamas commented 4 years ago

I thought this was about making a generalised installation process that could be used with all other plugins.

Hold your horses.... IMHO, stuff like this is best handled in smaller steps.

It's of course possible to add any kind of post-processing also to flatpak, I don't see it as a problem. Other plugins might have other needs, who knows. Still, this seems IMHO specialized enough to be best handled on a case-by-case basis and thus fixed in the ODraw context.

jongough commented 4 years ago

So we should blindly go down and alley that may not work for all plugins? I would have thought we should ensure that slightly more complex situations are at least considered whilst building this first step. I don't want to have to support a 'constrained' plugin as well as the full featured one because we did not consider the future. I am trying to understand the benefits and costs in going forward with flatpak. At the moment there seem to be quite a few costs and not too many benefits. We already have installables on most platforms, so this is just a different packaging methodology. Flatpak may have advantages but these have to be weighed against the costs for support and complexity and I am not sure that has been done yet. By all means investigate it, but do so with open eyes and not just one narrow view.

If we are going to support flatpak it needs to be usable, easy to understand/setup/configure/support and flexible enough for all plugins to work with it. I think it is currently failing on a few of these. I am finding it very confusing, complicated and difficult to work with. I may be simple and not up to speed on all packaging ideas, but I believe in KISS (Keep It Simple Silly). So I may have some questions that an expert user may not, but that is the price of introducing something like this. I have already spent days trying to get my head around this process and I am still not much closer to understanding it. The output from the process and the information about it is not easy to understand (for me at least).

jongough commented 4 years ago

I have just run another test and I don't see where cmake is run to generate the flatpak build process. It does not appear to be running the CMakeLists.txt file as there are no output messages. So which cmake files are being process. Here is the last part of the log of the last run I did (took over an hour to run here): `FB: Running: git --version FB: Git version: 2.17.2.0 FB: Running: git cat-file -e d474c8ffa3c4addbaf724d55b1b464e894b1934d FB: Running: git ls-remote origin FB: Running: git config transfer.fsckObjects 1 Fetching full git repo https://github.com/mauroc/squiddio_pi.git FB: Running: git fetch -f -p --no-recurse-submodules --tags origin ':' From https://github.com/mauroc/squiddio_pi