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

[groovy] Remove bundled groovy/apache commons binaries #24199

Merged
merged 3 commits into from
Dec 11, 2023

Conversation

fuzzard
Copy link
Contributor

@fuzzard fuzzard commented Dec 7, 2023

Description

Update Groovy/Apache commons binaries, and dont ship in tree

Motivation and context

Remove binaries from git tree and download from mirrors (or from provided local/remote url)

The following defines (-D) can be set to point to existing extracted instances
(eg -Dgroovy_SOURCE_DIR=/usr/share/groovy)

groovy_SOURCE_DIR
apache-commons-lang_SOURCE_DIR
apache-commons-text_SOURCE_DIR

@wsnipex @basilgello Ive tried to implement a way for system maintainers to supply the groovy/apache-commons binaries as defines for offline install. Let me know if its not suitable or whatever.

The reasoning for all this is for a more streamlined way to bump binary files, rather than someone extracting and adding in tree and no real way to verify/validate easily.

How has this been tested?

Build macos aarch64

What is the effect on users?

N/A

Screenshots (if appropriate):

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • Clean up (non-breaking change which removes non-working, unmaintained functionality)
  • Improvement (non-breaking change which improves existing functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that will cause existing functionality to change)
  • Cosmetic change (non-breaking change that doesn't touch code)
  • Student submission (PR was done for educational purposes and will be treated as such)
  • None of the above (please explain below)

Checklist:

  • My code follows the Code Guidelines of this project
  • My change requires a change to the documentation, either Doxygen or wiki
  • I have updated the documentation accordingly
  • I have read the Contributing document
  • I have added tests to cover my change
  • All new and existing tests passed

@fuzzard fuzzard added Type: Improvement non-breaking change which improves existing functionality v21 Omega labels Dec 7, 2023
@fuzzard fuzzard requested a review from wsnipex December 7, 2023 11:01
@fuzzard fuzzard added this to the Omega 21.0 Beta 2 milestone Dec 7, 2023
@basilgello
Copy link
Collaborator

Debian still ships 2.4 so I revert Jose's patches there. Will check and report back.

@fuzzard
Copy link
Contributor Author

fuzzard commented Dec 8, 2023

Debian still ships 2.4 so I revert Jose's patches there. Will check and report back.

So this isnt swig as such, these are binaries used for the groovy code generator after the swig usage.

https://commons.apache.org/proper/commons-lang/download_lang.cgi
https://commons.apache.org/proper/commons-text/download_text.cgi
https://groovy.apache.org/download.html

for the relevant packages. No idea if the apache commons stuff will be packaged in the debian world or not, but i imagine groovy is at least

edit: i guess debian packages them already, saw commons-text here https://packages.debian.org/source/sid/commons-text

@fuzzard fuzzard changed the title [swig] Remove bundled groovy/apache commons binaries [groovy] Remove bundled groovy/apache commons binaries Dec 8, 2023
@fuzzard
Copy link
Contributor Author

fuzzard commented Dec 9, 2023

From a distro building perspective, whats better as a definable option @basilgello , pass the tarball, or set the file for each directly? None of it is needed past build, so i imagine its simpler as a tarball?

@wsnipex
Copy link
Member

wsnipex commented Dec 9, 2023

if binaries are available in the distro, using them is always preferred from a packaging perspective. Using tarballs means having to include them in the source package.

@fuzzard
Copy link
Contributor Author

fuzzard commented Dec 10, 2023

Ive had a rework of this. Instead of allowing redirection of the tarball, a user can just set folder for the existing package contents (eg predownloaded/extracted, or system packages).

-Dgroovy_SOURCE_DIR=/usr/share/groovy
-Dapache-commons-lang_SOURCE_DIR=/usr/share/java
-Dapache-commons-text_SOURCE_DIR=/usr/share/java

This is based off debian packaging such as https://packages.debian.org/bookworm/all/groovy/filelist https://packages.debian.org/bookworm/all/libcommons-lang-java/filelist

Theres no need to specify (or know) about the version of each of the 3 packages from a distro perspective any longer, however it does just do a blanket add to the classpath of the above *_SOURCE_DIR paths. I dont think theres going to be issues, but who knows.

Ive also set it up to respect existing extractions (from DEPENDS_PATH), and also to save any downloaded tarballs to TARBALL_DIR for caching of the download. This should give us the benefit of updating them easier, but also reduce redownload/extraction where appropriate.

Hopefully this gives enough flexibility from a distro packaging perspective @wsnipex @basilgello

@fuzzard
Copy link
Contributor Author

fuzzard commented Dec 10, 2023

One thing i did try to do was to use NATIVEPREFIX, as the packages arent arch specific (ie java), but also they are only executed on build host, however not all platforms set NATIVEPREFIX (ie linux) so ive just gone with DEPENDS_PATH for now.

@wsnipex
Copy link
Member

wsnipex commented Dec 10, 2023

Looks good, but needs KODI_MIRROR to be defined for downloading when *_SOURCE_DIR isn't set (on non-depends builds)

@fuzzard
Copy link
Contributor Author

fuzzard commented Dec 10, 2023

God damn addon cmake gets me again, haha.

# in case we need to download something, set KODI_MIRROR to the default if not already set
if(NOT DEFINED KODI_MIRROR)
set(KODI_MIRROR "http://mirrors.kodi.tv")
endif()

Its probably a good thing to add that to core as well i guess rather than hardcoding it

@basilgello
Copy link
Collaborator

Yup, better to vore it and make overridable if needed

fuzzard added 3 commits December 10, 2023 17:56
Allows user to override (eg specific mirror/source preferred due to mirrors giving a
faulty redirect)
Remove binaries from git tree and download from mirrors (or from provided local/remote url)

The following defines (-D) can be set to point to existing extracted instances
(eg -Dgroovy_SOURCE_DIR=/usr/share/groovy)

groovy_SOURCE_DIR
apache-commons-lang_SOURCE_DIR
apache-commons-text_SOURCE_DIR
@fuzzard
Copy link
Contributor Author

fuzzard commented Dec 10, 2023

Haved added KODI_MIRROR definition as a commit. I think it should rightly be available fairly early on, and couldnt think of a better included location, so have just added to root CMakeLists.txt.

As i would also just forget, ive changed the 2 locations where it would be suitable in core in this PR. One in an externalproject_add macro for dependencies, and the other is the setting of KODI_MIRROR in the patch find module. Its no longer needed, as KODI_MIRROR is now defined in addon PrepareEnv.cmake and core appropriately.

A todo is to potentially propagate KODI_MIRROR supplied to windows build scripts into the cmake project, but thats one i dont really care about right now, and if i dont remember it later, no harm.

@wsnipex
Copy link
Member

wsnipex commented Dec 10, 2023

God damn addon cmake gets me again, haha.

# in case we need to download something, set KODI_MIRROR to the default if not already set
if(NOT DEFINED KODI_MIRROR)
set(KODI_MIRROR "http://mirrors.kodi.tv")
endif()

Its probably a good thing to add that to core as well i guess rather than hardcoding it

this is too late. We have to move this earlier in the chain. Probably best to move it to CMakeLists as an option and remove the various instances where this is set again.

edit: sorry, you did that already :)

