sources: Add RPM source

[Conan-Kudo]

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

[zyga]

Looks good to me

[elopio]

Thanks for this!
This is missing unit and integration tests. You can base your tests on the ones from snapcraft/tests/test_sources.py and integration_tests/test_deb_source.py.

[elopio]

This also need to be in the debian/control file.

[Conan-Kudo]

@elopio I ignored the debian/control file because I saw #834 pending. Should I still add python3-libarchive-c dependency to debian/control?

[elopio]

Yes please. I think that your PR will land before mine, because I still have work to do on the snapcraft/debian branch.

[Conan-Kudo]

Done. :)

[elopio]

I don't really understand which deps should be in install_requires and which should be in the requirements file.

[Conan-Kudo]

Well, ideally, install_requires should be using requirements.txt. I wasn't sure what to do here...

[sergiusens]

El 19/10/16 a las 10:17, Neal Gompa (ニール・ゴンパ) escribió:

@Conan-Kudo commented on this pull request.

---

In setup.py #870:

@@ -53,6 +53,7 @@
install_requires=[
'pyxdg',
'requests',

•    'libarchive-c',
  

Well, ideally, |install_requires| should be using |requirements.txt|.
I wasn't sure what to do here...

You want both https://packaging.python.org/requirements/

[Conan-Kudo]

@sergiusens So, I did both in this case. I suppose a separate pull request is required to address the mismatch between requirements.txt and setup.py, but that's out of scope for this one.

[sergiusens]

I don't understand why I could update the branch for your repo, but I just did!

[sergiusens]

Looks good, just two minor changes I would like to see here.

[sergiusens]

Just do

dst = dst.rstrip('/')

[Conan-Kudo]

Done.

[sergiusens]

This would be better to write as

rpm_file_entry.pathname = os.path.join(dst, rpm_file_entry.pathname.lstrip('./'))

[sergiusens]

oic, if rpm_file_entry.pathname starts with / this won't work.

[sergiusens]

Maybe then

rpm_file_path = rpm_file_entry.pathname.lstrip('./')
rpm_file_path = rpm_file_path.lstrip('/')
rpm_file_entry.pathname = os.path.join(dst, rpm_file_path)

[Conan-Kudo]

Done.

[Conan-Kudo]

@sergiusens The reason you were able to do that is that GitHub silently launched a new feature that allows people to allow maintainers to manipulate the branch of a pull request. They activated it by default, and expect users to switch it off if it's not wanted. You'd see the flag at the bottom right corner of the pull request, under the (un)subscribe button.

[Conan-Kudo]

@elopio @sergiusens The only thing I'm missing is the unit test, but I'm not sure how to write it. I don't really understand what's going on in the TestDeb class in tests/test_sources.py and how to make an equivalent for my RPM source.

[sergiusens]

@elopio mind helping @Conan-Kudo out?

[elopio]

You said you reported the bug. Can you please put a link to it here?

[Conan-Kudo]

I've just filed the bug with python-libarchive-c. Do you want me to mention it in the code itself?

[elopio]

yes, please. Something like:

# XXX: A bug in libarchive ...
# Reported in https://github.com/Changaco/python-libarchive-c/issues/43

[Conan-Kudo]

Done. Though apparently it's less of a bug and more undocumented limitation of libarchive, according to the author.

[elopio]

I'm not quite sure I understand this.
Looking at the sourcecode ( https://github.com/Changaco/python-libarchive-c/blob/master/libarchive/extract.py#L42 ) it says that extract_entries extracts the files in the current directory.
How is it that changing pathname makes it extract in a different directory?

[elopio]

extract_entries feels like internal to me. Is this a bad idea for some reason?
https://paste.ubuntu.com/23418276/
(it would need to be smarter about not using the abspath when the source is a url)

[Conan-Kudo]

@elopio It's not really internal, but libarchive is a really raw library. This is the only way to alter the path to extract to an alternative base path, since libarchive assumes a "tar-like" extraction model (it follows the path associated with a blob blindly to write out data).

[niemeyer]

Nice!

[elopio]

@Conan-Kudo I'm sorry it took so long to review your tests here.

This is the kind of unit tests I would like to see:

    def test_pull_rpm_file_must_extract(self):
        dest_dir = 'src'
        os.makedirs(dest_dir)

        test_file_path = os.path.join(self.path, 'test.txt')
        open(test_file_path, 'w').close()
        rpm_file_path = os.path.join(self.path, 'test.rpm')
        with libarchive.file_writer(rpm_file_path, 'cpio', 'gzip') as rpm:
            rpm.add_files(test_file_path)

        rpm_source = sources.Rpm(rpm_file_path, dest_dir)

        rpm_source.pull()
        self.assertEqual(os.listdir(dest_dir), '[test.txt]')

The idea is that we make an rpm file with all the interesting details that might cause it to fail, and pull it. However, this is failing for me, I'm still a little lost in this libarchive. But does it make sense?

[elopio]

ok, this one works: https://paste.ubuntu.com/23418469/

[sergiusens]

source-commit has been added so we will need some updates to the code in this area. There seems to also be need of running ./runtests.sh static

Cheers

[Conan-Kudo]

@elopio So I'm not crazy and I didn't break it by not touching anything?

👍

[elopio]

Not crazy until proven otherwise :)
Thanks for adding the tests. I'm wondering about these two lines.
Is there a way to make libarchive.file_writer to include entries with ./ and / ?

[sergiusens]

This was my suggestion as it felt nice than what was originally there, I'd leave it as a TODO in any case.

[sergiusens]

One last error:

======================================================================

ERROR: test_extract_and_keep_rpmfile (snapcraft.tests.test_sources.TestRpm)

----------------------------------------------------------------------

Traceback (most recent call last):

  File "/usr/lib/python3.5/shutil.py", line 538, in move

    os.rename(src, real_dst)

FileNotFoundError: [Errno 2] No such file or directory: 'src/test.rpm' -> '/tmp/tmpg0c8cv4s'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):

  File "/home/travis/build/snapcore/snapcraft/snapcraft/tests/test_sources.py", line 205, in test_extract_and_keep_rpmfile

    rpm_source.provision(dst=dest_dir, keep_rpm=True)

  File "/home/travis/build/snapcore/snapcraft/snapcraft/internal/sources.py", line 499, in provision

    shutil.move(rpm_file, tmp_rpm)

  File "/usr/lib/python3.5/shutil.py", line 552, in move

    copy_function(src, real_dst)

  File "/usr/lib/python3.5/shutil.py", line 251, in copy2

    copyfile(src, dst, follow_symlinks=follow_symlinks)

  File "/usr/lib/python3.5/shutil.py", line 114, in copyfile

    with open(src, 'rb') as fsrc:

FileNotFoundError: [Errno 2] No such file or directory: 'src/test.rpm'

----------------------------------------------------------------------

[Conan-Kudo]

@sergiusens I'm not sure why this test is failing. I think I've written the test wrong, since the part of Rpm() class it is failing on is exactly the same as the Deb() class, and the corresponding test written for the Deb source works fine.

[elopio]

You shouldn't call pull after provision. That will result on provision being called twice.

[Conan-Kudo]

Just removed it. We'll see how well that works...

[elopio]

Got it: http://paste.ubuntu.com/23432601/
A better solution would be to add a keep argument to pull, but that's a bigger change, not for this pr.

[Conan-Kudo]

Woo! All the tests pass!

@sergiusens Does it look good to merge now?

[sergiusens]

Actually we had a glitch in how we tested :-( (switched systems but managed to always trigger tests from PRs but ran on master)

This is the status now:

======================================================================
FAIL: test_extract_and_keep_rpmfile (snapcraft.tests.test_sources.TestRpm)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/tmp/autopkgtest.ayArTU/build.5eu/snapcraft/snapcraft/tests/test_sources.py", line 211, in test_extract_and_keep_rpmfile
    self.assertEqual(os.listdir(dest_dir), ['test.txt', rpm_file_name])
AssertionError: Lists differ: ['test.rpm', 'test.txt'] != ['test.txt', 'test.rpm']

First differing element 0:
'test.rpm'
'test.txt'

- ['test.rpm', 'test.txt']
+ ['test.txt', 'test.rpm']

----------------------------------------------------------------------

[sergiusens]

Use https://docs.python.org/3.2/library/unittest.html#unittest.TestCase.assertCountEqual

[Conan-Kudo]

@sergiusens I've pushed a fixup commit to switch to that.