apache / buildstream

BuildStream, the software integration tool
https://buildstream.build/
Apache License 2.0
82 stars 28 forks source link

Can't build with Python 3.12 #1855

Closed mbridon closed 1 year ago

mbridon commented 1 year ago

Buildstream can't build with Python 3.12.

Python 3.12 deprecated a few things, among which is configparser.SafeConfigParser.

https://docs.python.org/3.12/whatsnew/3.12.html#removed

Pretty sure the versioneer.py script could instead say something like :

    setup_cfg = os.path.join(root, "setup.cfg")
    try;
        if sys.version_info.major == 3 and sys.version_info.minor >= 12:
            from configparser import ConfigParser

        else:
            from configparser import SafeConfigParser as ConfigParser
nanonyme commented 1 year ago

I think SafeConfigParser was deprecated since moving away from Python 2 which BuildStream 2 does not support. As such, it could simply probably switch to plain ConfigParser.

mbridon commented 1 year ago

Exactly.

I'll try to cook up a patch if I have a moment, but I'm not sure when I'll have time for it before next week. 😕

mbridon commented 1 year ago

Tried patching versioneer.py as follows:

commit 06ca13b4181c9140a536a72086a324e242ec249b (HEAD -> main)
Author: Mathieu Bridon <mathieu@hashbang.coop>
Date:   Tue Aug 8 09:28:01 2023 +0200

    Try to fix #1855

diff --git a/versioneer.py b/versioneer.py
index 1c97e02..5751584 100644
--- a/versioneer.py
+++ b/versioneer.py
@@ -340,9 +340,16 @@ def get_config_from_root(root):
     # configparser.NoOptionError (if it lacks "VCS="). See the docstring at
     # the top of versioneer.py for instructions on writing your setup.cfg .
     setup_cfg = os.path.join(root, "setup.cfg")
-    parser = configparser.SafeConfigParser()
-    with open(setup_cfg, "r") as f:
-        parser.readfp(f)
+    if sys.version_info.major == 3 and sys.version_info.minor >= 12:
+        parser = configparser.ConfigParser
+        with open(setup_cfg, "r") as f:
+            parser.read_file(f)
+
+    else:
+        parser = configparser.SafeConfigParser
+        with open(setup_cfg, "r") as f:
+            parser.readfp(f)
+
     VCS = parser.get("versioneer", "VCS")  # mandatory

     def get(parser, name):

Building Buildstream in Fedora Rawhide (which does have Python 3.12) with that patch gave me the following :

+ /usr/bin/python3 setup.py build '--executable=/usr/bin/python3 -sP'
/builddir/build/BUILD/buildstream-2.0.1/versioneer.py:430: SyntaxWarning: invalid escape sequence '\s'
  LONG_VERSION_PY['git'] = '''
Traceback (most recent call last):
  File "/builddir/build/BUILD/buildstream-2.0.1/setup.py", line 73, in <module>
    version = versioneer.get_version()
              ^^^^^^^^^^^^^^^^^^^^^^^^
  File "/builddir/build/BUILD/buildstream-2.0.1/versioneer.py", line 1490, in get_version
    return get_versions()["version"]
           ^^^^^^^^^^^^^^
  File "/builddir/build/BUILD/buildstream-2.0.1/versioneer.py", line 1422, in get_versions
    cfg = get_config_from_root(root)
          ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/builddir/build/BUILD/buildstream-2.0.1/versioneer.py", line 346, in get_config_from_root
    parser.read_file(f)
TypeError: RawConfigParser.read_file() missing 1 required positional argument: 'f'

At that point, I'm at a complete loss... I mean, I explicitly wrote in the code : parser.read_file(f), so I did pass the positional argument f... :confused:

mbridon commented 1 year ago

Apologies for editing my comment earlier, I realised I had made a fool of myself and posted a wrong patch as well as wrong errors, which led me to a wrong conclusion. Should be all fixed now :disappointed:

nanonyme commented 1 year ago

Above thing fails because you have ConfigParser instead of ConfigParser(). But as said, delete the entire SafeConfigParser code. There is no point in that backwards-compatibility branch.

mbridon commented 1 year ago

There is for Python < 3.12.

And that will happen on all versions of Fedora except the latest (and not yet released?) 39.

Remember that we build buildstream on multiple OSes, and all have to work.

Ubuntu, Debian, etc... neither have Python 3.12 yet and won't have it for a while. Do you want to cut off Buildstream from all of those?

gtristan commented 1 year ago

Ubuntu, Debian, etc... neither have Python 3.12 yet and won't have it for a while. Do you want to cut off Buildstream from all of those?

Please no :)

We support python back to the last non EOL version (that is currently either 3.7 or 3.8 I believe...).

Do we intend to land this in BuildStream proper or land this in upstream versioneer first and then upgrade versioneer again ?

PS: Long time no see Mathieu, hope you're doing well.

nanonyme commented 1 year ago

This is not new API in Python 3.12. This is removing usage of API deprecated since Python 3.2.

nanonyme commented 1 year ago

Unless a project supports Python2, this compat code is pointless.

nanonyme commented 1 year ago

If the condition allowed supporting any Python version BuildStream 2 supports, I would not have proposed removing it.

gtristan commented 1 year ago

If the replacement used is available since 3.7 then we can remove the branch.

3.6 (EOL) support is almost impossible, I tried it and failed fast.

mbridon commented 1 year ago

Ubuntu, Debian, etc... neither have Python 3.12 yet and won't have it for a while. Do you want to cut off Buildstream from all of those?

Please no :)

We support python back to the last non EOL version (that is currently either 3.7 or 3.8 I believe...).

Do we intend to land this in BuildStream proper or land this in upstream versioneer first and then upgrade versioneer again ?

Ah, I didn't know versioneer was imported into Buildstream as a subproject, I thought the code lived in Buildstream itself :shrug:

So I guess the bug report needs to be opeed there then, I'll try to see if versioneer itself has the same problem on its own first. :wink:

PS: Long time no see Mathieu, hope you're doing well.

Now I am, not sure whether the others told you what happened to me or not?

Long story short, in march 2020 I caught the flu. I didn't know, but turns out a possible consequence of any flu is always that it attacks your spinal chord and nervous system, which is exactly whet happened to me. :disappointed:

Doctors told me while it's a known possible consequence, it's very serious and very rare, hurray for probabilities...

So now I'm in a wheelchair, and I can't trust my body any more as it keeps sending me fake news. :facepalm:

Also I had to change my account as the Github support were unwilling to give me back access to @bochecha and I had to open a new @mbridon... One day they'll realise that account has been inactive for years and the only activity is them sending emails about long closed issues, costing them money in storage and bandwidth :shrug:

Sorry for plumbing the ambiance :smile:

nanonyme commented 1 year ago

@gtristan https://python.readthedocs.io/en/stable/whatsnew/3.2.html#configparser the change is documented here. It literally happened on 3.2. Like 10 versions of Python back in in 2011. You only should reference SafeConfigParser for Python 2 support.

nanonyme commented 1 year ago

This read_file method was added in same release 12 years ago.

gtristan commented 1 year ago

Ubuntu, Debian, etc... neither have Python 3.12 yet and won't have it for a while. Do you want to cut off Buildstream from all of those?

Please no :) We support python back to the last non EOL version (that is currently either 3.7 or 3.8 I believe...). Do we intend to land this in BuildStream proper or land this in upstream versioneer first and then upgrade versioneer again ?

Ah, I didn't know versioneer was imported into Buildstream as a subproject, I thought the code lived in Buildstream itself 🤷

So I guess the bug report needs to be opeed there then, I'll try to see if versioneer itself has the same problem on its own first. 😉

Yeah that will be best (and maybe it will already be fixed there).

To clarify; versioneer is a strange beast, as it does need to be vendored iirc, however while upgrading other things we've found ourselves needing to revendor it and update it.

PS: Long time no see Mathieu, hope you're doing well.

Now I am, not sure whether the others told you what happened to me or not?

Long story short, in march 2020 I caught the flu. I didn't know, but turns out a possible consequence of any flu is always that it attacks your spinal chord and nervous system, which is exactly whet happened to me. 😞

Doctors told me while it's a known possible consequence, it's very serious and very rare, hurray for probabilities...

So now I'm in a wheelchair, and I can't trust my body any more as it keeps sending me fake news. 🤦

Oh my, I'm so sorry to hear all of this. There aren't words for this.

I had been wondering about your disappearance during the bst2 release days and didn't hear about any of this, I hope there is some path to recovery for you, and hope you are finding meaning and fulfillment in life, at least it's nice to see here you again !

nanonyme commented 1 year ago

To be fair, it looks like this is not the right way to integrate versioneer anymore either. https://github.com/python-versioneer/python-versioneer

nanonyme commented 1 year ago

Given this, I would consider just making the simple solution which works on 3.2-3.12 and concentrate on changing versioneer integration separately. The project seems to have changed so that it won't be a simple copy-paste matter.

mbridon commented 1 year ago

OK, so modifying my patch to use ConfigParser() instead of just ConfigParser seems to have done the trick :facepalm:

Thank you @nanonyme, that should have been obvious, I guess the flu also destroyed this :confused:

I'll open the same issue against versioneer then :wink:

abderrahim commented 1 year ago

Another idea: setuptools-scm seems to be the recommended way to do this with recent python. Maybe we should consider looking into it?

gtristan commented 1 year ago

Another idea: setuptools-scm seems to be the recommended way to do this with recent python. Maybe we should consider looking into it?

Bugs in setuptools-scm was the reason why we initially switched to versioneer:

commit 4be462d3a43e2d32c1d2115737acefe632c48ccc
Author: Tristan Van Berkom <tristan.vanberkom@codethink.co.uk>
Date:   Thu Apr 26 13:21:31 2018 +0900

    Use versioneer instead of setuptools_scm

    Using setuptools_scm had a couple of bad problems:

      o Unexpected versioning semantics, setuptools_scm would
        increment the micro version by itself in the case that
        we derive a version number from something that is not a tag,
        making the assumption that we are "leading up to" the next
        micro version.

        People mostly dont expect this.

      o When installing in developer mode, i.e. with `pip3 install --user -e .`,
        then we were always picking the generated version at install time
        and never again dynamically resolving it.

        Many of our users install this way and update through git, so it's
        important that we report more precise versions all the time.
...

I think we no longer really support developer mode installs with pip install -e, but still not sure that I have full trust in setuptools-scm compared to versioneer.

Callek commented 1 year ago

To be fair, it looks like this is not the right way to integrate versioneer anymore either. https://github.com/python-versioneer/python-versioneer

Vendoring is still supported, though the discussion above about ConfigParser/SafeConfigParser was already addressed. See my reply in the upstream ticket https://github.com/python-versioneer/python-versioneer/issues/375#issuecomment-1685359685

nanonyme commented 1 year ago

@Callek thanks for clarification. The fact that vendored versioneer apparently hasn't been updated for six years does though imply something in process needs to change.

nanonyme commented 1 year ago

Looks like in fact versioneer dropped Python 3.6 support back in 2022

gtristan commented 1 year ago

Looks like in fact versioneer dropped Python 3.6 support back in 2022

Python 3.6 afaik is EOL and near impossible to support for us.

To be fair, it looks like this is not the right way to integrate versioneer anymore either. https://github.com/python-versioneer/python-versioneer

Vendoring is still supported, though the discussion above about ConfigParser/SafeConfigParser was already addressed. See my reply in the upstream ticket python-versioneer/python-versioneer#375 (comment)

When I updated versioneer earlier this year feb494093cbdcf29def02074ec707404d112235a, I recall trying the non-vendored approach, and maybe this is still incomplete, it's somewhat of an unclear disaster involving python projects generally moving to python build and pyproject.toml while BuildStream still requires setup.py.

nanonyme commented 1 year ago

Looks like in fact versioneer dropped Python 3.6 support back in 2022

Python 3.6 afaik is EOL and near impossible to support for us.

Yes, I just mentioned for completeness because you were commenting about this earlier.

nanonyme commented 1 year ago

When I updated versioneer earlier this year feb4940, I recall trying the non-vendored approach, and maybe this is still incomplete, it's somewhat of an unclear disaster involving python projects generally moving to python build and pyproject.toml while BuildStream still requires setup.py.

Something does not make sense here. Upstream said we have 0.18 vendored which is six years old. How is it possible you updated earlier this year? Did you update to wrong thing?

abderrahim commented 1 year ago

When I updated versioneer earlier this year feb4940, I recall trying the non-vendored approach, and maybe this is still incomplete, it's somewhat of an unclear disaster involving python projects generally moving to python build and pyproject.toml while BuildStream still requires setup.py.

Something does not make sense here. Upstream said we have 0.18 vendored which is six years old. How is it possible you updated earlier this year? Did you update to wrong thing?

The commit above updates it from 0.18 to 0.28, but it's only in master, not in any released version.

nanonyme commented 1 year ago

@abderrahim the file in master still has this bug so I'm still confused.

gtristan commented 1 year ago

@abderrahim the file in master still has this bug so I'm still confused.

I may have created a frankeneer with my last update.

My commit did replace the generated file at src/buildstream/_version.py, but failed to update/remove the versioneer.py thing at the root, which might be supposed to go away with version 0.28.

Indeed... and so it looks like I had taken the versioneer install --no-vendor approach, and neglected to delete the required versioneer.py vendored file at the toplevel beside setup.py.

I should also say that... versioneer install --no-vendor does not make a vendorless installation, instead - it continues to vendor the file at src/buildstream/_version.py, instead of injecting these bits at setup.py build time as one might expect - so, I'm not sure what this means (perhaps it should be invoked as versioneer install --vendor-a-bit-less ?).

The toplevel versioneer.py module is a hard requirement because it solves the problem we had where some users had cloned BuildStream from git, and BuildStream did not have any checks to ensure that the user had obtained the tags (which is usually default, but sometimes people manage to get a git clone of BuildStream and don't have the required release tags, and then need to run git fetch --tags).

Other than this, the version needs to be known in order to call setup() in setup.py, and we also chain the build commands in with the cython custom build commands added in setup.py.

So, maybe the right way is to versioneer install --vendor, lets experiment with this...

gtristan commented 1 year ago

Not sure if this will pass CI correctly, but tried it locally with pip3 install . and seems to work: https://github.com/apache/buildstream/pull/1863

This uses the supported versioneer install --vendor approach, please let me know if there are issues with this :)

(note that as far as config parsing goes, configparser.ConfigParser() appears to be used with the updated versioneer.py).

nanonyme commented 1 year ago

(note that as far as config parsing goes, configparser.ConfigParser() appears to be used with the updated versioneer.py).

This is in line with what @Callek said earlier so I'm satisfied with that.

mbridon commented 1 year ago

Very cool.

Can we have a release so I can update the Fedora package to it and see how it fails to build in Rawhide ? :stuck_out_tongue_winking_eye:

Or how can I make a release myself to try it out ? :thinking:

gtristan commented 1 year ago

I mean to propose a release very soon (with apache we now have a release voting process yay...), just want to get #1862 in there first...

nanonyme commented 1 year ago

I strongly suggest rolling a tarball yourself and testing with Rawhide while waiting for the release. In case it still will not work, it means another release behind voting process.

mbridon commented 1 year ago

I strongly suggest rolling a tarball yourself and testing with Rawhide while waiting for the release. In case it still will not work, it means another release behind voting process.

Alright, I did just that.

From the latest git master (with updated versioneer) I ran python setyup.py sdist which generated BuildStream-2.0.1+32.gb2b12b08a.tar.gz.

I tried to build the Rawhide package with that, but then that tarball doesn't contain the same tree as the official one. :confused:

I tried to work around that, and then the build process failed because I hadn't added python3-Cython as a build requirement. I'm confused, because I can't find that in any of the various requirements files, where was this added?

Then it failed because the gcc command was not found... WAT? :scream: Pretty sure that's one of the packages installed by default in any mock build... :confused: Anyway, added it explicitly...

Now the build fails like this:

/builddir/build/BUILD/BuildStream-2.0.1+32.gb2b12b08a/doc/bst2html.py:42: SyntaxWarning: invalid escape sequence '\d'
  ANSI2HTML_CODES_RE = re.compile('(?:\033\\[(\d+(?:;\d+)*)?([cnRhlABCDfsurgKJipm]))')
Traceback (most recent call last):
  File "/builddir/build/BUILD/BuildStream-2.0.1+32.gb2b12b08a/doc/bst2html.py", line 36, in <module>
    from buildstream import _yaml
ModuleNotFoundError: No module named 'buildstream'
make: *** [Makefile:120: sessions/autotools.run] Error 1

That's weird... I wonder if that's not all a side effect of the fact I didn't create the tarball the right way... So before I continue, how was I supposed to create it ? :smile:

gtristan commented 1 year ago

I strongly suggest rolling a tarball yourself and testing with Rawhide while waiting for the release. In case it still will not work, it means another release behind voting process.

Alright, I did just that.

From the latest git master (with updated versioneer) I ran python setyup.py sdist which generated BuildStream-2.0.1+32.gb2b12b08a.tar.gz.

I tried to build the Rawhide package with that, but then that tarball doesn't contain the same tree as the official one. 😕

I tried to work around that, and then the build process failed because I hadn't added python3-Cython as a build requirement. I'm confused, because I can't find that in any of the various requirements files, where was this added?

This is now defined in pyproject.toml

In theory, https://docs.buildstream.build/2.0/main_install.html should be helpful here... if it's not, then let's try to keep it up to date :)

Then it failed because the gcc command was not found... WAT? 😱 Pretty sure that's one of the packages installed by default in any mock build... 😕 Anyway, added it explicitly...

Now the build fails like this:

/builddir/build/BUILD/BuildStream-2.0.1+32.gb2b12b08a/doc/bst2html.py:42: SyntaxWarning: invalid escape sequence '\d'
  ANSI2HTML_CODES_RE = re.compile('(?:\033\\[(\d+(?:;\d+)*)?([cnRhlABCDfsurgKJipm]))')
Traceback (most recent call last):
  File "/builddir/build/BUILD/BuildStream-2.0.1+32.gb2b12b08a/doc/bst2html.py", line 36, in <module>
    from buildstream import _yaml
ModuleNotFoundError: No module named 'buildstream'
make: *** [Makefile:120: sessions/autotools.run] Error 1

That's weird... I wonder if that's not all a side effect of the fact I didn't create the tarball the right way... So before I continue, how was I supposed to create it ? 😄

Aha, so this is strictly for building the docs - which we normally do in CI via tox -e docs (which I understand you cannot do because of the requirement of not having the internet...).

In this specific case, it looks like this is due to buildstream core not being installed at the time we are building docs, a condition which is satisfied by running the docs build in tox.

Possibly this can be worked around with buildstream built but not installed with some PYTHONPATH trickery, I haven't tried this.

mbridon commented 1 year ago

Well, that doc page tells me how to install buildstream (which I already knew how), but not how to generate a tarball :sweat_smile:

mbridon commented 1 year ago

So, buildstream still fails in Rawhide :

+ PYTEST_XDIST_AUTO_NUM_WORKERS=8
+ /usr/bin/pytest -vv -k 'not test_push_pull and not test_push_pull_all and not test_pull_secondary_cache and not test_push_pull_specific_remote and not test_push_pull_non_strict and not test_push_pull_track_non_strict and not test_push_pull_cross_junction and not test_pull_missing_blob and not test_pull_access_rights and not test_push and not test_push_all and not test_push_after_pull and not test_artifact_expires and not test_artifact_too_large and not test_recently_pulled_artifact_does_not_expire and not test_push_cross_junction and not test_push_already_cached and not test_project_error' --ignore=tests/frontend/mirror.py --ignore=tests/sources/git.py --ignore=tests/sources/remote.py --ignore=tests/sources/tar.py --ignore=tests/sources/zip.py --ignore=tests/testutils/file_server.py --ignore=tests/testutils/ftp_server.py --ignore=tests/frontend/compose_splits.py --ignore=tests/frontend/overlaps.py --ignore=tests/plugins/filter.py --ignore=tests/sources/deb.py --ignore=tests/sources/pip.py
ImportError while loading conftest '/builddir/build/BUILD/BuildStream-2.0.1+32.gb2b12b08a/tests/conftest.py'.
tests/conftest.py:23: in <module>
    from buildstream._testing import register_repo_kind, sourcetests_collection_hook
../../BUILDROOT/buildstream-2.0.1-1.fc40.x86_64/usr/lib64/python3.12/site-packages/buildstream/__init__.py:34: in <module>
    from .source import Source, SourceError, SourceFetcher
../../BUILDROOT/buildstream-2.0.1-1.fc40.x86_64/usr/lib64/python3.12/site-packages/buildstream/source.py:228: in <module>
    from ._loader.metasource import MetaSource
../../BUILDROOT/buildstream-2.0.1-1.fc40.x86_64/usr/lib64/python3.12/site-packages/buildstream/_loader/__init__.py:19: in <module>
    from .loadelement import LoadElement, Dependency, DependencyType
src/buildstream/_loader/loadelement.pyx:19: in init buildstream._loader.loadelement
    from pyroaring import BitMap, FrozenBitMap  # pylint: disable=no-name-in-module
E   ModuleNotFoundError: No module named 'pyroaring'

I find this in the buildstram source code from pyroaring import BitMap, which makes me think this is the package I need someone to add to Fedora : roaring, preferably not me, because I've been trying to reduce the number of packages I maintain and I'd really like to keep things that way :smile:

So first, can you confirm this is the right package ? If so, I'll start searching for someone to maintain it :wink:

mbridon commented 1 year ago

Actually, it might be this one instead... pyroaring :confused:

nanonyme commented 1 year ago

Latter looks correct

nanonyme commented 1 year ago

@mbridon indeed https://gitlab.com/freedesktop-sdk/freedesktop-sdk/-/blob/master/elements/components/python3-pyroaring.bst?ref_type=heads#L19