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

py311-310: Update to latest version #5866

Merged
merged 27 commits into from
Sep 6, 2023

Conversation

th0ma7
Copy link
Contributor

@th0ma7 th0ma7 commented Sep 2, 2023

Description

Update to Python 3.10.13 and 3.11.5.

Fixes #5855, fixes #5841, fixes #5857 and possibly #5847

Notes:

  1. PyYaml is broken with Cython >= 3.x "AttributeError: cython_sources" with Cython 3.0.0a10 yaml/pyyaml#601 thus keep using older version of Cython 0.29.x for the crossenv creation
  2. Deprecating python 3.11 armv5 support due to missing libatomic (ref: Migrate all packages to openssl3 and python 3.11 #5820 (comment) and sabnzbd not starting after upgrade sabnzbd to version 4.0.2-62 and Python to version 3.11.4-6 on my NAS DS212+ #5841)
  3. Once again able to migrate to newer setuptools as issue with nunmpy now resolved [BUG] breaks numpy in cross-compiling environment pypa/setuptools#3549

Checklist

  • Build rule all-supported completed successfully
  • New installation of package completed successfully
  • Package upgrade completed successfully (Manually install the package again)
  • Package functionality was tested
  • Any needed documentation is updated/created

Type of change

  • Bug fix
  • New Package
  • Package update
  • Includes small framework changes
  • This change requires a documentation update (e.g. Wiki)

@th0ma7 th0ma7 self-assigned this Sep 2, 2023
@th0ma7 th0ma7 requested a review from hgy59 September 2, 2023 17:15
@hgy59
Copy link
Contributor

hgy59 commented Sep 3, 2023

@th0ma7 thanks a lot for fixing this. I am longing for this to be merged for #5857

@th0ma7
Copy link
Contributor Author

th0ma7 commented Sep 3, 2023

thanks a lot for fixing this. I am longing for this to be merged for #5857

@hgy59 Always a pleasure. Note that I am temporary marking homeassistant as broken. You'll have to revert that once you rebase against master.

EDIT: I finally had to revert upgrading setuptools so numpy's version specific to homeassistant can build properly. Therefore BROKEN now removed.

@th0ma7
Copy link
Contributor Author

th0ma7 commented Sep 3, 2023

@hgy59 everything should now be solved and build to complete sucessfully. Last thing I noticed is znc failing on DSM7 with github-action but I can't reproduce this on my end?! Can you check on your side if you're able to reproduce in your environment?

@th0ma7 th0ma7 linked an issue Sep 3, 2023 that may be closed by this pull request
1 task
@hgy59
Copy link
Contributor

hgy59 commented Sep 3, 2023

@hgy59 everything should now be solved and build to complete sucessfully. Last thing I noticed is znc failing on DSM7 with github-action but I can't reproduce this on my end?! Can you check on your side if you're able to reproduce in your environment?

We already faced this #5820 (comment)
It must be related to github-action since local builds do not fail.

@hgy59
Copy link
Contributor

hgy59 commented Sep 3, 2023

@hgy59 Always a pleasure. Note that I am temporary marking homeassistant as broken. You'll have to revert that once you rebase against master.

This is a bad idea. Homeassistant must build here (as it did before). #5857 is only a runtime dependency fix for IKEA tradfri (I forgot to include cross/dtlssocket). Everything else (i.e. the current version) must build as it did in #5757 based on #5820.

@hgy59
Copy link
Contributor

hgy59 commented Sep 3, 2023

@th0ma7 another small issue
python311, when built with all wheels, has two times cryptography in the wheelhouse of the package
for x64:

  • cryptography-41.0.3-cp36-abi3-linux_x86_64.whl
  • cryptography-41.0.3-cp311-cp311-linux_x86_64.whl

probably it is obvious (in your eyes) and can be fixed for future builds (similar issue in python 3.10).

@hgy59
Copy link
Contributor

hgy59 commented Sep 3, 2023

@th0ma7 homeassistant fails to build numpy 1.23.2

  × Preparing metadata (pyproject.toml) did not run successfully.
  │ exit code: 1
  ╰─> [102 lines of output]
      <string>:71: RuntimeWarning: NumPy 1.23.2 may not yet support Python 3.11.
      Running from numpy source directory.
      <string>:86: DeprecationWarning:
      
        `numpy.distutils` is deprecated since NumPy 1.23.0, as a result
        of the deprecation of `distutils` itself. It will be removed for
        Python >= 3.12. For older Python versions it will remain present.
        It is recommended to use `setuptools < 60.0` for those Python versions.
        For more details, see:
          https://numpy.org/devdocs/reference/distutils_status_migration.html

any idea on how to fix this?
an updtae of numpy is not an option, homeassistant 2023.7.3 requires numpy==1.23.2.. And an update to homeassistant 2023.8.x is not yet possible, (requires Pillow==10.0.0, and still depends on numpy==1.23.2).

@th0ma7
Copy link
Contributor Author

th0ma7 commented Sep 3, 2023

any idea on how to fix this? an updtae of numpy is not an option, homeassistant 2023.7.3 requires numpy==1.23.2.. And an update to homeassistant 2023.8.x is not yet possible, (requires Pillow==10.0.0, and still depends on numpy==1.23.2).

@hgy59 yes, now fixed. Had to revert setuptools once again to accomodate this version of numpy.

@th0ma7
Copy link
Contributor Author

th0ma7 commented Sep 3, 2023

another small issue python311, when built with all wheels, has two times cryptography in the wheelhouse of the package for x64:

  • cryptography-41.0.3-cp36-abi3-linux_x86_64.whl
  • cryptography-41.0.3-cp311-cp311-linux_x86_64.whl

probably it is obvious (in your eyes) and can be fixed for future builds (similar issue in python 3.10).

@hgy59 actually this isn't an issue. By default python311 is built with close to no wheels being included. When testing all wheels building indeed it test out that cryptography does work both from cross AND from requirement files. It's not an issue but rather a confirmation that all is working OK. At the end of day, python311 provided online will not provide any of that but rather the bare minimal wheels from requirements-pure.txt.

@th0ma7
Copy link
Contributor Author

th0ma7 commented Sep 3, 2023

Last thing I noticed is znc failing on DSM7 with github-action but I can't reproduce this on my end?! Can you check on your side if you're able to reproduce in your environment?

We already faced this #5820 (comment) It must be related to github-action since local builds do not fail.

Thnx for the reminder... I'll investigate and check if I can find something out to enforce it to work out on DSM7 on github-action so we can get clean builds.

@hgy59 hgy59 mentioned this pull request Sep 3, 2023
6 tasks
@hgy59
Copy link
Contributor

hgy59 commented Sep 3, 2023

Thnx for the reminder... I'll investigate and check if I can find something out to enforce it to work out on DSM7 on github-action so we can get clean builds.

It still does not find openssl on DSM 7.x

is says found: yes, but usable no

2023-09-03T17:15:43.5575197Z checking for openssl... yes
2023-09-03T17:15:43.5740183Z checking whether openssl is usable... no

full log of znc build for x64-7.1

2023-09-03T17:15:41.5363851Z ===>  Extracting for znc
2023-09-03T17:15:41.5384402Z tar -xzpf /github/workspace/cross/znc/../../distrib/znc-1.8.2.tar.gz -C /github/workspace/spk/znc/work-x64-7.1 
2023-09-03T17:15:41.6074642Z ===>  Patching for znc
2023-09-03T17:15:41.6136062Z ===>  Configuring for znc
2023-09-03T17:15:41.6145218Z ===>  - Configure ARGS: --enable-python --enable-openssl
2023-09-03T17:15:41.6153641Z ===>  - Install prefix: /var/packages/znc/target
2023-09-03T17:15:41.6163485Z ===>  - Install prefix [var]: /var/packages/znc/var
2023-09-03T17:15:41.6190485Z cd /github/workspace/spk/znc/work-x64-7.1/znc-1.8.2 && env PKG_CONFIG_LIBDIR=/github/workspace/spk/znc/work-x64-7.1/install//var/packages/znc/target/lib/pkgconfig WORK_DIR=/github/workspace/spk/znc/work-x64-7.1 INSTALL_PREFIX=/var/packages/znc/target TC=syno-x64-7.1 SYSROOT="/github/workspace/toolchain/syno-x64-7.1/work/x86_64-pc-linux-gnu/x86_64-pc-linux-gnu/sys-root" LD="/github/workspace/toolchain/syno-x64-7.1/work/x86_64-pc-linux-gnu/bin/x86_64-pc-linux-gnu-ld" LDSHARED="/github/workspace/toolchain/syno-x64-7.1/work/x86_64-pc-linux-gnu/bin/x86_64-pc-linux-gnu-gcc -shared" CPP="/github/workspace/toolchain/syno-x64-7.1/work/x86_64-pc-linux-gnu/bin/x86_64-pc-linux-gnu-cpp" NM="/github/workspace/toolchain/syno-x64-7.1/work/x86_64-pc-linux-gnu/bin/x86_64-pc-linux-gnu-nm" CC="/github/workspace/toolchain/syno-x64-7.1/work/x86_64-pc-linux-gnu/bin/x86_64-pc-linux-gnu-gcc" AS="/github/workspace/toolchain/syno-x64-7.1/work/x86_64-pc-linux-gnu/bin/x86_64-pc-linux-gnu-as" RANLIB="/github/workspace/toolchain/syno-x64-7.1/work/x86_64-pc-linux-gnu/bin/x86_64-pc-linux-gnu-ranlib" CXX="/github/workspace/toolchain/syno-x64-7.1/work/x86_64-pc-linux-gnu/bin/x86_64-pc-linux-gnu-g++" AR="/github/workspace/toolchain/syno-x64-7.1/work/x86_64-pc-linux-gnu/bin/x86_64-pc-linux-gnu-ar" STRIP="/github/workspace/toolchain/syno-x64-7.1/work/x86_64-pc-linux-gnu/bin/x86_64-pc-linux-gnu-strip" OBJDUMP="/github/workspace/toolchain/syno-x64-7.1/work/x86_64-pc-linux-gnu/bin/x86_64-pc-linux-gnu-objdump" OBJCOPY="/github/workspace/toolchain/syno-x64-7.1/work/x86_64-pc-linux-gnu/bin/x86_64-pc-linux-gnu-objcopy" READELF="/github/workspace/toolchain/syno-x64-7.1/work/x86_64-pc-linux-gnu/bin/x86_64-pc-linux-gnu-readelf" CFLAGS="-I/github/workspace/toolchain/syno-x64-7.1/work/x86_64-pc-linux-gnu/x86_64-pc-linux-gnu/sys-root/usr/include -I/github/workspace/spk/znc/work-x64-7.1/install/var/packages/znc/target/include " CPPFLAGS="-I/github/workspace/toolchain/syno-x64-7.1/work/x86_64-pc-linux-gnu/x86_64-pc-linux-gnu/sys-root/usr/include -I/github/workspace/spk/znc/work-x64-7.1/install/var/packages/znc/target/include " CXXFLAGS="-I/github/workspace/toolchain/syno-x64-7.1/work/x86_64-pc-linux-gnu/x86_64-pc-linux-gnu/sys-root/usr/include -I/github/workspace/spk/znc/work-x64-7.1/install/var/packages/znc/target/include " LDFLAGS="-L/github/workspace/toolchain/syno-x64-7.1/work/x86_64-pc-linux-gnu/x86_64-pc-linux-gnu/sys-root/lib -L/github/workspace/spk/znc/work-x64-7.1/install/var/packages/znc/target/lib -Wl,--rpath-link,/github/workspace/spk/znc/work-x64-7.1/install/var/packages/znc/target/lib -Wl,--rpath,/var/packages/znc/target/lib -Wl,--rpath-link,/github/workspace/spk/python311/work-x64-7.1/install/var/packages/python311/target/lib -Wl,--rpath,/var/packages/python311/target/lib" CARGO_HOME="/github/workspace/distrib/cargo" RUSTUP_HOME="/github/workspace/distrib/rustup" CARGO_BUILD_TARGET="x86_64-unknown-linux-gnu" CARGO_TARGET_X86_64_UNKNOWN_LINUX_GNU_AR="/github/workspace/toolchain/syno-x64-7.1/work/x86_64-pc-linux-gnu/bin/x86_64-pc-linux-gnu-ar" CARGO_TARGET_X86_64_UNKNOWN_LINUX_GNU_LINKER="/github/workspace/toolchain/syno-x64-7.1/work/x86_64-pc-linux-gnu/bin/x86_64-pc-linux-gnu-gcc" TC_GCC=$(eval $(echo /github/workspace/spk/znc/work-x64-7.1/../../../toolchain/syno-x64-7.1/work/x86_64-pc-linux-gnu/bin/x86_64-pc-linux-gnu-gcc -dumpversion) 2>/dev/null || true) TC_GLIBC=2.26 TC_KERNEL=4.4.180+ ./configure --host=x86_64-pc-linux-gnu --build=i686-pc-linux --prefix=/var/packages/znc/target --localstatedir=/var/packages/znc/var --enable-python --enable-openssl
2023-09-03T17:15:41.8394805Z checking whether the C++ compiler works... yes
2023-09-03T17:15:41.8395292Z checking for C++ compiler default output file name... a.out
2023-09-03T17:15:41.9078780Z checking for suffix of executables... 
2023-09-03T17:15:41.9113705Z checking whether we are cross compiling... yes
2023-09-03T17:15:41.9356418Z checking for suffix of object files... o
2023-09-03T17:15:41.9576766Z checking whether we are using the GNU C++ compiler... yes
2023-09-03T17:15:41.9806781Z checking whether /github/workspace/toolchain/syno-x64-7.1/work/x86_64-pc-linux-gnu/bin/x86_64-pc-linux-gnu-g++ accepts -g... yes
2023-09-03T17:15:42.2440692Z checking whether /github/workspace/toolchain/syno-x64-7.1/work/x86_64-pc-linux-gnu/bin/x86_64-pc-linux-gnu-g++ supports C++11 features by default... yes
2023-09-03T17:15:42.2537604Z checking for a BSD-compatible install... /usr/bin/install -c
2023-09-03T17:15:42.2564201Z checking for grep that handles long lines and -e... /bin/grep
2023-09-03T17:15:42.2609487Z checking for a sed that does not truncate output... /bin/sed
2023-09-03T17:15:42.2750120Z checking build system type... i686-pc-linux-gnu
2023-09-03T17:15:42.2809007Z checking host system type... x86_64-pc-linux-gnu
2023-09-03T17:15:42.3133896Z checking for special C compiler options needed for large files... no
2023-09-03T17:15:42.3436474Z checking for _FILE_OFFSET_BITS value needed for large files... no
2023-09-03T17:15:42.3668835Z checking whether the -Werror option is usable... yes
2023-09-03T17:15:42.3898037Z checking for simple visibility declarations... yes
2023-09-03T17:15:42.3910052Z checking for git... /usr/bin/git
2023-09-03T17:15:42.3916254Z checking for x86_64-pc-linux-gnu-pkg-config... /usr/bin/x86_64-pc-linux-gnu-pkg-config
2023-09-03T17:15:42.3931846Z checking pkg-config is at least version 0.9.0... yes
2023-09-03T17:15:42.4095167Z checking whether compiler predefines _FORTIFY_SOURCE... no
2023-09-03T17:15:42.4576796Z checking for getopt_long in -lgnugetopt... no
2023-09-03T17:15:42.5333747Z checking for lstat... yes
2023-09-03T17:15:42.6148389Z checking for getopt_long... yes
2023-09-03T17:15:42.7022354Z checking for getpassphrase... no
2023-09-03T17:15:42.7805017Z checking for clock_gettime... yes
2023-09-03T17:15:42.8628850Z checking for tcsetattr... yes
2023-09-03T17:15:43.0106286Z checking for library containing dlopen... -ldl
2023-09-03T17:15:43.0743038Z checking for the pthreads library -lpthreads... no
2023-09-03T17:15:43.1652509Z checking whether pthreads work without any flags... no
2023-09-03T17:15:43.1771402Z checking whether pthreads work with -Kthread... no
2023-09-03T17:15:43.1894323Z checking whether pthreads work with -kthread... no
2023-09-03T17:15:43.2526579Z checking for the pthreads library -llthread... no
2023-09-03T17:15:43.3403508Z checking whether pthreads work with -pthread... yes
2023-09-03T17:15:43.4223987Z checking for joinable pthread attribute... PTHREAD_CREATE_JOINABLE
2023-09-03T17:15:43.4224690Z checking if more special flags are required for pthreads... no
2023-09-03T17:15:43.5060661Z checking for PTHREAD_PRIO_INHERIT... yes
2023-09-03T17:15:43.5471861Z checking whether getaddrinfo() supports AI_ADDRCONFIG... yes
2023-09-03T17:15:43.5575197Z checking for openssl... yes
2023-09-03T17:15:43.5740183Z checking whether openssl is usable... no
2023-09-03T17:15:43.5754805Z configure: error: OpenSSL not found. Try --disable-openssl.
2023-09-03T17:15:43.5983523Z make[3]: *** [../../mk/spksrc.configure.mk:57: configure_target] Error 1
2023-09-03T17:15:43.5987563Z make[3]: Leaving directory '/github/workspace/cross/znc'
2023-09-03T17:15:43.5988261Z make[2]: *** [../../mk/spksrc.depend.mk:54: depend_target] Error 2
2023-09-03T17:15:43.5988790Z make[2]: Leaving directory '/github/workspace/spk/znc'
2023-09-03T17:15:43.5998610Z make[1]: *** [../../mk/spksrc.spk.mk:691: build-arch-x64-7.1] Error 1
2023-09-03T17:15:43.5999251Z make[1]: Leaving directory '/github/workspace/spk/znc'
2023-09-03T17:15:43.6002770Z make: *** [../../mk/spksrc.spk.mk:685: arch-x64-7.1] Error 2
2023-09-03T17:15:43.6003207Z make: Leaving directory '/github/workspace/spk/znc'
2023-09-03T17:15:43.6121322Z make: Entering directory '/github/workspace/spk/znc'
2023-09-03T17:15:43.6498350Z rm -fr work work-* build-*.log publish-*.log
2023-09-03T17:15:43.8783345Z make: Leaving directory '/github/workspace/spk/znc'
2023-09-03T17:15:43.8784312Z ##[endgroup]

@hgy59
Copy link
Contributor

hgy59 commented Sep 3, 2023

@th0ma7 again, I would not postpone this PR for finding a solution for znc and DSM 7.1.
It might be related to the sum of packages that are built before znc (since znc is the very latest).
To validate this, we must merge this PR and then trigger a build of znc only.

Or - to validate my guess, we could move znc to the head of the package list for the gh build action (just after python3* and ffmpeg*) - but IMHO it is still not worth the effort.

@th0ma7
Copy link
Contributor Author

th0ma7 commented Sep 3, 2023

@hgy59 I believe I have now sort out issues with znc... Now let's wait and see how the full build goes.

@th0ma7
Copy link
Contributor Author

th0ma7 commented Sep 3, 2023

@hgy59 libtree output of resulting shared objects where errors are actually ok as resulting from my build system but demonstrating the rpat are properly set:

$ libtree -vvv ./lib/znc/modpython/_znc_core.so
./lib/znc/modpython/_znc_core.so 
├── libstdc++.so.6 [ld.so.conf]
│   ├── libm.so.6 [ld.so.conf]
│   │   ├── libc.so.6 [ld.so.conf]
│   │   │   └── ld-linux.so.2 [ld.so.conf]
│   │   └── ld-linux.so.2 [ld.so.conf]
│   ├── libgcc_s.so.1 [ld.so.conf]
│   │   └── libc.so.6 [ld.so.conf]
│   │       └── ld-linux.so.2 [ld.so.conf]
│   ├── ld-linux.so.2 [ld.so.conf]
│   └── libc.so.6 [ld.so.conf]
│       └── ld-linux.so.2 [ld.so.conf]
├── libc.so.6 [ld.so.conf]
│   └── ld-linux.so.2 [ld.so.conf]
├── libpthread.so.0 [ld.so.conf]
│   ├── libc.so.6 [ld.so.conf]
│   │   └── ld-linux.so.2 [ld.so.conf]
│   └── ld-linux.so.2 [ld.so.conf]
├── libgcc_s.so.1 [ld.so.conf]
│   └── libc.so.6 [ld.so.conf]
│       └── ld-linux.so.2 [ld.so.conf]
├── libm.so.6 [ld.so.conf]
│   ├── libc.so.6 [ld.so.conf]
│   │   └── ld-linux.so.2 [ld.so.conf]
│   └── ld-linux.so.2 [ld.so.conf]
└── libpython3.11.so.1.0 not found
    ┊ Paths considered in this order:
    ┊ 1. rpath:
    ┊    depth 1
    ┊    /var/packages/znc/target/lib
    ┊    /var/packages/python311/target/lib
    ┊ 2. LD_LIBRARY_PATH was not set
    ┊ 3. runpath was not set
    ┊ 4. ld config files:
    ┊    /usr/lib/x86_64-linux-gnu/libfakeroot
    ┊    /usr/local/lib
    ┊    /usr/local/lib/x86_64-linux-gnu
    ┊    /lib/x86_64-linux-gnu
    ┊    /usr/lib/x86_64-linux-gnu
    ┊    /lib32
    ┊    /usr/lib32
    ┊    /libx32
    ┊    /usr/libx32
    ┊ 5. Standard paths:
    ┊    /lib
    ┊    /lib64
    ┊    /usr/lib
    ┊    /usr/lib64
Error [./lib/znc/modpython/_znc_core.so]: Not all dependencies were found

$ libtree -vvv ./lib/znc/push.so 
├── libdl.so.2 [ld.so.conf]
├── libc.so.6 [ld.so.conf]
├── libpthread.so.0 [ld.so.conf]
│   ├── libc.so.6 [ld.so.conf]
│   └── ld-linux.so.2 [ld.so.conf]
├── libgcc_s.so.1 [ld.so.conf]
├── libm.so.6 [ld.so.conf]
├── libstdc++.so.6 [ld.so.conf]
├── libicudata.so.71 not found
├── libssl.so.3 not found
├── libcrypto.so.3 not found
├── libz.so.1 not found
└── libicuuc.so.71 not found
    ┊ Paths considered in this order:
    ┊ 1. rpath:
    ┊    depth 1
    ┊    /var/packages/znc/target/lib
    ┊    /var/packages/python311/target/lib
    ┊ 2. LD_LIBRARY_PATH was not set
    ┊ 3. runpath was not set
    ┊ 4. ld config files:
    ┊    /usr/lib/x86_64-linux-gnu/libfakeroot
    ┊    /usr/local/lib
    ┊    /usr/local/lib/x86_64-linux-gnu
    ┊    /lib/x86_64-linux-gnu
    ┊    /usr/lib/x86_64-linux-gnu
    ┊    /lib32
    ┊    /usr/lib32
    ┊    /libx32
    ┊    /usr/libx32
    ┊ 5. Standard paths:
    ┊    /lib
    ┊    /lib64
    ┊    /usr/lib
    ┊    /usr/lib64
Error [./lib/znc/push.so]: Not all dependencies were found

@th0ma7
Copy link
Contributor Author

th0ma7 commented Sep 4, 2023

@hgy59 theses are my last two testing use-cases in hope for fixing ZNC. In the meantime I did resolved something else which allowed me to remove bzip2 from the exception, leaving only zlib to always be built locally.

All in all, it's looking promising but if ZNC fails to build online I'll remove the speicifcs for it and my re-convert it back to normal build process.

EDIT: I've now moved the "exceptions" needed for ZNC directly within ZNC cross file.

EDIT2: And also noticed that vim wasn't migrated to python311 yet...

spk/vim/Makefile Outdated
Comment on lines 6 to 11
DEPENDS = cross/$(SPK_NAME)
PYTHON_PACKAGE = python311
SPK_DEPENDS = "python311>=3.11.5-8"
UNSUPPORTED_ARCHS = $(OLD_PPC_ARCHS) $(ARMv5_ARCHS)

MAINTAINER = SynoCommunity
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since python is an optional dependency for the vim package, we must not add "SPK_DEPENDS"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@th0ma7 we could either use python310 for those archs or drop python support.

We should keep off the hardcoded dependency of python, since python support is optionally and must be manually configured by the user.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@th0ma7 probably I am wrong with the "manually configure vim for python" but vim can be built with or without the python feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hgy59 do you mind if we revisit this to a later time? I agree that it can certainly be worked out differently, and would benefit from an update also. But for the sake of py311 upgrade I'd keep it as is...?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@th0ma7 if you want to postpone this, then please leave PYTHON_PACKAGE for vim at python310 to keep the packages for OLD_PPC_ARCHS and ARMv5_ARCHS.

And please remove SPK_DEPENDS.

Copy link
Contributor Author

@th0ma7 th0ma7 Sep 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hgy59 Created a new commit which should allow creating vim for all archs...
eb0a842

What do you think?

Update: It built succesfully :)

@th0ma7
Copy link
Contributor Author

th0ma7 commented Sep 4, 2023

@hgy59 success at last!

@th0ma7 th0ma7 requested a review from hgy59 September 5, 2023 17:33
spk/vim/Makefile Outdated
Comment on lines 21 to 24
ifneq ($(findstring $(ARCH),$(ARMv5_ARCHS) $(OLD_PPC_ARCHS)),$(ARCH))
SPK_DEPENDS = "python311>=3.11.5-8"
endif

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we keep this dependency optional and remove the SPK_DEPENDS?

So (as before) you can install vim without python, but you can't use the python feature until you install python311.
If one wants to use the python feature, now python311 is needed and can be installed before or after the vim package.

So the changelog must change 2. Migrate to Python 3.11 to 2. You now need Python 3.11 to enable Python support.

Probably it is not enough to mention this in the changelog, and we should add a note (except for ARMv5 and OLD_PPC) to the package description(s) like:

"To enable Python support you need to install the Python 3.11 package"

sorry to remain pedantic, but why sould a user need to install python for the update, when the python feature is not needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This really was to demonstrate two things:

  1. it's still possible to build vim for armv5
  2. it's also possible to migrate vim to python 3.11

As far as python being optional, I had not looked into that just yet. Indeed it can be but there is a need to provide proper messaging on that matter... And not sure the changlog is the best but certainly is the easiest (wizard perhaps would be better but more complex due to arch build differences?). Personnally I find making it mandatory by default is the simplest and safest.

