rpkg

#361 Add flatpak-build subcommand
Merged by cqi. Opened by otaylor.
otaylor/rpkg flatpak-build  into  master

Download 361.patch

The flatpak-build subcommand starts a build of the current git repository as a Flatpak. It's pretty much identical to container-build, but the build target is determined by looking up the module that will be built into the flatpak in container.yaml and then determining what platform version that uses by traversing module dependencies.

Missing pygobject in pypi.txt

It would be good to list libmodulemd in fedora-py[23].txt as well.

What's the reason to use assert here? If the req_stream_list is expected to have only one stream, should it raise a rpkgError from here?

In the first call to _find_platform_stream, there could be version passed. But, here version is not pass anymore. Is this expected? What cases make it unnecessary to pass version argument in recursive calls?

I'm thinking if these two for-loops could be merged into one, e.g.

for dep in modules[0].props.dependencies:
    if 'platformat' in dep.props.requires:
        # Do what in the first loop
    else:
        # Do what in the second loop

This merge would be much straightforward to understand the logic than reading through all lines of two nested for-loops.

So, the logic is if a module depends on platform, just return the platform stream, otherwise let's go through each dependency recursively to find out the possible dependent platform stream.

Is my understand correct?

I'm a little worried about the performance of this method due to the recursive calls.

What would be the data size of module's dependencies and the depth of module dependencies generally?

At least in my understanding, the ModuleMD for a module that has been built in MBS should already have streams expanded - that means that there should be exactly one stream in the stream list for the build. If this is not true, the assumptions this code is making is wrong, hence the assert. But there would be no problem making it a more verbose error message - I'll update the patch.

It's possible, though not particularly useful, to put a particular version (within a stream) in the container.yaml to build the Flatpak out of a particular build of a module, rather than just using the latest from a stream. Since this is supported by OSBS when building the container, it makes sense to support it here. But when we are traversing the dependencies of the module, dependencies are only specified by stream, and not by version, so we'll never have a version to pass in.

The reasoning for the double loops is to avoid recursing and making another network round trip when we already have a 'platform' requirement directly for this module. I think this is worthwhile from a performance point of view, and will add an explanatory comment.

The typical situation is:
application module => flatpak-runtime module => platform

So two modulemd's are queried from the network and then platform is found. This surprisingly takes a couple of seconds (MBS may need some optimization) but it's certainly not a big issue when the build that is being started will take ~5 minutes.

Can you follow the subparser register style of build, chain-build and scratch-build?

That is to define common arguments in a separate method, and register container-build and flatpak-build separately with the common arguments.

it's certainly not a big issue when the build that is being started will take ~5 minutes.

Sorry, what does it mean?

I mean that flatpak-build is a long running operation (like other builds),
so if looking up the target via the module build server takes a couple of
seconds, that's not going to annoy the user very much. In other contexts a
couple of seconds could be considered a long time.

On Tue, Aug 7, 2018, 11:32 AM Chenxiong Qi pagure@pagure.io wrote:

cqi commented on the pull-request: Add flatpak-build subcommand that you
are following:
``

it's certainly not a big issue when the build that is being started will
take ~5 minutes.

Sorry, what does it mean?
``

To reply, visit the link below or just reply to this email
https://pagure.io/rpkg/pull-request/361

When I run it locally several times, this test takes 3 seconds in average.
Edited by cqi

The tests should be really fast, since they don't do any network traffic.
What I'm talking about taking time is the actual build in OSBS that is
started via Koji. I hope that makes sense. Sorry for the confusion! Do you
still have concerns about the performance of the target lookup?

On Tue, Aug 7, 2018, 11:46 AM Chenxiong Qi pagure@pagure.io wrote:

cqi commented on the pull-request: Add flatpak-build subcommand that you
are following:
When I ran it locally several times, this test takes 3 seconds in average.

To reply, visit the link below or just reply to this email
https://pagure.io/rpkg/pull-request/361

On 08/07/2018 05:51 PM, Owen Taylor wrote:

otaylor commented on the pull-request: Add flatpak-build subcommand that you are following:
``
The tests should be really fast, since they don't do any network traffic.
What I'm talking about taking time is the actual build in OSBS that is
started via Koji. I hope that makes sense. Sorry for the confusion! Do you
still have concerns about the performance of the target lookup?

The target lookup traverses through built modules. I'm actually confused
why you mentioned the flatpak build could take long time to build. I'm
not clear the connection between your explanation and target lookup
connection. :)

It's ok at this moment. It's not a blocker for us to move forward.

On Tue, Aug 7, 2018, 11:46 AM Chenxiong Qi pagure@pagure.io wrote:

cqi commented on the pull-request: Add flatpak-build subcommand that you
are following:
When I ran it locally several times, this test takes 3 seconds in average.

To reply, visit the link below or just reply to this email
https://pagure.io/rpkg/pull-request/361

``

To reply, visit the link below or just reply to this email
https://pagure.io/rpkg/pull-request/361

--
Regards,
Chenxiong Qi

One comment: can the dependency on libmodulemd be made optional and raise an error if a flatpak build is attempted without it? Otherwise all users of rpkg will be forced to have it available, which could be difficult. For example rhpkg on RHEL 6 would break, since getting libmodulemd is not really possible there. People wanting to build flatpaks are probably using something slightly newer anyway, so it would not limit them.

While it might be possible to install pygobject from pypy, there would be lots of -devel package requirements, and I think it's better to install it from rpms. So I've added it to fedora-py[23].txt instead.

rebased onto 9005b541460de69eac483ec4c71a439d6f240f72

1 new commit added

  • Make the libmodulemd import optional

OK, I repushed my changes with (I think) all of @cqi's comments addressed, then added a commit on top to make the libmodulemd import optional as suggested by @lsedlar

While it might be possible to install pygobject from pypy, there would be lots of -devel package requirements, and I think it's better to install it from rpms. So I've added it to fedora-py[23].txt instead.

fedora-py[23].txt does not aim to list additional dependent packages for pypi.txt. It lists all packages required only for users who want to install from Fedora packages rather than PyPI.

All dependent Python packages should be added to pypi.txt, which is for many things for rpkg to be a Python package. -devel packages have to be installed from distro package repositories, which is a common case and acceptable.

Why write container.yaml here? Tests under tests/commands/ do not test flatpak and container build.

Hi @otaylor thanks for updating this PR. Generally it looks good to me. But still have issues, can you please fix them as well?

  • As my comment mentioned, pygobject should be listed in pypi.txt
  • You have commit which is not signed off

fedora-py[23].txt does not aim to list additional dependent packages for pypi.txt. It lists all packages required only for users who want to install from Fedora packages rather than PyPI.
All dependent Python packages should be added to pypi.txt, which is for many things for rpkg to be a Python package. -devel packages have to be installed from distro package repositories, which is a common case and acceptable.

OK, will add.

Why write container.yaml here? Tests under tests/commands/ do not test flatpak and container build.

Oops, forgot to add a file! - the tests of the new logic in pyrpkg.__init__.py mostly was meant to be in tests/commands/test_flatpak_build.py, which is why I added additional set up in tests/commands/__init__.py.

Will push a new build with the pypi.txt addition, the missing file, and the missing sign-off.
Edited by otaylor

3 new commits added

  • Make the libmodulemd import optional
  • Add flatpak-build subcommand
  • Don't pass the MBS API URL around as a parameter

Oops, forgot to add a file! - the tests of the new logic in pyrpkg.init.py mostly was meant to be in tests/commands/test_flatpak_build.py, which is why I added additional set up in tests/commands/init.py.

I see. test_cli.py is the right place to write tests for commands, like flatpak-build. tests/commands/ includes legacy tests that haven't been rewritten.

Pretty please pagure-ci rebuild

As far as I could see,tests_cli.py is testing the logic in cli.py - and the tests there basically just check that the methods on Commands get called with the right arguments. What I put into tests/commands/test_flatpak_build.py is more like the tests in tests/tests_commands.py but I thought it was better to keep it in a separate file. I can either merge it into test_commands.pyor I can make a separate file directly under tests/ - tests/test_flatpak.py. What do you think is better?

I see. test_cli.py is the right place to write tests for commands, like flatpak-build. tests/commands/ includes legacy tests that haven't been rewritten.

I just see file tests/commands/test_flatpak_build.py, which tests relative methods in Commands. So, please move those test to test_commands.py, or at least inherit from new CommandTestCase in tests/utils.py. It has some convenient methods to help you create git repos and make instance of Commands.

So, please move those test to test_commands.py, or at least inherit from new CommandTestCase in tests/utils.py. It has some convenient methods to help you create git repos and make instance of Commands.

OK, I'll look at that tomorrow.

Jenkins job is updated and tests succeed to start. Some new tests fail.

rebased onto 819e78a473568a77825d8172ef3bff32e19cc26d

4 new commits added

  • Make the libmodulemd import optional
  • Add flatpak-build subcommand
  • TestContainerBuildWithKoji: tear down the mock appropriately
  • Don't pass the MBS API URL around as a parameter

I moved tests/commands/test_flatpak_build.py to tests/test_flatpak_build.py and changed it to depend on utils.CommandTestCase - if you would like it merged into test_commands.py for consistency let me know.

Hopefully the last push will also fix the test failures - it changes the usage of libmodulemd to avoid some constructs that have been problematical in past versions of libmodulemd. I'm not sure which version of libmodulemd the tests ran against - they work fine with the latest version in f27 and f28.

pretty please pagure-ci rebuild.

4 new commits added

  • Make the libmodulemd import optional
  • Add flatpak-build subcommand
  • TestContainerBuildWithKoji: tear down the mock appropriately
  • Don't pass the MBS API URL around as a parameter

One last round of flake8 fixes and the tests pass!

I moved tests/commands/test_flatpak_build.py to tests/test_flatpak_build.py and changed it to depend on utils.CommandTestCase - if you would like it merged into test_commands.py for consistency let me know.

It's fine to it here at this moment. I've been considering to split test_commands.py and test_cli.py actually, which already have many tests.

rpkg has to run with Python 2.6. So, we have to use compatible format syntax. Here, {} should be replaced with {0}, and others.

Tests fail without libmodulemd is installed, which is not installed as BuildRequires for building EL6 package. These tests could probably be skipped if libmodulemd is not installed.

======================================================================
ERROR: test_find_target (test_flatpak_build.FlatpakBuildCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/builddir/build/BUILD/rpkg-1.55/tests/test_flatpak_build.py", line 246, in test_find_target
    assert self.cmd.flatpak_build_target == 'f28-flatpak-candidate'
  File "/builddir/build/BUILD/rpkg-1.55/pyrpkg/__init__.py", line 908, in flatpak_build_target
    self.load_flatpak_build_target()
  File "/builddir/build/BUILD/rpkg-1.55/pyrpkg/__init__.py", line 990, in load_flatpak_build_target
    platform_stream = self._find_platform_stream(name, stream, version=version)
  File "/builddir/build/BUILD/rpkg-1.55/pyrpkg/__init__.py", line 931, in _find_platform_stream
    objects = Modulemd.objects_from_string(mmd_str)
NameError: global name 'Modulemd' is not defined
======================================================================
ERROR: test_find_target_version (test_flatpak_build.FlatpakBuildCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/builddir/build/BUILD/rpkg-1.55/tests/test_flatpak_build.py", line 250, in test_find_target_version
    assert self.cmd.flatpak_build_target == 'f28-flatpak-candidate'
  File "/builddir/build/BUILD/rpkg-1.55/pyrpkg/__init__.py", line 908, in flatpak_build_target
    self.load_flatpak_build_target()
  File "/builddir/build/BUILD/rpkg-1.55/pyrpkg/__init__.py", line 990, in load_flatpak_build_target
    platform_stream = self._find_platform_stream(name, stream, version=version)
  File "/builddir/build/BUILD/rpkg-1.55/pyrpkg/__init__.py", line 931, in _find_platform_stream
    objects = Modulemd.objects_from_string(mmd_str)
NameError: global name 'Modulemd' is not defined
======================================================================
ERROR: test_flatpak_build (test_flatpak_build.FlatpakBuildCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/builddir/build/BUILD/rpkg-1.55/tests/test_flatpak_build.py", line 286, in test_flatpak_build
    self.cmd.container_build_koji(nowait=True, flatpak=True)
  File "/builddir/build/BUILD/rpkg-1.55/pyrpkg/__init__.py", line 2887, in container_build_koji
    raise rpkgError("libmodulemd is required to build Flatpaks")
rpkgError: libmodulemd is required to build Flatpaks
======================================================================
FAIL: test_find_target_bad_modulemd (test_flatpak_build.FlatpakBuildCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/builddir/build/BUILD/rpkg-1.55/tests/test_flatpak_build.py", line 276, in test_find_target_bad_modulemd
    self.module_failure(['bad-modulemd:f28'], "Failed to load modulemd")
  File "/builddir/build/BUILD/rpkg-1.55/tests/test_flatpak_build.py", line 257, in module_failure
    self.assertIn(exception_str, str(e.exception))
AssertionError: 'Failed to load modulemd' not found in "global name 'Modulemd' is not defined"
    """Fail immediately, with the given message."""
>>  raise self.failureException('\'Failed to load modulemd\' not found in "global name \'Modulemd\' is not defined"')
======================================================================
FAIL: test_find_target_no_platform (test_flatpak_build.FlatpakBuildCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/builddir/build/BUILD/rpkg-1.55/tests/test_flatpak_build.py", line 282, in test_find_target_no_platform
    self.module_failure(['nodeps:f28'], "Unable to find 'platform' module in the dependencies")
  File "/builddir/build/BUILD/rpkg-1.55/tests/test_flatpak_build.py", line 257, in module_failure
    self.assertIn(exception_str, str(e.exception))
AssertionError: "Unable to find 'platform' module in the dependencies" not found in "global name 'Modulemd' is not defined"
    """Fail immediately, with the given message."""
>>  raise self.failureException('"Unable to find \'platform\' module in the dependencies" not found in "global name \'Modulemd\' is not defined"')
======================================================================
FAIL: test_find_target_unexpected (test_flatpak_build.FlatpakBuildCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/builddir/build/BUILD/rpkg-1.55/tests/test_flatpak_build.py", line 279, in test_find_target_unexpected
    self.module_failure(['unexpanded:f28'], "stream list for 'platform' is not expanded")
  File "/builddir/build/BUILD/rpkg-1.55/tests/test_flatpak_build.py", line 257, in module_failure
    self.assertIn(exception_str, str(e.exception))
AssertionError: "stream list for 'platform' is not expanded" not found in "global name 'Modulemd' is not defined"
    """Fail immediately, with the given message."""
>>  raise self.failureException('"stream list for \'platform\' is not expanded" not found in "global name \'Modulemd\' is not defined"')
Edited by cqi

The tests should definitely just be skipped. Do you have a vagrantfile or Dockerfile that you use to run tests with EL6?

Not yet. I just run tests locally inside a Python virtualenv with tox. python26 has to be installed. Do you have any problem in this way?

I think those failure tests should be skipped if libmodulemd is not available as in EL6 libmodulemd will not be a BuildRequires in the SPEC.

I think those failure tests should be skipped if libmodulemd is not available as in EL6 libmodulemd will not be a BuildRequires in the SPEC.

Just realize that this is not correct. Instead, the skip criteria should be "if python version 2.6". Because, generally, libmodulemd is installed in developer's system, those tests should always run except with python26 interpreter.

Hi @otaylor, there is actually an issue to run tests with Python 2.6 with tox. I just created a PR https://pagure.io/rpkg/pull-request/378, FYI.

I think those failure tests should be skipped if libmodulemd is not available as in EL6 libmodulemd will not be a BuildRequires in the SPEC.

Just realize that this is not correct. Instead, the skip criteria should be "if python version 2.6". Because, generally, libmodulemd is installed in developer's system, those tests should always run except with python26 interpreter.

You could do it as skip if "import libmodulemd" fails. But I went with your suggestion of sys.version_info >= (2, 7, 0) as more robust - I don't want to start skipping the flatpak tests if libmodulemd imports happen to start failing.

I had ended up with removing PyGObject from test-pypi.txt and in tox.ini

[testenv]
deps =
     rpm-py-installer
     PyGObject
     -r{toxinidir}/requirements/test-pypi.txt
[testenv:py26]
deps =
    rpm-py-installer
    -r{toxinidir}/requirements/test-pypi.txt

Since PyGObject doesn't compile with the Python 2.6 headers. Is there a better way to handle this?

rebased onto b0827ff493c4717586ff040b903261fe0ed52b8f

Pretty please pagure-ci rebuild

Hi @otaylor, there is actually an issue to run tests with Python 2.6 with tox. I just created a PR https://pagure.io/rpkg/pull-request/378, FYI.

As an alternative, is it possible to change the Jenkins environment to use the system python2-virtualenv package? It looks like the F27/F28 Fedora python-virtualenv-1.16 was patched to add back python2.6 support:

https://bodhi.fedoraproject.org/updates/FEDORA-2018-2c7e519668

(Was wondering since it seemed to work fine here.)

As an alternative, is it possible to change the Jenkins environment to use the system python2-virtualenv package? It looks like the F27/F28 Fedora python-virtualenv-1.16 was patched to add back python2.6 support:
https://bodhi.fedoraproject.org/updates/FEDORA-2018-2c7e519668
(Was wondering since it seemed to work fine here.)

I filed a ticket[1] to upgrade python2-virtualenv in Jenkins F27 slave.

[1] https://pagure.io/fedora-infrastructure/issue/7200

It is still painful to handle Python 2.6 compatibility. PyGObject had dropped Python 2.6 support since 3.9.1, that was 5 years ago. Meanwhile, libmodulemd is not available in EL6 and we have decided in above comments to skip something when that happens. Now, we have to deal with PyGObject again.

I even tried to install PyGObject from VCS link, unfortunately, looks the code base is too old:

pip install -e "git+https://gitlab.gnome.org/GNOME/pygobject.git@3.8.1#egg=PyGObject"
DEPRECATION: Python 2.6 is no longer supported by the Python core team, please upgrade your Python. A future version of pip will drop support for Python 2.6
Obtaining PyGObject from git+https://gitlab.gnome.org/GNOME/pygobject.git@3.8.1#egg=PyGObject
  Cloning https://gitlab.gnome.org/GNOME/pygobject.git (to 3.8.1) to ./.py26env/src/pygobject
    Complete output from command python setup.py egg_info:
    Traceback (most recent call last):
      File "<string>", line 1, in <module>
    IOError: [Errno 2] No such file or directory: '/home/cqi/code/rpkg/.py26env/src/pygobject/setup.py'
    ----------------------------------------
Command "python setup.py egg_info" failed with error code 1 in /home/cqi/code/rpkg/.py26env/src/pygobject/

Current situation is command flatpak-build requires libmodulemd and PyGObject is required to load that module and access APIs. The former one is not available in EL6 (where Python 2.6 is the default interpreter), which causes flatpak-build command is not usable in EL6, so I think it makes no sense to import PyGObject as well.

So, some comments based on that:

  • define a module level variable PY26
  • if not PY26, do not register flatpak-build command
  • if not PY26, skip corresponding tests (this is already done in test_flatpak_build.py)
  • in __init__.py, if not PY26, do not import yaml and gi, then do not load Modulemd
  • HAVE_LIBMODULEMD can be saved, since command is not registered and argument flatpak of container_build_koji is always False

These updates should make the code much clear and easier for package maintainers to know which dependent packages should be dropped for EL6.

Meanwhile, the variable PY26 could be reused in other places.

Sorry for taking your time again to update this PR. The idea in this comment I had in mind emerged when I ran tests locally with Python 2.6. Thank you very much.

4 new commits added

  • Don't registry flatpak-build command on Python-2.6
  • Add flatpak-build subcommand
  • TestContainerBuildWithKoji: tear down the mock appropriately
  • Don't pass the MBS API URL around as a parameter

I even tried to install PyGObject from VCS link, unfortunately, looks the code base is too old:

Yeah, getting that working is not realistic.

define a module level variable PY26 ... if not PY26, do not register flatpak-build command ...

That sounds a little cleaner than my approach. I've updated the patch that way. Does it look like what you imagined?

Are there any remaining concerns with the PR?

This has to be added to setup.py, otherwise it will fail while provisioning py26 testenv.

EDIT:

Sorry, please ignore this comment.
Edited by cqi

Cool :thumbsup: I succeeded to schedule a scratch build https://koji.stg.fedoraproject.org/koji/taskinfo?taskID=90001748

No other comments except rebase and fixing conflicts. Thank you very much.

rebased onto 5faef228f299ddd84c0e950ba2364a030a50b513

Rebased - tox.ini required splitting the [testenv]deps variable into deps and base_deps. Please check you are happy with how I did that.

Looks good to me :thumbsup:

@otaylor changes to tox.ini is ok for me. I have another PR opened, that refactors tox.ini.

Thank you very much again.

The comment should probably either mention flatpak builds or be removed.

Typo here: flaptaks -> flatpaks
but it's good to see I'm not the only person making this one :smile:

Nothing serious that would need to be fixed! Thanks for the patch.
Looks good!

why not raise rpkgError("Cannot find any builds for module %s", nsvc) ?

Thanks for the reviews @lsedlar and @onosek! - I've pushed a new version with those fixes.

I know that similar testing scenario is used also in other tests, but what about placing so large input data to external files somehow? But I do not require it.

4 new commits added

  • Don't registry flatpak-build command on Python-2.6
  • Add flatpak-build subcommand
  • TestContainerBuildWithKoji: tear down the mock appropriately
  • Don't pass the MBS API URL around as a parameter

After code-review I went further and tried to build rpkg package. I am not sure whether it is related to the code or not ...
rpkg.spec:

...
%if 0%{?rhel}
BuildRequires:  PyYAML
BuildRequires:  python-gobject-base
BuildRequires:  libmodulemd
...
%if 0%{?rhel}
Requires:       PyYAML
Requires:       python-gobject-base
Requires:       libmodulemd
...

It failed. @cqi, you can check in brew
Edited by onosek

The failure there: 

 test_find_target (test_flatpak_build.FlatpakBuildCase) ... /usr/bin/python: symbol lookup error: /lib64/libmodulemd.so.1: undefined symbol: g_log_structured_standard

Means that libmodulemd was built against glib2 2.56 or 2.58, while the glib2 version installed in the buildroot is 2.54. This might because of an build override that timed out or something. I don't think it has anything to do with this patch, other than it's using libmodulemd. Feel free to follow up by email.

Thank you, @otaylor for your explanation. Let's say it was a warning for anybody who will do the release and who have to deal with these dependencies.

@onosek I think internal build of libmodulemd or its dependency like glib2 should be dealt with.

Lets't merge. I have no other comments to add.

Pull-Request has been merged by cqi
Metadata
None
No Tags
Static archive. pagure.io was sunset in 2026; this is a read-only snapshot.
Red Hat Logo

Fedora is sponsored by Red Hat. Learn more about the relationship between Red Hat and Fedora »

© Red Hat, Inc. and others.