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

Build failure due to invalid version string #14228

Open
akstrfn opened this issue Jan 16, 2024 · 9 comments
Open

Build failure due to invalid version string #14228

akstrfn opened this issue Jan 16, 2024 · 9 comments
Assignees
Milestone

Comments

@akstrfn
Copy link

akstrfn commented Jan 16, 2024

Hi, since python setuptools version 66 non conformant version strings are not accepted hence build fails with InvalidVersion exception. (cf. pypa/setuptools#3772 and https://peps.python.org/pep-0440/).

As a workaround to make the build work for Archlinux users of AUR (https://aur.archlinux.org/cgit/aur.git/tree/PKGBUILD?h=sumo), I've hacked a solution:

sed -i "42i\    return 'v1_19_00'" tools/build/version.py

Note that in HEAD this file under is build_config since the folder was renamed and I didn't really examine the problem in details i.e. this was very quick hack to make the build work.

Cloning the repo and building makes the version number from .git hence it also doesnt conform hence HEAD build also fails.

Related to this issues is the fact that downloading a tarball and running the build results in Version == UNKNOWN which is an unexpected behavior. Note that github readme and detailed build instructions link refer to two different tarballs which from cursory check have different sizes (I'm using github tarballs for AUR builds).

SUMO-version:

  • HEAD
  • 1.19.0

operating system:

ArchLinux

@behrisch
Copy link
Contributor

Please do not download a github snapshot. I think with an "official" source release from https://sumo.dlr.de/docs/Downloads.php#sources it should work.

@behrisch
Copy link
Contributor

Also please elaborate on the fact that we are linking different tarballs from https://sumo.dlr.de/docs/Installing/Linux_Build.html and https://github.com/eclipse-sumo/sumo/blob/main/README.md. As far as I can see the latter does not link any tarball at all. The two source links in the download page are last release and nightly snapshot, so they are expected to be different.

@akstrfn
Copy link
Author

akstrfn commented Jan 17, 2024

My bad, I've scanned the github readme very fast and as usual practice assumed that build instructions refer to the release tarball from github because this is a very common practice, so indeed there is no direct reference to the tarball in the github readme.

"Official" tarball works so I'll switch the AUR source to use it.

Its really confusing to have a "Release" tarball on github which is different from the actual release tarball as simple git tag would convey the message much better (if the message is that its just a snapshot of the git repo).

I've also tested the HEAD builds again and now they worked even without generating version.h file which yesterday had version.h made from .git... I've no idea whats going on but I don't have time to debug it.

@akstrfn akstrfn closed this as completed Jan 17, 2024
@behrisch
Copy link
Contributor

behrisch commented Jan 18, 2024

Glad that it works now!

Its really confusing to have a "Release" tarball on github which is different from the actual release tarball as simple git tag would convey the message much better (if the message is that its just a snapshot of the git repo).

It actually is just that, a tag. We do not have any releases on GitHub, if you look here: https://github.com/eclipse-sumo/sumo/releases. But if you have any idea on how we can make this clearer for the user please tell us.

I would also be willing to setup a regular Arch build at the open build service. I actually already added an Arch distro here: https://build.opensuse.org/project/show/home:behrisch, if that counts as a first step ;-). If you have any tips concerning open build service and Arch I would be happy to push this forward.

@akstrfn
Copy link
Author

akstrfn commented Jan 18, 2024

It actually is just that, a tag. We do not have any releases on GitHub, if you look here: https://github.com/eclipse-sumo/sumo/releases. But if you have any idea on how we can make this clearer for the user please tell us.

Now that you point it out and taking a second look this is more a github UI issue, it bundles Release and tag together unless explicitly clicked on tag so not much can be done on your side.

I would also be willing to setup a regular Arch build at the open build service. I actually already added an Arch distro here: https://build.opensuse.org/project/show/home:behrisch, if that counts as a first step ;-). If you have any tips concerning open build service and Arch I would be happy to push this forward.

AUR is user contributed and is not a part of official ArchLinux packages. In principle it is supposed to contain only build definition from the source and is defined by PKGBUILD file that is then built on each user's computer.

I can help out if you have some specific issues. In general ArchLinux has one of the simplest approaches to packaging out of all distros, which is probably the reason why AUR is so huge. PKGBUILD defines the build and is in principle just a set of commands you'd run on command line.

I'm not using Open Build Services but cursory check of the documentation points that they use PKGBUILD files which means you could take PKGBUILD file from AUR that I'm maintaining (semi regularly...) and use it directly. Its not really up to date with the latest packaging standards for cmake nor did I review SUMO build options for quite some time but if needed I can do this during this weekend. Note that I've commented out test part of PKGBUILD because first test (example) fails which makes the whole package "fail" so as convenience to users that use AUR helpers I've commented it out so that they can simply install SUMO.

In 1.19.0 it seems that SUMO is using virtual env during the build to pin the python package versions? If so this would exclude it for future inclusion in the official repositories of ArchLinux because maintainers prefer distro packages and IIRC maintainers follow this quite strictly so they would either patch the build or not bother and never include it. So if there is a desire in the future to reach the official repositories it would be better to try to keep working with multiple versions of used libraries.

Having automated testing build on ArchLinux would in my view be very beneficial because it will be the first place to hit bugs related to new libraries since its rolling release and often has all the latest software. For example setuptools v69 is already in the official repos and the bug from this report first appears in setuptools v66.

@behrisch
Copy link
Contributor

OK, the open build service works now. I did some modifications to the PKGBUILD, maybe you want to reintegrate them, see here: https://build.opensuse.org/package/show/home:behrisch/sumo_nightly. The virtualenv thing is actually a feature of the new python package build process if I understand it correctly but I will have a closer look.

@akstrfn akstrfn reopened this Jan 21, 2024
@akstrfn
Copy link
Author

akstrfn commented Jan 21, 2024

I've reopened the issue because the original bug is not fixed. I found a way to trigger a failure, sumo just needs to be installed in the system and then build fails because somehow UNKOWN is set as version but I didnt get to the bottom of the reason but it clear that build is not deterministic. As mentioned at the start this is not compliant with PIP440 so function https://github.com/eclipse-sumo/sumo/blob/main/tools/build_config/version.py#L45 has a bug because it doesnt handle all cases properly. Overall version handling code is way too complicated for such a simple task and needs a thorough review and should never return UNKOWN.

I've checked PKGBUILD and seems that you've added more deps and changed version to git or I missed something? Originally I avoided adding java to the package since it doesnt seem that its often used in linux but I've added it now and reviewed PKGBUILD completely but I'm still not sure its completely clean. Note that git as version is not correct way to set version (doesnt matter for open builds I think) also setting -j4 is wrong because it overrides the default flags (unless required by open builds). It would be better if test also worked for archlinux (currently commented out).

During review of the build I've found multiple issues and weird things so I'll list them here but they could be split into separate issues however I dont really have much more time to deal with this. You can decide what is relevant.

There is a trivial CMake warning:
CMake Warning (dev) at CMakeLists.txt:39 (project): cmake_minimum_required() should be called prior to this top-level project() call. Please see the cmake-commands(7) manual for usage documentation of both commands. This warning is for project developers. Use -Wno-dev to suppress it.

Java build can be set to ON but build wont fail if it relevant java tools are not found. This is at minimum an unexpected behavior but I'd classify this as a build bug. Additionally it seems that maven is needed for completing the build but also if not present build just runs as if everything is ok.

During java build I noticed that maven produced 1.20 snapshot instead of version 1.19 snip: [INFO] ----------------------< org.eclipse.sumo:libsumo >---------------------- [INFO] Building libsumo 1.20.0-SNAPSHOT

SUMO seems to have reached a point where build for python should be split and observed as separate package e.g. for ArchLinux I think that python-sumolib is appropriate. I dont have complete overview of how SUMO deals with this so maybe its still not possible to make a split, if yes then cmake build should be adapted for this use case.

Build behaves differently if pip is installed which is quite unexpected. This is due to this line:

execute_process(COMMAND ${PYTHON_EXECUTABLE} -c "import build; import pip; build.__version__" RESULT_VARIABLE PYTHON_BUILD_MISSING ERROR_QUIET)
which then affects

sumo/CMakeLists.txt

Lines 642 to 655 in c6b7b48

if (${PYTHON_BUILD_MISSING})
install(CODE "execute_process(COMMAND ${PYTHON_EXECUTABLE} build_config/setup-sumolib.py clean --all install --root=\$ENV{DESTDIR}/ --prefix=${CMAKE_INSTALL_PREFIX} --optimize=1 WORKING_DIRECTORY ${TOOLS_DIR})"
COMPONENT pysumolib)
install(CODE "execute_process(COMMAND ${PYTHON_EXECUTABLE} build_config/setup-traci.py clean --all install --root=\$ENV{DESTDIR}/ --prefix=${CMAKE_INSTALL_PREFIX} --optimize=1 WORKING_DIRECTORY ${TOOLS_DIR})"
COMPONENT pytraci)
else ()
add_custom_target(pure_wheels ALL
COMMAND ${PYTHON_EXECUTABLE} ./build_config/version.py ./build_config/setup-sumolib.py ./setup.py
COMMAND ${PYTHON_EXECUTABLE} -m build --wheel . -o ${CMAKE_CURRENT_BINARY_DIR}
COMMAND ${PYTHON_EXECUTABLE} ./build_config/version.py ./build_config/setup-traci.py ./setup.py
COMMAND ${PYTHON_EXECUTABLE} -m build --wheel . -o ${CMAKE_CURRENT_BINARY_DIR}
WORKING_DIRECTORY ${TOOLS_DIR})
install(CODE "execute_process(COMMAND ${PYTHON_EXECUTABLE} -m pip install --root=\$ENV{DESTDIR}/ --prefix=${CMAKE_INSTALL_PREFIX} -f ${CMAKE_BINARY_DIR} --no-index traci)"
COMPONENT pytraci)

Approach used to generate setup.py file is a hack

sumo/CMakeLists.txt

Lines 649 to 652 in c6b7b48

COMMAND ${PYTHON_EXECUTABLE} ./build_config/version.py ./build_config/setup-sumolib.py ./setup.py
COMMAND ${PYTHON_EXECUTABLE} -m build --wheel . -o ${CMAKE_CURRENT_BINARY_DIR}
COMMAND ${PYTHON_EXECUTABLE} ./build_config/version.py ./build_config/setup-traci.py ./setup.py
COMMAND ${PYTHON_EXECUTABLE} -m build --wheel . -o ${CMAKE_CURRENT_BINARY_DIR}

At least after v1.19 you've changed the code not to use text substitution to generate setup.py by importing get_pep440_version() but honestly its a bit insane to approach python programming with text substituting of the source files. To make the matters worse cmake, which is supposed to be out of source build, actually triggers modification of the source files (setup.py).

Since cmake is out of source it shouldnt reference the source folders directly during the build. Now there are multiple hacks to reference the directories of the source files directly.

This is also very lazy and hacky approach to solve for python version used

find ./tools -name '*.py' -print0 | xargs -0 sed -i 's,^#!/usr/bin/env python2,#!/usr/bin/python2,'
find ./tools -name '*.py' -print0 | xargs -0 sed -i 's,^#!/usr/bin/env python3,#!/usr/bin/python3,'
find ./tools -name '*.py' -print0 | xargs -0 sed -i 's,^#!/usr/bin/env python,#!/usr/bin/python3,'
find ./docs -name '*.py' -print0 | xargs -0 sed -i 's,^#!/usr/bin/env python,#!/usr/bin/python3,'
find ./docs -name '*.py' -print0 | xargs -0 sed -i 's,"python","python3",'
. Note that find has -exec option. (side note why not drop python2?)

When built package is very large and sumo/tools is 106MB. I'm not sure how much of that is really necessary. For example sumo/tools/contributed is 19MB and some code there doesnt even seem to work. Cleanup and modular build would help slim down the package.

After building the package I've ran namcap to check for common errors sumo-namcap.txt. There seems to be great deal of unnecessary linking.

Since this took me way too much time today I dont plan on devoting much more time to SUMO build for now but if you have some question that I can quickly answer or some quick fixes that I need to apply to the PKGBUILD I'll be available.

@behrisch
Copy link
Contributor

behrisch commented Jan 22, 2024

Wow, thanks for your thorough analysis! Yes, the build and versioning got a bit out of control. I will try to have a look at the issues one by one. Concerning the PKGBUILD changes I just wanted to point your attention to the fact that sumo already ships a desktop file and some sh files for setting the environment variable in build_config/package, so there is no need to have separate ones in Arch.

@akstrfn
Copy link
Author

akstrfn commented Jan 22, 2024

Perfect, I'm changing PKGBUILD to use the script and the desktop file from sumo. To make this more streamlined you could add it directly to the install with cmake so that there is no need to do anything else except run install from make file. Using https://cmake.org/cmake/help/latest/module/GNUInstallDirs.html should make it portable on linux systems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants