search

fedora-infra / mirrormanager2

Rewrite of the MirrorManager application in Flask and SQLAlchemy
https://mirrormanager.fedoraproject.org
GNU General Public License v2.0
63 stars 46 forks source link

Enable MirrorManager2 to be built using Python 3 for Fedora #260

Closed Conan-Kudo closed 5 years ago

Conan-Kudo commented 5 years ago

This changes all the shebangs to an unversioned one that works in venvs for development, while adjusting the spec file to change all the shebangs to the correct Python interpreter during the package build.

Tests are also now run using Python 3 on Fedora, and Python 2 on CentOS 7.

This strategy is similar to what is used for Pagure.

Signed-off-by: Neal Gompa ngompa13@gmail.com

Conan-Kudo commented 5 years ago

I have a test failure only with Python 3 that I don't understand:

=================================== FAILURES ===================================
_____________________________ UMDLTest.test_1_umdl _____________________________
self = <tests.test_umdl.UMDLTest testMethod=test_1_umdl>
    def test_1_umdl(self):
        """ Test the umdl cron. """

        # Fill the DB a little bit
        tests.create_base_items(self.session)
        tests.create_directory(self.session)
        tests.create_category(self.session)
        tests.create_categorydirectory(self.session)

        # Run the UDML

        process = subprocess.Popen(
            args=self.umdl_command.split(),
            stdout=subprocess.PIPE, stderr=subprocess.PIPE,
            universal_newlines=True)
        stdout, stderr = process.communicate()

        self.assertEqual(stdout, '')
        self.assertEqual(stderr, '')

        with open(self.logfile) as stream:
            logs = stream.readlines()
        logs = ''.join([
            log.split(':', 3)[-1]
            for log in logs
        ])

        for i in [
                'N/A:Starting umdl',
                'Fedora Linux: has changed: 0 !=',
                'atomic/21/refs/heads/fedora-atomic/f21/x86_64/updates has',
                'Linux:atomic/rawhide/objects has changed: 0',
                'Fedora Linux:atomic/rawhide/tmp has changed: 0 !=',
                'Fedora Linux:development has changed: 0 !=',
                'Fedora Linux:releases/20/Fedora has changed: 0 !=',
                'Fedora/source/SRPMS/r has changed: 0 !=',
                'releases/20/Fedora/x86_64/iso has changed: 0 !=',
                'releases/20/Live/x86_64 has changed: 0 !=',
                'Fedora Linux:Updating FileDetail <mirrormanager2.lib.model',
                'Created Version(product=<Product(2 - Fedora)>',
                'Created Repository(prefix=atomic-unknown, version=developm',
                'Version(product=<Product(2 - Fedora)>, name=20',
                'Repository(prefix=None, version=20, arch=source',
                'ategory Fedora Secondary Arches does not exist',
                'Fedora Other:Refresh the list of repomd.xml'
        ]:
            self.assertTrue(i in logs)

        # The DB should now be filled with what UMDL added, so let's check it
        results = mirrormanager2.lib.query_directories(self.session)
        self.assertEqual(len(results), 0)

        results = mirrormanager2.lib.get_versions(self.session)
        self.assertEqual(len(results), 3)
>       self.assertEqual(results[0].name, 'development')
E       AssertionError: '21' != 'development'
E       - 21
E       + development
tests/test_umdl.py:172: AssertionError
adrianreber commented 5 years ago

Maybe it is related to your encoding change which results in different ordering in the test database.

hroncok commented 5 years ago

The order on Python 3 is Version(21) first, then Version(development) and Version(20). The test expects a different order.

I guess this is related to dict ordering. The code iterates over dict items and creates the versions inside the DB based on that order. Is that order actually important or is it just a test that assumes the order to test the actual content?

hroncok commented 5 years ago

That is the dict I'm talking about:

for relativeDName, value in category_directories.items():
hroncok commented 5 years ago

Maybe it is related to your encoding change which results in different ordering in the test database.

The items are sorted by their ID. It's the creation order here that has changed.

encukou commented 5 years ago

The id (on which Version is sorted in mirrormanager2.lib.get_versions) is defined as autoincrement (the default for Integer, primary_key=True). I don't think autoincrement guarantees anything but uniqueness. Is using it to sort by "creation order" really OK?

adrianreber commented 5 years ago

I guess this is related to dict ordering. The code iterates over dict items and creates the versions inside the DB based on that order. Is that order actually important or is it just a test that assumes the order to test the actual content?

The order is not important. That is just what worked until now. We probably need to be smarter with out tests here.

adrianreber commented 5 years ago

@Conan-Kudo Thanks for this PR. I have a fix for the test failure you are seeing. The problem with my fix is that it depends on this PR. So I have to merge one PR with test failures anyway.

I will merge this PR now and then open a new PR with fixed tests.

adrianreber commented 5 years ago

@Conan-Kudo I get failures building the RPM:

+ exit 0
Processing files: mirrormanager2-0.9.0-1.fc29.noarch
error: Directory not found: /home/adrian/rpmbuild/BUILDROOT/mirrormanager2-0.9.0-1.fc29.x86_64/usr/lib/python3.7/site-packages/__pycache__
Executing(%doc): /bin/sh -e /var/tmp/rpm-tmp.aT3Gg0
+ umask 022
+ cd /home/adrian/rpmbuild/BUILD
+ cd mirrormanager2-0.9.0
+ DOCDIR=/home/adrian/rpmbuild/BUILDROOT/mirrormanager2-0.9.0-1.fc29.x86_64/usr/share/doc/mirrormanager2
+ export LC_ALL=C
+ LC_ALL=C
+ export DOCDIR
+ /usr/bin/mkdir -p /home/adrian/rpmbuild/BUILDROOT/mirrormanager2-0.9.0-1.fc29.x86_64/usr/share/doc/mirrormanager2
+ cp -pr README.rst /home/adrian/rpmbuild/BUILDROOT/mirrormanager2-0.9.0-1.fc29.x86_64/usr/share/doc/mirrormanager2
+ cp -pr doc/ /home/adrian/rpmbuild/BUILDROOT/mirrormanager2-0.9.0-1.fc29.x86_64/usr/share/doc/mirrormanager2
+ exit 0

RPM build errors:
    Directory not found: /home/adrian/rpmbuild/BUILDROOT/mirrormanager2-0.9.0-1.fc29.x86_64/usr/lib/python3.7/site-packages/__pycache__

What do I need to do to fix this?

Conan-Kudo commented 5 years ago

Oops, it's a simple mistake. https://github.com/fedora-infra/mirrormanager2/blob/a1162c6c4ee6409a0e3af6275117b24fa0469a8f/utility/mirrormanager2.spec#L352

should be:

%dir %{python_sitelib}/%{name}/__pycache__
adrianreber commented 5 years ago

Thanks, one last error:

    Installed (but unpackaged) file(s) found:
   /usr/share/mirrormanager2/__pycache__/mirrormanager2_createdb.cpython-37.opt-1.pyc
   /usr/share/mirrormanager2/__pycache__/mirrormanager2_createdb.cpython-37.pyc

Should these files be included in the RPM or not?

Conan-Kudo commented 5 years ago

They should. After the following line: https://github.com/fedora-infra/mirrormanager2/blob/a1162c6c4ee6409a0e3af6275117b24fa0469a8f/utility/mirrormanager2.spec#L358

Add the following

%if ! (0%{?rhel} && 0%{?rhel} <= 7)
%{_datadir}/mirrormanager2/__pycache__/mirrormanager2_createdb.*.py*
%endif