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.

Sign up for GitHub

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

Jump to bottom

Add Python 3.13, remove Python 3.8 and update CI Workflow #3767

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add Python 3.13, remove Python 3.8 and update CI Workflow #3767

wants to merge 2 commits into from

Conversation

digitalresistor
Copy link
Member

@digitalresistor digitalresistor commented Oct 26, 2024 •
edited
Loading

Takes care of the following things:

  • Updates the CI workflow to include Python 3.13 and testing on a variety of more platforms/including newer ones
  • Updates tox.ini to clean it up syntax wise
  • Updates some tests to make sure they still fail on Python 3.13

Closes #3762 #3761

@digitalresistor
Update tests for Python 3.13, add nocover in pshell
bc84ac3 
Inner functions such as:

    def outer():
        def inner():
            pass

Now have an attribute called `__name__`, which will be set to `inner`.

Also it seems that there is a `__doc__` attribute that is now standard,
and thus we add a pragma nocover to pshell.
@digitalresistor
Copy link
Member Author

This replaces/supplements #3762

@digitalresistor
Copy link
Member Author

Windows support is broken on Py 3.13

@jenstroeger
Copy link
Contributor

Are there any further efforts to move to Python 3.13 and in particular to address the various deprecation warnings?

Probably somewhat related PR Pylons/webob#475 (issue Pylons/webob#473).

@robvdl
Copy link
Contributor

robvdl commented Dec 15, 2024 via email

@jenstroeger
Copy link
Contributor

Just something to be aware of, but dependabot still doesn't support Python 3.13 dependabot/dependabot-core#10828

Huh, I was not aware of that… that would explain why I have’t seen Dependabot PRs in those repos yet 👍🏼

Independently of that, do you have a timeline for merging this PR?

@ulgens
Copy link

ulgens commented Dec 20, 2024

@robvdl @jenstroeger Dependabot has 3.13 support by today.

Also, I'm not sure what are the plans for this PR but if needed, I can help to get it merge faster by reviewing, testing or splitting it into smaller / focused PRs.

@digitalresistor
Copy link
Member Author

@ulgens if you want to help, fixing the errors on Windows is the primary thing right now. Splitting the PR into multiple smaller ones won't help, I have done the bare minimum in this PR to add support for Python 3.13, except that on Windows the tests are now broken:

================================== FAILURES ===================================
__ Test_abspath_from_asset_spec.test_pname_is_None_after_resolve_asset_spec ___

self = <tests.test_asset.Test_abspath_from_asset_spec testMethod=test_pname_is_None_after_resolve_asset_spec>

    def test_pname_is_None_after_resolve_asset_spec(self):
        result = self._callFUT('/abc', '__main__')
>       self.assertEqual(result, '/abc')
E       AssertionError: 'D:\\a\\pyramid\\pyramid\\.tox\\py\\Lib\\site-packages\\pytest\\abc' != '/abc'
E       - D:\a\pyramid\pyramid\.tox\py\Lib\site-packages\pytest\abc
E       + /abc

tests\test_asset.py:61: AssertionError
__________________________ TestCallerPath.test_isabs __________________________

self = <tests.test_path.TestCallerPath testMethod=test_isabs>

    def test_isabs(self):
        result = self._callFUT('/a/b/c')
>       self.assertEqual(result, '/a/b/c')
E       AssertionError: 'D:/a/b/c' != '/a/b/c'
E       - D:/a/b/c
E       ? --
E       + /a/b/c

tests\test_path.py:21: AssertionError
_________________ TestPRoutesCommand.test_route_static_views __________________

self = <tests.test_scripts.test_proutes.TestPRoutesCommand testMethod=test_route_static_views>

    def test_route_static_views(self):
        config = self._makeConfig(autocommit=True)
        config.add_static_view('static', 'static', cache_max_age=3600)
        path2 = os.path.normpath('/var/www/static')
        config.add_static_view(name='static2', path=path2)
        config.add_static_view(
            name='pyramid_scaffold',
            path='pyramid:scaffolds/starter/+package+/static',
        )
    
        command = self._makeOne()
        L = []
        command.out = L.append
        command.bootstrap = dummy.DummyBootstrap(registry=config.registry)
        result = command.run()
        self.assertEqual(result, 0)
        self.assertEqual(len(L), 5)
    
        expected = [
            [
                '__static/',
                '/static/*subpath',
                'tests.test_scripts:static/',
                '*',
            ],
            ['__static2/', '/static2/*subpath', path2 + os.sep, '*'],
            [
                '__pyramid_scaffold/',
                '/pyramid_scaffold/*subpath',
                'pyramid:scaffolds/starter/+package+/static/',
                '*',
            ],
        ]
    
        for index, line in enumerate(L[2:]):
            data = line.split()
