Replace rpmbuild dependency with new rpmwriter module by giuseppe · Pull Request #1005 · projectatomic/atomic

giuseppe · 2017-05-17T14:48:31Z

Description

Replace rpmbuild dependency with a simpler module to write directly an RPM file.

We lose the capability of building an RPM from a spec file, but this was really an over-engineered solution, so better drop it before it is too late.

Users who want a more complex RPM file, then they must provide it as part of the image itself '/exports/container.rpm'

Related Issue Numbers

#981

giuseppe · 2017-05-17T14:59:57Z

@cgwalters @baude this is the WIP for replacing rpmbuild, I need to add tests

giuseppe · 2017-05-18T15:20:24Z

I've adapted the tests to work with the new module. There are quite a few tests to cover the rpm generation. Dropping the WIP and ready for review

giuseppe · 2017-05-18T15:20:40Z

The CI failure is unrelated to the PR

ashcrow · 2017-05-18T15:30:04Z

+ '[' /var/mnt /sysroot/ostree/deploy/centos-atomic-host/var/mnt ']'
./tests/integration/test_storage.sh: line 39: [: /var/mnt: unary operator expected
+ wipefs -a /dev/vdb
wipefs: error: /dev/vdb: probing initialization failed: Device or resource busy

ashcrow · 2017-05-18T15:40:00Z

Related: ostreedev/ostree#859

baude · 2017-05-18T20:42:53Z

@giuseppe assuming #1008 merges, just rebase and you should be good.

giuseppe · 2017-05-18T21:07:05Z

rebased on master so that the storage test is disabled ⬆️

cgwalters · 2017-05-19T15:22:54Z

How did you come up with rpmwriter.py? Did you just reverse-engineer librpm and translate to Python? A few comments about how you did it would help maintenance.

Down the line...I'd like to change things so that the "synthesize an RPM" code lives in rpm-ostree in that case. Basically have atomic pass the metadata as JSON/GVariant/whatever + path to rootfs.

For now though this seems pretty sane to me.

giuseppe · 2017-05-22T08:50:46Z

@cgwalters I've used the documentation here as a starting point: http://ftp.rpm.org/max-rpm/s1-rpm-file-format-rpm-file-format.html

I got stuck a few times so I added more debug messages to rpm that helped me, but in general the file format documentation + rpmtag.h was enough to write rpmwriter.h. I've added a fixup patch to add more info about it

TomSweeneyRedHat · 2017-05-22T11:20:27Z

Atomic/rpmwriter.py

+    RPMTAG_REQUIRENAME		= 1049
+    RPMTAG_REQUIREVERSION	= 1050
+    RPMTAG_CONFLICTFLAGS	= 1053
+    RPMTAG_CONFLICTNAME		= 1054


micro nit. You've an extra tab it looks like between the variable and the equals for a few of these definitions. I personally like this style and would ask that you do the same for the group of variables above too (lines 40->62)

I've pushed a fixup patch to use the same style for all variables

TomSweeneyRedHat · 2017-05-22T11:22:30Z

Atomic/rpmwriter.py

+
+    def add_header(self, tag, typ, count, value, pad=1):
+        try:
+            # this fails on python3


Should a TODO comment be added to fix in python3?

no, this is the desired behavior. The try inhibits the exception to be raised as there is no unicode in python3

Ah, I read it to mean you were having unexpected issues here. Perhaps change the comment to something like:

"The next line is expected to always fail under python3, in that case just continue."

or something similar. Another nit, not a hold the merge comment.

TomSweeneyRedHat · 2017-05-22T11:42:57Z

Atomic/rpmwriter.py

@@ -0,0 +1,369 @@
+import os


Do you need to have a shebang here declaring the version of python you're supporting? Perhaps:

#!/usr/bin/env python2

Given your python3 issue noted below?

atomic must support both versions. On RHEL we have python2, on Fedora python3

Sounds good, I misunderstood your comment on the python3 failure. Given that it would stlll be good to have some kind of shebang statement at the top of the file. Perhaps:

#!/usr/bin/env python

But certainly not a comment to hold the merge for.

What's the point of having shebang if the file is not meant to be run directly?

ashcrow · 2017-05-22T13:50:14Z

Atomic/rpm_host_install.py

+        rpm_name = "atomic-container-%s" % name
+        rpm_out = os.path.join(result_dir, "%s.rpm" % rpm_name)
+        def split_name_version(pkg):
+            r = r"([abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789\-\._\+]+)(.*)"


nit: This regex could be shortened to ([a-zA-Z0-9_\-\.\+]+)(.*)

thanks, pushed a fixup patch.

ashcrow · 2017-05-22T13:51:41Z

Atomic/rpm_host_install.py

            rpm_file = RPMHostInstall.find_rpm(rpm_root)
            if rpm_file:
-                dest_path = os.path.join(destination, "container.rpm")
+                orig_name = "atomic-container-%s.rpm" % name


nit: I recommend using {} and format with string formatting unless there is a need to hold compatibility with very old Python versions.

change included in the fixup patch

ashcrow · 2017-05-22T13:53:39Z

Atomic/rpmwriter.py

+    def add_conflict(self, name, version):
+        self.conflict.append([name, version])
+
+    def __init__(self, out, root, name, version, release, summary='', description='', license_='gpl2', changelog='', url='', group='', stderr=sys.stderr, whitelist=None):


I ❤️ GPL but why default to gpl2 if no license is provided?

rpm complains if no license is specified, so I've just used the same license of atomic itself

TomSweeneyRedHat · 2017-05-22T14:10:45Z

LGTM, assuming happy test results. Thanks for the nit touch ups.

giuseppe · 2017-05-23T13:02:41Z

@rhatdan what do you think of this change?

rhatdan · 2017-05-23T17:28:18Z

@giuseppe Can this handle dependencies? IE Can I build a "spec" file that says it requires "foobar" package be installed?

giuseppe · 2017-05-23T18:44:58Z

@rhatdan the rpmwriter doesn't use a spec file to build the .rpm file. It creates the cpio archive given a rootfs and the metadata is read from labels and injected in the .rpm:

LABEL atomic.requires=foo>1,bar<2

We support: summary, version, release, license, url, requires, provides, conflicts, description.

I'll document this once the PR looks fine.

@TomasTomecek

TomasTomecek · 2017-05-24T21:12:04Z

Atomic/rpmwriter.py

+# The complete list is available here:
+# https://github.com/rpm-software-management/rpm/blob/master/lib/rpmtag.h
+
+class RpmWriter(object):


My opinion is that this module should be a separate project. If you look at it, it's not tight to atomic in any way. I think that plenty of other users and projects could benefit from a lightweight and friendler user-experience interaction with rpm builds. Even further, it would be awesome if rpm team reviewed this piece of code and ack'd it.

We're not building here though. More like just repacking binaries. What other use cases are there where one isn't using a spec file? Or without wanting a dependency on rpmbuild?

I've created a separate repository here: https://github.com/giuseppe/tinyrpmbuild, if someone is really interested in it.

I agree with @cgwalters, this is not a replacement for rpmbuild, it is really implementing the minimum necessary to get an installable .rpm file. Not sure how many other users will have the same requirement of building an .rpm on the fly without using rpmbuild.

TomasTomecek · 2017-05-24T21:13:59Z

Atomic/rpmwriter.py

+        def try_read(stdout, gzip_out):
+            written = 0
+            while True:
+                readable, _, _ = select.select([stdout], [], [], 0)


Wow, is select needed here? I would assume that simple pipe with reads and writes is just enough.

I want it to not block on _try_read, just read as much as it is available. In this way I can feed more data to the cpio process through cpio_process.stdin.write(filename.encode() + b"\n")

TomasTomecek · 2017-05-24T21:19:06Z

Atomic/rpmwriter.py

+import shutil
+
+# More information on the RPM file format can be found here:
+# http://ftp.rpm.org/max-rpm/s1-rpm-file-format-rpm-file-format.html


Is this spec backwards and forward compatible?

baude · 2017-05-30T17:26:47Z

@rh-atomic-bot r+ 8537474

rh-atomic-bot · 2017-05-30T17:26:47Z

🙀 8537474 is not a valid commit SHA. Please try again with 271ebdb.

baude · 2017-05-30T17:37:15Z

@rh-atomic-bot r+ 271ebdb

rh-atomic-bot · 2017-05-30T17:37:17Z

🔒 Merge conflict

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

Closes: projectatomic#981 Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

Since we don't use it anymore, run all the tests Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

giuseppe · 2017-05-30T18:43:45Z

@baude thanks for the r+, I've manually fixed the conflicts with the fixup patches. Pushed a new version ⬆️

rhatdan · 2017-05-30T19:40:54Z

@rh-atomic-bot retry

rhatdan · 2017-05-30T19:41:30Z

@rh-atomic-bot r+ be51f9c

rh-atomic-bot · 2017-05-30T19:41:35Z

⌛ Testing commit be51f9c with merge 3d1ca35...

Closes: #981 Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> Closes: #1005 Approved by: rhatdan

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> Closes: #1005 Approved by: rhatdan

Since we don't use it anymore, run all the tests Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> Closes: #1005 Approved by: rhatdan

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> Closes: #1005 Approved by: rhatdan

rh-atomic-bot · 2017-05-30T19:59:35Z

☀️ Test successful - status-redhatci
Approved by: rhatdan
Pushing 3d1ca35 to master...

giuseppe force-pushed the replace-rpmbuild-with-rpmwriter branch 8 times, most recently from 464c98a to bc385e9 Compare May 18, 2017 14:17

giuseppe changed the title ~~[WIP] Replace rpmbuild dependency with new rpmwriter module~~ Replace rpmbuild dependency with new rpmwriter module May 18, 2017

ashcrow mentioned this pull request May 18, 2017

Replace tarfile with tar on ostree import #1007

Closed

giuseppe force-pushed the replace-rpmbuild-with-rpmwriter branch from bc385e9 to d2bca31 Compare May 18, 2017 20:20

TomSweeneyRedHat reviewed May 22, 2017

View reviewed changes

giuseppe force-pushed the replace-rpmbuild-with-rpmwriter branch from 3dafbd5 to 637647f Compare May 22, 2017 13:44

ashcrow reviewed May 22, 2017

View reviewed changes

giuseppe force-pushed the replace-rpmbuild-with-rpmwriter branch from 637647f to 271ebdb Compare May 22, 2017 14:01

TomasTomecek reviewed May 24, 2017

View reviewed changes

ganhuang mentioned this pull request May 25, 2017

System package no hardcode openshift/openshift-ansible#4272

Merged

giuseppe added 7 commits May 30, 2017 20:40

Atomic/rpmwriter.py: new module for writing an RPM file

cd7619d

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

Atomic/rpm_host_install: replace rpmbuild with rpmwriter module

a5d0ad0

Closes: projectatomic#981 Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

Atomic/rpm_host_install: rename the RPM to the container name

df538dc

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

tests: drop check for rpmbuild

e574377

Since we don't use it anymore, run all the tests Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

tests: adjust the .rpm name

36371e9

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

tests: skip rpm installation on Atomic Host

a4f2326

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

Prefix RPM labels with atomic.

be51f9c

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

giuseppe force-pushed the replace-rpmbuild-with-rpmwriter branch from 271ebdb to be51f9c Compare May 30, 2017 18:42

rh-atomic-bot pushed a commit that referenced this pull request May 30, 2017

Atomic/rpm_host_install: replace rpmbuild with rpmwriter module

a0de950

Closes: #981 Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> Closes: #1005 Approved by: rhatdan

rh-atomic-bot pushed a commit that referenced this pull request May 30, 2017

Atomic/rpm_host_install: rename the RPM to the container name

5ce39df

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> Closes: #1005 Approved by: rhatdan

rh-atomic-bot pushed a commit that referenced this pull request May 30, 2017

tests: drop check for rpmbuild

6864d6e

Since we don't use it anymore, run all the tests Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> Closes: #1005 Approved by: rhatdan

rh-atomic-bot pushed a commit that referenced this pull request May 30, 2017

tests: adjust the .rpm name

763b9d6

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> Closes: #1005 Approved by: rhatdan

rh-atomic-bot pushed a commit that referenced this pull request May 30, 2017

tests: skip rpm installation on Atomic Host

19a6da4

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> Closes: #1005 Approved by: rhatdan

rh-atomic-bot pushed a commit that referenced this pull request May 30, 2017

Prefix RPM labels with atomic.

3d1ca35

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> Closes: #1005 Approved by: rhatdan

rh-atomic-bot closed this in 8a9e0c7 May 30, 2017

Conversation

giuseppe commented May 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue Numbers

Uh oh!

giuseppe commented May 17, 2017

Uh oh!

giuseppe commented May 18, 2017

Uh oh!

giuseppe commented May 18, 2017

Uh oh!

ashcrow commented May 18, 2017

Uh oh!

ashcrow commented May 18, 2017

Uh oh!

baude commented May 18, 2017

Uh oh!

giuseppe commented May 18, 2017

Uh oh!

cgwalters commented May 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

giuseppe commented May 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TomSweeneyRedHat commented May 22, 2017

Uh oh!

giuseppe commented May 23, 2017

Uh oh!

rhatdan commented May 23, 2017

Uh oh!

giuseppe commented May 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

baude commented May 30, 2017

Uh oh!

rh-atomic-bot commented May 30, 2017

giuseppe commented May 17, 2017 •

edited

Loading

cgwalters commented May 19, 2017 •

edited

Loading