Add Subversion support

[tsimonq2]

This adds support for Subversion and fixes bug 1543243 in Launchpad.

[snappy-m-o]

Can one of the admins verify this patch?

[snappy-m-o]

Can one of the admins verify this patch?

[elopio]

Thanks for your contribution @tsimonq2, and welcome to snapcraft!
I'll leave a note here for the rest that you have already signed the contributors agreement. Thanks for that too.

This branch is missing unit tests. Take a look at snapcraft/tests/test_sources.py for the kind of things that we require for full code coverage. Let me know if you need a hand with the tests.

[elopio]

This looks good, but it's missing that make file, and the test file. I'm not sure if it's work in progress, or you forgot to add the files to git.

[chadmiller]

It would be nice to test "svn:" source lines, too, and one check-out and one update.

[elopio]

woohoo, Chad is here! 🎉

[tsimonq2]

@elopio - I'm a little confused here. the git-* and hg-* directories have almost the same contents. What is different, and if applicable, what should I put in the make file and test file?

@chadmiller - Sort of going with the comment I left for elopio, would you suggest I do it as separate parts in this snapcraft.yaml, or in separate tests, and if so, would I rename the directory from simple-svn to svn-*?

[kyrofa]

Unless I'm mistaken, it doesn't look like source-type: svn will actually work given the code (i.e. it would need to be source-type: subversion)

[tsimonq2]

@kyrofa Fixed in the recent set of commits I pushed

[chadmiller]

@tsimonq2, I also think it makes us worry that you didn't run the test and discover the problem, which is kind of bad; or that you did run tests and this test should have run and failed but did not, which is really really bad. Please find out what the problem is, here.

[tsimonq2]

@chadmiller If you are talking about kyrofa's issue, I found that out already, but I forgot to update my code. My bad, but it's fixed now.

What else are you referring to if not that? Can you clarify?

[chadmiller]

@tsimonq2, yes, I mean that the source-type couldn't have matched anything in the test, earlier, and should have caused a test failure.

[tsimonq2]

@chadmiller Well like I said, I figured that out myself but I forgot to edit the code. It was a simple mistake that I'm glad was caught, and I'm thankful for that. That's all.

Now, what do I do now with this directory and this snapcraft.yaml file? See comment #4 on this line note string(if that makes any sense).

[elopio]

Not a big deal, but please revert this change.

[tsimonq2]

It was an accident, I'll revert it :)

[elopio]

another nit, please move this case after the git one. I'm a little code order freak, and I would like that all the cvs sources are together.

[snappy-m-o]

Can one of the admins verify this patch?

[snappy-m-o]

Can one of the admins verify this patch?

[kyrofa]

I know it's stupid, but can we have svn next to the other version control systems and leave tar and zip at the end?

[kyrofa]

Is there a reason not to also resolve 'svn' to Subversion?

[tsimonq2]

Good point, nice catch, @elopio

[tsimonq2]

@elopio @chadmiller @kyrofa This line is causing something to fail, I think the unit tests? https://travis-ci.org/ubuntu-core/snapcraft/jobs/137352934

What do I need to correct to make this go away? Do I need to remove 'svn://my-source' from line 334?

[sergiusens]

on 334 you specify source_dir and on 337 you expect source-dir

[tsimonq2]

@sergiusens oh! nice catch, thanks.

[sergiusens]

why not

subprocess.check_call(['svn', 'update'], cwd=self.source_dir)

?

[tsimonq2]

Good call, I'll make this change

[sergiusens]

this should be

elif source_type == 'subversion' or source_type == 'svn'

[tsimonq2]

Nice catch, I'm glad this was caught, I'll make the change

[sergiusens]

why not

self.mock_run.assert_called_once_with(['svn', 'update'], cwd=svn.source_dir)

if you take the advice from above

[tsimonq2]

Yeah, I prefer this over the code I had. Thanks!

[sergiusens]

For a proper integration test you will need to create this layout either on the fly or commited (on the fly might be more versatile)

Here's a shell snippet

svncreate repo
svn checkout file:///$(pwd)/repo checkout1 #  the source entry in the test code would use this
svn checkout file:///$(pwd)/repo checkout2
cd checkout2
touch some_file
svn add file
svn commit -m 'some file'
cd ../checkout1
svn update # this would be running pull again for the source
ls some_file

[sergiusens]

@elopio do you mind looking at that integration test I mention ^^

[sergiusens]

this is missing a # noqa statement, line it up with the Zip one above

[tsimonq2]

All fixed, thanks :)

[elopio]

2016

[elopio]

As I discussed on irc, this is missing the calls to self.run_snapcraft('pull', project_dir)
The purpose of the tests is to test the pull, not svn.

[elopio]

When the string has a single quote, we usually write it with double quotes:
"can't specify a source-tag..."

[elopio]

ditto

[elopio]

you are missing two tests to check the errors when tag and branch are used.

[elopio]

Can you please update the copyright year in this file to 2015, 2016 ?

[tsimonq2]

@elopio Sure

[tsimonq2]

Done

[elopio]

you can remove the \

[tsimonq2]

@elopio Bazaar has the same problem, I'll correct Subversion, but might it be worth a whole other PR to fix this?

[elopio]

Generally we call subprocess when there is no good way to do it with pure python. In the case of touch, you can do instead:

open(os.path.join('local', 'file'), 'w').close()

[elopio]

You can also check that the file exists.

[tsimonq2]

@elopio Good call

[tsimonq2]

Done

[tsimonq2]

@sergiusens Integrated ;)

[tsimonq2]

This seems to me like it's close to done if not completely done given the feedback I've received. Does anybody notice anything else that I should correct?

[elopio]

a nit, remove this empty line.

[elopio]

please use os.path.join('parts', 'svn', 'src')
part_src_path = os.path.join('parts', 'svn', 'src')
revno = subprocess.check_output(['svnversion', parts_src_path]).strip()

[elopio]

You can use self.assertThat(os.path.join(parts_src_path, 'file'), FileExists())
Add to the top:
from testtools.matchers import FileExists. Between shutil and integration_tests, with empty lines around.

[elopio]

Looks good to me. Please rebase or merge with master, or hit the update button that git shows you here vvv.

[chadmiller]

The 'chdir's bother me. Use instead the "cwd" keyword on subprocess functions. Or at least try/finally to save and restore the current working dir for the process.

[tsimonq2]

@chadmiller can you clarify what you mean by this?

[chadmiller]

subprocess.check_call( ['svn', 'add', 'file'],stdout=subprocess.DEVNULL, pwd=project_dir)

not

chdir
s.check_call

If this is a standalone test program, this doesn't matter very much, but changing global state in a test that continues past your test is a bad idea.

[chadmiller]

That $() is not going to do what you think unless subversion is going to interpret it, which I doubt. Please verify.

[tsimonq2]

@chadmiller running that command works locally, and playing around with it, I'm not seeing another way to do it. I think it's fine.

[elopio]

'file://{}'.format(os.path.join(project_dir, 'repo'))

[tsimonq2]

@elopio thanks, didn't know that was something I could do!

[tsimonq2]

@chadmiller Does it look good now?

@elopio I've merged with master, it should be good to go now.

[elopio]

One more thing I forgot: please add subversion to debian/tests/control, on the integration section.

[elopio]

This will fail the pep8 check. You need to put one empty space after the commas.

[elopio]

Looking at the coveralls coverage, you are still missing one test for the uri.
Take a look in test_sources.py, the TestUri test case. You need to add one that's just like the git test.

[tsimonq2]

@elopio The only other thing I can think to fix is https://coveralls.io/builds/6601979/source?filename=snapcraft%2Finternal%2Fsources.py#L378

What file would I edit to add coverage for this?

[elopio]

Look at snapcraft/tests/test_yaml.py, test_config_adds_vcs_packages...

[tsimonq2]

@elopio thanks

[elopio]

OK to test

[elopio]

retest this please

[elopio]

I'm wondering what would be a better way to do this.
what about reversing the loging and
if os.path.isdir(self.source), append file://, else, leave it alone ?

[tsimonq2]

@elopio I'll play around with this.

[tsimonq2]

@elopio that's a better solution, thanks, pushing now

[elopio]

notice that here you are not testing anything, because you are passing to the Subversion constructor a source that will return false in isdir.
On the constructor, pass only 'my-source'.

[elopio]

You can put this test in the same file as the other. Call one test_pull_svn_checkout and the other test_pull_svn_update.

[tsimonq2]

Oh okay, I'll do this, thanks

[tsimonq2]

Seems like the autopkgtest failure is not related to my snap if I'm reading this correctly: http://paste.ubuntu.com/17582500/

[tsimonq2]

Nope, found it

[tsimonq2]

This is causing the integration test errors. I have no clue what I can do to fix this. Thoughts? @elopio

[elopio]

Hey, sorry for the delay.
check_output never returns an int. By default, it returns bytes. I think it's better if you pass universal_newlines=True, and then assertEqual('1', revno)

[tsimonq2]

thanks @elopio