>           self.assertEqual(data, expected[index])
E           AssertionError: Lists differ: ['__s[14 chars]atic2/*subpath', 'tests.test_scripts:\\var\\www\\static/', '*'] != ['__s[14 chars]atic2/*subpath', '\\var\\www\\static\\', '*']
E           
E           First differing element 2:
E           'tests.test_scripts:\\var\\www\\static/'
E           '\\var\\www\\static\\'
E           
E           + ['__static2/', '/static2/*subpath', '\\var\\www\\static\\', '*']
E           - ['__static2/',
E           -  '/static2/*subpath',
E           -  'tests.test_scripts:\\var\\www\\static/',
E           -  '*']

tests\test_scripts\test_proutes.py:486: AssertionError
_________________________ Test_static_url.test_it_abs _________________________

self = <tests.test_url.Test_static_url testMethod=test_it_abs>

    def test_it_abs(self):
        request = self._makeRequest()
        result = self._callFUT('/foo/bar/abc', request, _app_url='')
        self.assertEqual(result, 'static url')
>       self.assertEqual(request.path, '/foo/bar/abc')
E       AssertionError: 'tests:/foo/bar/abc' != '/foo/bar/abc'
E       - tests:/foo/bar/abc
E       ? ------
E       + /foo/bar/abc

tests\test_url.py:1175: AssertionError
________________________ Test_static_path.test_it_abs _________________________

self = <tests.test_url.Test_static_path testMethod=test_it_abs>

    def test_it_abs(self):
        request = self._makeRequest()
        result = self._callFUT('/foo/bar/abc', request, _anchor='anchor')
        self.assertEqual(result, 'static path')
>       self.assertEqual(request.path, '/foo/bar/abc')
E       AssertionError: 'tests:/foo/bar/abc' != '/foo/bar/abc'
E       - tests:/foo/bar/abc
E       ? ------
E       + /foo/bar/abc

tests\test_url.py:1212: AssertionError

I don't have a Windows environment to test/validate/figure out what needs to be fixed.

So this can't be merged until the tests are fixed.

@robvdl
Copy link
Contributor

robvdl commented Dec 20, 2024 •
edited
Loading

E AssertionError: 'tests:/foo/bar/abc' != '/foo/bar/abc'
E - tests:/foo/bar/abc

It looks like it is picking up "tests" as if it was a package, hence the colon tests:/path. Is there an __init__.py present in the tests directory perhaps? That can make it think it's a package.

edit: actually that is weird, because it is using a src-based layout! it shouldn't think tests is a package at all, because it is outside of the src directory. Something is off here.

@digitalresistor
Copy link
Member Author

E AssertionError: 'tests:/foo/bar/abc' != '/foo/bar/abc'
E - tests:/foo/bar/abc

It looks like it is picking up "tests" as if it was a package, hence the colon tests:/path. Is there an __init__.py present in the tests directory perhaps? That can make it think it's a package.

edit: actually that is weird, because it is using a src-based layout! it shouldn't think tests is a package at all, because it is outside of the src directory. Something is off here.

Why wouldn't that be an issue on Linux/macOS too though?

@robvdl
Copy link
Contributor

robvdl commented Dec 21, 2024

Why wouldn't that be an issue on Linux/macOS too though?

Exactly, that is why I said "something seems off".

One thing I don't understand why Pyramid has both a setup.py + setup.cfg AND a pyproject.toml

Normally when I modernise a project the point is to delete the setup.py and replace it with a pyproject.toml, could it be that the fact it has two (setup.py AND pyproject.toml) is confusing things?

@digitalresistor
Copy link
Member Author

setup.py was necessary for pip install -e to still be viable, not sure if that has been fixed, but for the longest time pip did not allow editable installs without it. I am not sure if that is still the case, but on almost all of our projects we still ship setup.py if we are using setup tools. Pyramid is not unique in that regard.

Mixing and matching pyproject.toml, setup.cfg and setup.py shouldn't matter. All of the issues are after tox has installed the wheel into the environment and is running the test suite.

russellballestrini
russellballestrini approved these changes Dec 21, 2024
View reviewed changes
Copy link
Contributor

@russellballestrini russellballestrini left a comment •
edited
Loading

Choose a reason for hiding this comment

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

If this fixes windows which ships python 3.13 by default, just tested yesterday morning, we should merge sooner than later.

Thank you for the work on this! I skimmed the changes and nothing jumps out at me.

================================== FAILURES ===================================
__ Test_abspath_from_asset_spec.test_pname_is_None_after_resolve_asset_spec ___

self = <tests.test_asset.Test_abspath_from_asset_spec testMethod=test_pname_is_None_after_resolve_asset_spec>

    def test_pname_is_None_after_resolve_asset_spec(self):
        result = self._callFUT('/abc', '__main__')
>       self.assertEqual(result, '/abc')
E       AssertionError: 'D:\\a\\pyramid\\pyramid\\.tox\\py\\Lib\\site-packages\\pytest\\abc' != '/abc'
E       - D:\a\pyramid\pyramid\.tox\py\Lib\site-packages\pytest\abc
E       + /abc

tests\test_asset.py:61: AssertionError
__________________________ TestCallerPath.test_isabs __________________________

self = <tests.test_path.TestCallerPath testMethod=test_isabs>

    def test_isabs(self):
        result = self._callFUT('/a/b/c')
>       self.assertEqual(result, '/a/b/c')
E       AssertionError: 'D:/a/b/c' != '/a/b/c'
E       - D:/a/b/c
E       ? --
E       + /a/b/c

tests\test_path.py:21: AssertionError
_________________ TestPRoutesCommand.test_route_static_views __________________

self = <tests.test_scripts.test_proutes.TestPRoutesCommand testMethod=test_route_static_views>

    def test_route_static_views(self):

self = <tests.test_url.Test_static_url testMethod=test_it_abs>

    def test_it_abs(self):
        request = self._makeRequest()
        result = self._callFUT('/foo/bar/abc', request, _app_url='')
        self.assertEqual(result, 'static url')
>       self.assertEqual(request.path, '/foo/bar/abc')
E       AssertionError: 'tests:/foo/bar/abc' != '/foo/bar/abc'
E       - tests:/foo/bar/abc
E       ? ------
E       + /foo/bar/abc

tests\test_url.py:1175: AssertionError
________________________ Test_static_path.test_it_abs _________________________

self = <tests.test_url.Test_static_path testMethod=test_it_abs>

    def test_it_abs(self):
        request = self._makeRequest()
        result = self._callFUT('/foo/bar/abc', request, _anchor='anchor')
        self.assertEqual(result, 'static path')
>       self.assertEqual(request.path, '/foo/bar/abc')
E       AssertionError: 'tests:/foo/bar/abc' != '/foo/bar/abc'
E       - tests:/foo/bar/abc
E       ? ------
E       + /foo/bar/abc

tests\test_url.py:1212: AssertionError
============================== warnings summary ===============================

can we do a follow up to fix the tests or will the package not build with errors?

@russellballestrini
Copy link
Contributor

I have access to a windows machine, I will try running the tests locally today.

I wonder if D drive is hard coded or if the test runner picks that because it uses D for checkouts?

@digitalresistor thanks again.

@russellballestrini
Copy link
Contributor 

exclude:
  # Temporarily exclude Windows + Python 3.13 due to path handling issues
  - os: "windows-latest"
    py: "3.13"

@digitalresistor thoughts?

@robvdl
Copy link
Contributor

robvdl commented Dec 21, 2024 via email

@emdneto emdneto mentioned this pull request Dec 23, 2024
Open
12 tasks
@digitalresistor
Copy link
Member Author

No, we will not be skipping Windows because tests are failing on that platform.

We need to find out why, since it seems to affect url generation and looks like it would cause issues for people that would want to use Python 3.13 on Windows with Pyramid.

Right now people can use Pyramid on Python 3.13 anyway if they want, we don't limit the upper Python version in our wheels/source dist, we just don't officially support it yet and as can be seen from this PR, it's broken on Windows so that's a good reason to not officially support it until we can be reasonably sure that everything is working as intended.

@russellballestrini
Copy link
Contributor

Thanks for the explaination.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@russellballestrini russellballestrini russellballestrini approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@digitalresistor @jenstroeger @robvdl @ulgens @russellballestrini