But AFAIK you seem to object to that therefore I can change this easily by switching SPK_DEPENDS to a changelog += instead. Before I make the necessary changes, would that meet your train of thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hgy59 Would 1a355eb + 59b561d suit your need? I've checked and the INFO looks OK from a " perspective:

$ cat work-88f6281-6.2.4/INFO 
package="vim"
version="9.0-6"
description="Vim is a highly configurable text editor built to enable efficient text editing. It is an improved version of the vi editor distributed with most UNIX systems."
description_csy="Vim je pokročilý textový editor, který usiluje o to poskytnout sílu de-facto Unixového editoru Vi, s komplexnější sadou vlastností."
description_fre="Vim est un éditeur de texte avancé basé sur Vi (unix) avec des fonctions plus complètes."

arch="88f6281"
maintainer="SynoCommunity"
maintainer_url="https://github.com/SynoCommunity"
distributor=""
distributor_url=""
os_min_ver="6.2-25556"
helpurl="https://www.vim.org/"
ctl_stop="no"
displayname="Vim"
dsmappname="com.synocommunity.packages.vim"
changelog="1. Update vim to v9.0"
checksum="2d40a3b4be313ac1f3f7737fb25b698b"
$ cat work-x64-7.1/INFO 
package="vim"
version="9.0-6"
description="Vim is a highly configurable text editor built to enable efficient text editing. It is an improved version of the vi editor distributed with most UNIX systems."
description_csy="Vim je pokročilý textový editor, který usiluje o to poskytnout sílu de-facto Unixového editoru Vi, s komplexnější sadou vlastností."
description_fre="Vim est un éditeur de texte avancé basé sur Vi (unix) avec des fonctions plus complètes."

arch="apollolake avoton braswell broadwell broadwellnk broadwellnkv2 broadwellntbap bromolow cedarview denverton epyc7002 geminilake grantley kvmx64 purley r1000 v1000"
maintainer="SynoCommunity"
maintainer_url="https://github.com/SynoCommunity"
distributor=""
distributor_url=""
os_min_ver="7.1-42661"
helpurl="https://www.vim.org/"
ctl_stop="no"
displayname="Vim"
dsmappname="com.synocommunity.packages.vim"
changelog="1. Update vim to v9.0 <br/>2. Migrate to Python 3.11 <br/><br/>NOTE: You need to install Python 3.11 package to enable Python support."
checksum="721d1b9df75c45c26927d4dcfef56b52"

@th0ma7 th0ma7 requested a review from hgy59 September 6, 2023 13:08
spk/vim/Makefile Outdated Show resolved Hide resolved
Co-authored-by: hgy59 <hpgy59@gmail.com>
@th0ma7 th0ma7 merged commit 5b75b46 into SynoCommunity:master Sep 6, 2023
17 checks passed
@th0ma7 th0ma7 deleted the py311-310-update branch September 6, 2023 23:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants