jongough / testplugin_pi

Pluing to test JSON and ODAPI
GNU General Public License v3.0
3 stars 9 forks source link

TP Convert to 'opencpn-libs' submodule #332

Closed rgleason closed 11 months ago

rgleason commented 1 year ago

Convert to opencpn-libs submodule


  1. Create a new branch called "sublibs" (locally)

    $ git checkout -b sublibs
  2. In root directory of plugin, do:

     $ git submodule add -b devel https://github.com/leamas/opencpn-libs.git

    1.1. This adds the "devel" branch of leamas' opencpn-libs repo to the plugin project as a submodule. 1.2. It also immediately downloads opencpn-libs, and places it in the plugin project root directory. 1.3 This needs only to be done ONCE. 1.4. Also, DO NOT add the raw contents of opencpn-libs to the plugin project git tree. 1.5. Submodules use only a reference to the github resident version of the submodule.

    Do not edit the copy of opencpn-libs as pulled to local plugin root.  Treat as READ-ONLY.
    
    $ git commit -m "Adding opencpn-libs submodule"
    $ git push origin sublibs
    Commit and push the submodule definition.
  3. Edit CMakeLists.txt in plugin project to change all instances of "libs/x" to "opencpn-libs/x"

    Use the syntax shown in watchdog_pi for adding directories from the opencpn-libs submodule.

    You may test build locally from the plugin project root as normal. When satisfies with local builds, got to #3.

    Review the CMakeLists.txt changes made in Watchdog Weatherfax, Weather_routing, Testplugin for submodule changes including:

  1. Copy entire folder "ci" from watchdog_pi to plugin root directory, verbatim. This adds the required sublibrary command to all build scripts.

Build Locally

For your local builds, after doing:

 Delete opencpn-libs,and then issue:
$ git submodule update --init opencpn-libs
$ git submodule update --remote --merge opencpn-libs
Commit and push the results.
This command pulls in all the latest updates to the opencpn-libs module.
This will fix all builds except flatpak.

This is exactly what the CI process does. It will bring in a fresh copy of opencpn-libs from github. It does not change the git repo contents at all, since you already have the git reference to opencpn-libs.

You generally need to only do this once (for each plugin), unless opencpn-libs is changed by me or Alec. We will let you know if that happens.

Remember: treat opencpn-libs as a read-only library. Do not edit. Do not commit contents.

Dave

More rules about submodules

  1. There is only one true opencpn-libs, and it lives in the Alec's opencpn-libs repo.
  2. All plugins must use the same golden opencpn-libs. That is the objective of this "sublibs" project.
  3. To fetch the most current opencpn-libs, just do git submodule update --init opencpn-libs
  4. It is always safe and good practice to do "submodule update" whenever you start working on a project.
  5. Do not ever edit or change opencpn-libs locally. If you do, then the next "submodule update...." will fail.
  6. If you find that opencpn-libs differs in two projects locally, it means that one or both need "submodule update...."

If you follow these rules locally, then you will be following exactly the same process as CCI does in its scripts. Remember, CCI is the "golden build", from which production plugins are released.

I'm sure you know all this, so just reminders.

Regarding Watchdog opencpn-libs Issue

from Dave More research, working on the flatpak builds. You need to do this for each plugin:

  1. In ci directory, add this line to build scripts after each submodule update.

    git submodule update --init opencpn-libs git submodule update --remote --merge opencpn-libs <-- ADD THIS.

Then commit and push the results. This will fix all builds except flatpak.

  1. To fix failing flatpak builds, for each plugin that you have IN PROCESS that fails flatpak on CCI, from source root, do this:

    $ cd opencpn-libs $ git checkout devel $ git pull origin devel $ cd .. $ git add opencpn-libs $ git commit -m "Updating the submodule 'opencpn-libs' to the latest version" $ git push origin sublibs

For plugin that you have not started yet, you only need to do this, as described in the adaptation guide.:

$ git submodule add -b devel https://github.com/leamas/opencpn-libs.git

Lets confirm this on watchdog, and then move on.

Changes to opencpn-libs (notice from Dave or Alec)

When the change lands in opencpn-libs main branch, you will need to (for these two plugins only): $ git submodule update --remote --merge opencpn-libs $ git add opencpn-libs $ git commit -m "Update opencpn-libs submodule" $ git push origin master (or other branch as you decide)

jongough commented 1 year ago

If all plugins are going to use opencpn-libs shouldn't these really be available from the opencpn git repository? It may well be a good idea to do this, as you document above, I am just not convinced it should be in a non-opencpn git.

Also, this whole process should allow different versions of the libs to be used depending on which version of OCPN that the plugins are being built for. I have found with the current version of OCPN 5.8.4 on a Pi 3B+ using binary signalK that the resource requirements are on the limit when following a route. The OCPN 5.6 did not have this issue. Trying to use the character version of signalK does not seem to 'turn off' the processing of digital signalK so is worse. I will investigate this when I get back to my dev machines.

rgleason commented 1 year ago

Thank you Jon. I am sure Dave will consider these suggestions. I need to send him this thread.

rgleason commented 1 year ago

Keep in mind that the plugin uses the opencpn-libs that are needed and the opencpn-libs folder that is on github is kind of virtual, or empty, just referring to the leams location.

However I am finding that there is a downside in terms of disk usage for devs (about 87mb per plugin). I hope there is a more efficient way or process available. For 20+ plugins that is about 1.8gb for just one or two plugins it is not so bad.

I could simply delete the local repos and git clone it when needed, but that takes some time and bandwidth. Alternatively I could just ignore the problem and continue as I have.

Dave wrote:

You may automate this by script, as you suggest. Just delete opencpn-libs, and git submodule init and update when you want to actually work on a plugin.

Since I will have about 77mb of opencpn-libs x 20+ plugins on my local machine, one way to save space is to simply delete the opencpn-libs folder after pushing the commits and then later when the plugin needs to be used again, execute the two commands

$ git submodule update --init opencpn-libs
$ git submodule update --remote --merge opencpn-libs

to recreate opencpn-libs. Of course this will require downloads of 77mb.

I also asked Dave if he knew if I could just copy the opencpn-libs directory to the next plugin. He said it was not good practice but it could be done. I think the problem with this is when opencpn-libs is updated the updates are not caught. Also the .git directory has some information about opencpn-libs and I don't know enough about that yet.

rgleason commented 1 year ago

I've just checked my source/shipdriver_pi, vdr_pi and logbook_pi local repositories and they simply have a directory called "libs" with the various sub-directories for what I think is only the libs that are needed by the plugin. Mike advises that I need to remove the entire folder and clone again, because this is an old version of Shipdriver. See https://github.com/Rasbats/shipdriver_pi/issues/560#issue-1905105562

On the other hand, I think I need to implement the opencpn-libs folder in Vdr_pi and LogbookKonni_pi because they are still using the older "libs" directory. ..also as Dave notes, they might have "private" libs, so beware when making this change.

Also I see that my remote has opencpn-libs submodule https://github.com/rgleason/shipdriver_pi This is the same for mike's repos https://github.com/Rasbats/shipdriver_pi

I wonder if this is the way the TP plugins are supposed to work? Or should I be updating my local Shipdriver plugins???

It might be better if the plugin just downloaded what was needed in the way of libs or opencpn-libs but that may not be possible.

bdbcat commented 1 year ago

"they simply have a directory called "libs" with the various sub-directories for what I think is only the libs that are needed by the plugin."

Hmmm. My shipdriver_pi contains only submodule reference. I do not see libs in Rasbat's git repo. Must be some old cruft ?

However, if present, these "libs" directories might contain extra source files that are private to the plugin build, That is, they are not used by any other plugin, so don't really belong in the common opencpn-libs.

bdbcat commented 1 year ago

@jongough re: "Also, this whole process should allow different versions of the libs to be used depending on which version of OCPN that the plugins are being built for. I have found with the current version of OCPN 5.8.4 on a Pi 3B+ using binary signalK that the resource requirements are on the limit when following a route."

Not sure I understand your concern here. Correct me if I am wrong, but I think opencpn-libs has no direct support for signalK, at present. Maybe it should. It does, however, have a useful wxJSON library.

rgleason commented 12 months ago

Dave, I asked mike about the local version having libs directories. He says that is an old clone, delete the local dir and clone it again. https://github.com/Rasbats/shipdriver_pi/issues/560#issue-1905105562

So my question re shipdriver plugins was answered. He agrees with you, shipdriver, vdr & logbook should have opencpn-libs directory, not "libs". This conforms to the way the sublibs branch of weatherfax, watchdog, weather_routing and testplugin are working.

rgleason commented 12 months ago

Prevent Appveyor from building. I believe there are a number of ways to do this. I would like to be able to put "build: false" at the top of the appveyor.yml file but that does not appear to be available.

Plugin github > settings > webhooks > Delete then Save --- Works. but but then it has to be setup again. Plugin github > settings > webhooks > Disable then Save --- Works.
Appveyor Branches conditional -- Tried branch: only: -(blank) and format was wrong.

rgleason commented 11 months ago

Examples of typical fixes

Findit_pi Correct Android Build Errors

Autopilot_route_pi Correct Debian Build Errors

Plots_pi Fixes macos, debian, flatpak ubuntu about 10 builds failing https://github.com/rgleason/plots_pi/commit/f9be1c47827c05539ef247712fc2d86ac5a57c7f

First do this in cmakelists.txt

 set(CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake")
set(CMAKE_POSITION_INDEPENDENT_CODE ON)

First # try it, then do static

#   add_subdirectory(opencpn-libs/glu)
#    target_link_libraries(${PACKAGE_NAME} ocpn::glu)
   add_subdirectory(opencpn-libs/glu)
    target_link_libraries(${PACKAGE_NAME} ocpn::glu_static)

Celestial_navigation Correct Android and MacOS builds #18

Deviation Correct Android build

Deviation Correct all builds

Pypilot More good examples Enable fPIC, Add wxServDisc to main build dir avoiding linkage problems

jongough commented 11 months ago

There is now a disconnect between WeatherRouting_pi on github with opencpn-libs and what is actually in opencpn-libs. The version within WeatherRouting_pi has opencpn-libs/odapi but in the real opencpn-libs it apears to be opencpn-libs/ODAPI. This may not matter on windows, but on linux it is case sensitive.

jongough commented 11 months ago

I seem to be fighting this process as the opencpn-libs stuff seems to be under active development which is invalidating some of my pushes. I would have hoped there would only be additions now not name/case changes.

jongough commented 11 months ago

ODraw has an updated version of pidc with more methods for handling the stuff that OD needs. Is it worth picking this version up and making it the pidc version? The OD version is based on the pidc one originally.

rgleason commented 11 months ago

Jon, Dave is working on this now!!! Don't fight it. Just stop until they are finished.

rgleason commented 11 months ago

Email Dave about this, he is pretty busy.

ODraw has an updated version of pidc with more methods for handling the stuff that OD needs. Is it worth picking this version up and making it the pidc version? The OD version is based on the pidc one originally.

rgleason commented 11 months ago

Closing, moved to Process and Examples.

bdbcat commented 11 months ago

"ODraw has an updated version of pidc with more methods for handling the stuff that OD needs. Is it worth picking this version up and making it the pidc version? The OD version is based on the pidc one originally."

I was waiting for your return to address this I do think it would be a good idea to pick up your extended pidc class in opencpn-libs.

As mentioned elsewhere, you may now make this change yourself in the opencpn-libs "devel" branch, test it, and request a merge to branch "main" when ready, thus bringing it into production.

Now would be an ideal time to make that update. Thanks Dave

rgleason commented 11 months ago

@jongough I hope you saw bdbcat's post as I had closed this!

jongough commented 11 months ago

Yep, working on the OpenCPN/opencpn-libs now.

jongough commented 11 months ago

I now have a version up on OpenCPN/opencpn-libs with a modified pidc.cpp/h in a new branch, add_oddc_to_pidc, branched from devel.

I now need to check if I have caused issues for other users with these changes. The Android and Flatpak builds seem to be failing. Not sure if this is my change or is something that has not been fixed yet. The runable code should be in the alpha cloudsmith ODraw, but I have not created the plugin xml update yet (it should be installable via downloading the file and doing a local install).

bdbcat commented 11 months ago

The Android builds fail due to undefined GL constant, not available in GLES2 model. Should be easy fix. Not sure about flatpak. I just tried a build of watchdog_pi against OpenCPN/opencpn-libs - main branch. It completed without error.

bdbcat commented 11 months ago

Jon.. I have merged the Alpha build. Are you content with the patches made in your new branch? I'd like to merge it up to main as soon as reasonable, to allow Rick to proceed with all TP plugin rebuilds. Thanks

jongough commented 11 months ago

Dave, The builds all work, the ones in the list you and Rick sorted out, and I have done a local check on ubuntu. I would like a check on windows, android and macos if possible before adding it into main. If you want to put it into devel I can do that as well, but need to merge my 'add_oddc_to_pidc' into devel and update my submodule to point to that level.

bdbcat commented 11 months ago

Jon... Please merge and push your new branch into devel now. That will make the workflow somewhat easier now. Rick and I can then do a remote update from devel and get the new code immediately. I'll do some testing, read some code diffs, etc. Maybe we can target a day or two of study before agreeing to merge into main branch. Thanks

jongough commented 11 months ago

I did the merge, but something is slightly different as the macos builds are now failing. I did notice that someone did some work on the need for OpenGL/gl.h and OpenGL/glu.h a few weeks back. If anyone knows what happened to that work it would be useful. My original add_oddc_to_pidc did build for all environments, but now it is not. (https://app.circleci.com/pipelines/github/jongough/ocpn_draw_pi/545). I am investigating but the submodule process is rather convoluted when the submodule needs to change branch, particularly when doing local builds as well.

Sorry for the mucking up of devel.

rgleason commented 11 months ago

Jongough's repos seems to be building now. https://app.circleci.com/pipelines/github/jongough/ocpn_draw_pi?branch=sublibs

bdbcat commented 11 months ago

Yes. You may like to get a head start on the small adaptations needed on TP plugins. For local builds, do:

  1. $ git submodule update --remote --merge opencpn-libs
  2. For each plugin that uses ODAPI, change the line:

add_subdirectory(opencpn-libs/ODAPI)

to

add_subdirectory(opencpn-libs/odapi)

  1. From ci directory, all files, remove all lines where present:

git submodule update --remote --merge opencpn-libs

  1. After opencpn-libs branch "devel" is merged to "main, there will be a set of git commands required to update each project's submodule access. I'll share the code for that when "main" is ready.
rgleason commented 11 months ago

Dave, I can do that today. I have a question about these two commits in weather_routing sublibs branch that look like they were done by Jon 3 days ago:

  1. Update to use ODAPI on github and use WinndowsHeaders for MSVC build only https://github.com/rgleason/weather_routing_pi/commit/83d38686766c6d49e842db299b5d352fc041e36b

  2. Update to only include WindowsHeaders when doing MSVC build Change case of ODAPI to odapi to match what is in opencpn-src https://github.com/rgleason/weather_routing_pi/commit/83f3fc9753e57a65a9ca622ec40d88a5cf61dfc5

There is some overlap in that change ODAPI to lower case, but should I also make the other changes as done in the commit? I particular the conditional for windowheaders?

It would help to have a clarification before starting this process.

Also, should I push the changes up to my github repos sublibs? or just keep the commit local for the time being?

bdbcat commented 11 months ago

The update for odapi is good, I actually did the windowsheaders change in PluginConfigure.cmake, around line 496.

   if(USE_LOCAL_GLU)
        message(STATUS "${CMLOC}    Adding local GLU")
        if (WIN32)
            add_subdirectory(opencpn-libs/WindowsHeaders)
            target_link_libraries(${PACKAGE_NAME} windows::headers)
        endif(WIN32)

        add_subdirectory(opencpn-libs/glu)
        message(STATUS "${CMLOC}PACKAGE_NAME: ${PACKAGE_NAME}")
        target_link_libraries(${PACKAGE_NAME} ocpn::glu_static)
        add_definitions(-DocpnUSE_GL)
        message(STATUS "${CMLOC}    Revised GL Lib (with local): " ${OPENGL_LIBRARIES})

This seems cleaner to me, since it becomes part of the common template instead of repeated for every plugin CMakeLists.txt.

I would keep this all local for the time being. If you push them, the CCI builds will fail. No need for that confusion.

jongough commented 11 months ago

Which testplugin repository did you make this change too? I have pulled Ricks version and applied it to mine to get it up to date. There is one change that is needed to make the built macos wx315 work with the xml, which is to not put the wx version into the target name. A one line change to PluginSetup.cmake.

bdbcat commented 11 months ago

I did not push it anywhere. Just copy my 4 line code snip above, into the correct place in PluginConfigure.cmake, leave it out of CMakeLists.txt, and all will be well. Or wait for Rick to update his forks, and re-pull. Verify by local build, since this is a MSVC specific change.

jongough commented 11 months ago

I believe my testplugin_pi sublibs branch is now up to date.

rgleason commented 11 months ago

Yes, I've just finished this list of changes " Last steps: before final submodule reload #343 " which is in Watchdog sublibs branch
and I did push these changes after setting workflow_deploy to false to see what happens with the builds. A lot of the builds did work. Some did not.

Msvc-wx32 did not build https://app.circleci.com/pipelines/github/rgleason/watchdog_pi/413/workflows/6e3293a0-b161-4ad1-84ec-9777264665ea/jobs/5442

Msvc did not build https://app.circleci.com/pipelines/github/rgleason/watchdog_pi/413/workflows/6e3293a0-b161-4ad1-84ec-9777264665ea/jobs/5441

I want to be sure I have this right before completing the change. In particular the changes to windowsheaders. Dave can you check the changes I made?

Do these commit changes look right? https://github.com/rgleason/watchdog_pi/commit/e974d511b2a61f82fb339236b28d0ab79886e0ef

rgleason commented 11 months ago

Jon spotted this possible fix for Apple https://github.com/jongough/testplugin_pi/commit/f9f45b5b6d18aba46f1a2fe03f62597b586198b8

Dave, should I make that change too?

bdbcat commented 11 months ago

Rick... None of the builds succeeded, since the opencpn-libs submodule was never loaded.

  1. You should not remove git submodule update --init opencpn-libs from the ci files.

what I said was: From ci directory, all files, remove all lines where present: git submodule update --remote --merge opencpn-libs See the difference?

  1. You should adopt Jon's https://github.com/jongough/testplugin_pi/commit/f9f45b5b6d18aba46f1a2fe03f62597b586198b8
  2. Otherwise, OK.
rgleason commented 11 months ago

I put the git submodule update --init opencpn-libs back There are no git submodule update --remote --merge opencpn-libs in the ci files.

Have made Jon's change. Thanks. will start tomorrow.

bdbcat commented 11 months ago

OK, great. Have not heard from Alec about "main" merge yet. We shall see tomorrow. Thanks, and sorry for the extra work.

rgleason commented 11 months ago

Dave, now getting a number of these errors

CMake Error at cmake/PluginPackage.cmake:175 (endif):
  endif An ENDIF command was found outside of a proper IF ENDIF structure.
  Or its arguments did not match the opening IF command.
Call Stack (most recent call first):
CMake Error at cmake/PluginPackage.cmake:175 (endif):
  Flow control statements are not properly nested.
Call Stack (most recent call first):
  CMakeLists.txt:261 (include)

See this recent push https://github.com/rgleason/watchdog_pi/commits/sublibs The bottom shows cmake/pluginPackage.cmake https://github.com/rgleason/watchdog_pi/commit/95c50247e8676ae579ebf70765302095e23e0603

What did I do wrong here?

as compared to what Jon had https://github.com/jongough/testplugin_pi/commit/f9f45b5b6d18aba46f1a2fe03f62597b586198b8

rgleason commented 11 months ago

Oh it is # endif(APPLE)

bdbcat commented 11 months ago

Yep.

jongough commented 11 months ago

Probably my bad as I commented out the 'APPLE' and added the 'FALSE' as I was testing at the time to try and workout the issue with macos builds.

rgleason commented 11 months ago

It seems to be working now. Thanks all!

https://github.com/rgleason/watchdog_pi/tree/sublibs

rgleason commented 11 months ago

I have a question about Climatology_pi CMakeLists.txt

Screenshot (1828)

Can this be simplified? Perhaps this is already done in the pluginxxxx.cmake files? if(QT_ANDROID) add_subdirectory(opencpn-libs/glu) target_link_libraries(${PACKAGE_NAME} ocpn::glu_static) endif()

and maybe there is a better way to express the private zlib code?

I haven't pushed these changes up, but the current commit in rgleason/climatology_pi https://github.com/rgleason/climatology_pi/blob/sublibs/CMakeLists.txt shows the code around line 228

Thanks

rgleason commented 11 months ago

These "final TP chngs" have been completed https://github.com/jongough/testplugin_pi/issues/177 with the exception of the question above about climatology_pi

bdbcat commented 11 months ago

Rick... Not really possible to simplify.

  1. Most plugins do not actually need glu_static. Indeed, climatology and oDraw may be the only ones that truly need it. So if Android needs it, it must be installed explicitly.
  2. Same for zlib. Only one plugin, climatology, needs it, I think. Used to unzip the downloaded climate data.

So these kinds of "on-off" dependencies belong in the CMakeLists.txt file, however complicated that appears.

Dave

rgleason commented 11 months ago

Thanks Dave. Everything is so clean, I thought perhaps there was a way, but this is perfectly ok as it is.

bdbcat commented 11 months ago

Rick... opencpn-libs master is now inplace. Time to adapt TP plugins to use it.

For each plugin, do this:

A. Remove the current opencpn-libs submodule linkage.

    $ git submodule deinit -f opencpn-libs
    $ git rm --cached opencpn-libs
    $ rm -rf .git/modules/opencpn-libs  (on Windows, simply delete the target directory ".git/modules/opencpn-libs" )
    $ rm -rf opencpn-libs  (on Windows, simply delete the target directory "opencpn-libs" )
    $ git config -f .gitmodules --remove-section submodule.opencpn-libs
    $ git add .gitmodules
    $ git commit -m "Remove opencpn-libs submodule."

B. Add the new linkage

    $ git submodule add https://github.com/OpenCPN/opencpn-libs.git
    $ git commit -m "Adding revised opencpn-libs submodule main"

C. Push all $ git push origin sublibs

Let's do one plugin fully, and verify the process. Maybe testplugin_pi?

rgleason commented 11 months ago

Made a batch file link-rm.bat

REM Remove the current opencpn-libs submodule linkage.
    git submodule deinit -f opencpn-libs
    git rm --cached opencpn-libs
    rmdir /S .\.git\modules\opencpn-libs 
    rmdir /S .\opencpn-libs
    git config -f .gitmodules --remove-section submodule.opencpn-libs
    git add .gitmodules
    git commit -m "Remove opencpn-libs submodule."
REM opencpn-libs linkage and files removed and  commit made.

REM Add the new linkage
   git submodule add https://github.com/OpenCPN/opencpn-libs.git
   git commit -m "Adding revised opencpn-libs submodule main"
REM Added new linkage to module opencpn-libs main and made commit.
   git log --oneline

Made link-add.bat and (at bottom of link-rm.bat)

REM Add the new linkage
git submodule add https://github.com/OpenCPN/opencpn-libs.git
git commit -m "Adding revised opencpn-libs submodule main"
REM Added new linkage to module opencpn-libs main and made commit.
git log --oneline
rgleason commented 11 months ago

After using

     git submodule deinit -f opencpn-libs
     git rm --cached opencpn-libs
     rmdir /S .\.git\modules\opencpn-libs 
     rmdir /S .\opencpn-libs

When I

    C:\Users\fcgle\source\testplugin_pi>git config -f .gitmodules --remove-section submodule.opencpn-libs
   fatal: no such section: submodule.opencpn-libs

Haven't I already removed it with

rmdir /S .\.git\modules\opencpn-libs ??

rgleason commented 11 months ago

Isn't this a less brutal way to simply remove submodule.opencpn-libs? without using the rmdir command?
This would be best for a plugin like climatology which has a data submodule that we don't want to screw up.

$ git config -f .gitmodules --remove-section submodule.opencpn-libs

rgleason commented 11 months ago

Dave I've done testplugin and pushed it. I've gone through these commands and think one of them is redundant.

REM Remove the current opencpn-libs submodule linkage.
    git submodule deinit -f opencpn-libs
    git rm --cached opencpn-libs
    rmdir /S .\.git\modules\opencpn-libs    <--- This one or the one below
    rmdir /S .\opencpn-libs
    git config -f .gitmodules --remove-section submodule.opencpn-libs  <--- OR this one.
    git add .gitmodules
    git commit -m "Remove opencpn-libs submodule."
REM opencpn-libs linkage and files removed and  commit made