@fuzzard fuzzard merged commit 1bf0f7b into xbmc:master Dec 11, 2023
2 checks passed
@fuzzard fuzzard deleted the swig_binaries branch December 11, 2023 05:54
bkuhls added a commit to bkuhls/buildroot that referenced this pull request Jan 4, 2024
Upstream removed the bundled groovy/apache commons binaries:
xbmc/xbmc@d6bc920
xbmc/xbmc#24199
bkuhls added a commit to bkuhls/buildroot that referenced this pull request Jan 5, 2024
Upstream removed the bundled groovy/apache commons binaries:
xbmc/xbmc@d6bc920
xbmc/xbmc#24199
bkuhls added a commit to bkuhls/buildroot that referenced this pull request Jan 17, 2024
Upstream removed the bundled groovy/apache commons binaries:
xbmc/xbmc@d6bc920
xbmc/xbmc#24199
bkuhls added a commit to bkuhls/buildroot that referenced this pull request Feb 10, 2024
Upstream removed the bundled groovy/apache commons binaries:
xbmc/xbmc@d6bc920
xbmc/xbmc#24199
bkuhls added a commit to bkuhls/buildroot that referenced this pull request Feb 16, 2024
Added new dependency to libdisplay-info for gbm support:
xbmc/xbmc@ce96264

Added new required dependency to tinyxml2:
xbmc/xbmc@9e983ed

Added new required dependency to libudfread: Since upstream commit
xbmc/xbmc@5f9b9cf
kodi does not build anymore without libudfread.

Upstream removed the bundled groovy/apache commons binaries:
xbmc/xbmc@d6bc920
xbmc/xbmc#24199

- JsonSchemaBuilder fixes:

Upstream moved CMakeLists.txt to src/ subfolder:
xbmc/xbmc@7e87d98

- TexturePacker fixes:

texturepacker now depends on c++17:
xbmc/xbmc@54bd6d7

Upstream moved CMakeLists.txt to src/ subfolder
xbmc/xbmc@e336a75

Due to this update we can remove all of our patches for texturepacker.

Set KODI_SOURCE_DIR variable to root directory of the source tarball,
LibreELEC added something similar:
LibreELEC/LibreELEC.tv@70abdd2
bkuhls added a commit to bkuhls/buildroot that referenced this pull request Feb 27, 2024
Added new dependency to libdisplay-info for gbm support:
xbmc/xbmc@ce96264

Added new required dependency to tinyxml2:
xbmc/xbmc@9e983ed

Added new required dependency to libudfread: Since upstream commit
xbmc/xbmc@5f9b9cf
kodi does not build anymore without libudfread.

Upstream removed the bundled groovy/apache commons binaries:
xbmc/xbmc@d6bc920
xbmc/xbmc#24199

- JsonSchemaBuilder fixes:

Upstream moved CMakeLists.txt to src/ subfolder:
xbmc/xbmc@7e87d98

- TexturePacker fixes:

texturepacker now depends on c++17:
xbmc/xbmc@54bd6d7

Upstream moved CMakeLists.txt to src/ subfolder
xbmc/xbmc@e336a75

Due to this update we can remove all of our patches for texturepacker.

Set KODI_SOURCE_DIR variable to root directory of the source tarball,
LibreELEC added something similar:
LibreELEC/LibreELEC.tv@70abdd2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Build Type: Improvement non-breaking change which improves existing functionality v21 Omega
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants