New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implementation of SCM feature for SVN #3192

Merged
merged 67 commits into from Oct 8, 2018

Conversation

@jgsogo
Member

jgsogo commented Jul 10, 2018

  • Refer to the issue that supports this Pull Request: closes #3130
  • If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
  • I've read the Contributing guide.
  • I've followed the PEP8 style guides for Python code.
  • Docs: conan-io/docs#837
  • Added tests
  • Add functionality #3479
  • Add test related to #3479: use subfolder for a SVN repo
  • Add #3496
  • Affected by #3500?
  • Add corresponding changes to adopt #3373
  • Open ticket with this: #3192 (comment)

Following issue #3130 reported by @keithrob91, I'm implementing SCM for SVN. This is just the first draft as further testing and more comprenhensive use cases have to be tried. I need some advice from Conan team related to some TODOs stated at the code, and also how to test it against a mocked SVN server.

Changelog: Feature: [Experimental] Add SCM support for SVN.

@wafflebot wafflebot bot added the contributor pr label Jul 10, 2018

@CLAassistant

This comment has been minimized.

CLAassistant commented Jul 10, 2018

CLA assistant check
All committers have signed the CLA.

cmd_command = "svn"

def __init__(self, runner=None, *args, **kwargs):
def runner_no_strip(command):

This comment has been minimized.

@jgsogo

jgsogo Jul 10, 2018

Member

I cannot strip SVN output because it relies on chars at specific columns. It may be used for git too and promote this function to the common ancestor class (here).

This comment has been minimized.

@memsharded

memsharded Jul 11, 2018

Contributor

Who is stripping the output? The regular runner?

This comment has been minimized.

@jgsogo

jgsogo Jul 11, 2018

Member

If no runner is given, the implementation of SCMBase::run does (here):

         if not self._runner:
             return subprocess.check_output(command, shell=True).decode().strip()
         else:
             return self._runner(command)
output = self.run("status -u -r {}".format(wc_revision))
offending_columns = [0, 1, 2, 3, 4, 6, 7, 8] # 5th column informs if the file is locked (7th is always blank)
it = iter(output.splitlines())
while True:

This comment has been minimized.

@lasote

lasote Jul 11, 2018

Contributor

for item in output.splitlines():?

This comment has been minimized.

@jgsogo

jgsogo Jul 11, 2018

Member

I need to skip next line in case of a tree conflict, so I need an iterator:

if item[6] == 'C':
    next(it)  # Skip line as it contains information about the conflict.
def get_revision(self):
wc_revision = self.run("info --show-item revision").strip()

# Check if working copy is consistent

This comment has been minimized.

@lasote

lasote Jul 11, 2018

Contributor

Good point and a nice to have. But I'm not especially worried about it, we could start supposing that the user is following some good practices like continuous integration automation where the repository will be consistent.

This comment has been minimized.

@jgsogo

jgsogo Jul 11, 2018

Member

I would output a warning to let the user know (in case they care about those lines)... but I don't know what is the conan way to emit a warning: shall I log it, do I need to pass an output stream to write into it?

This comment has been minimized.

@lasote

lasote Jul 11, 2018

Contributor

Good question. Here we don't have any output. In other similar situations we are passing an optional output to the constructor and then:

out = out or ConanOutput(sys.stdout, True)

The out usage is the same as in the conanfile.output: out.warn("C is not C++")

return self.run("info --show-item url").strip()

def get_revision(self):
wc_revision = self.run("info --show-item revision").strip()

This comment has been minimized.

@lasote

lasote Jul 11, 2018

Contributor

What is the output of this command? (I have no idea) but in svn the "revisions" are just sequential integers, right?. if we want to have a unique ID of the code (reproducibility) probably we would need also the branch?

This comment has been minimized.

@jgsogo

jgsogo Jul 11, 2018

Member

Its output is just the revision number, as I told you before there is no branch in SVN, it is just another directory/path inside the repository, so it will be encoded in the URL.

super(SVN, self).__init__(runner=runner, *args, **kwargs)

def clone(self, url, branch=None, submodule=None):
assert branch is None, "This concept has no meaning for SVN"

This comment has been minimized.

@lasote

lasote Jul 11, 2018

Contributor

Doesn't it? As far as I remember there were branches in svn. Could we check out first the repo and then change to the branch?

This comment has been minimized.

@jgsogo

jgsogo Jul 11, 2018

Member

The concept of branch in SVN is just another place/path in the repository and it will be encoded in the URL. SVN is just a filesystem with historized files, branches and tags are just another directory and usually you have several projects (if not all) in the same repository.

The most common layout for a project inside a repo in SVN is to create three folders: trunk, branches and tags; so you will have master branch at https://my.svn/project/trunk a branch called featureX at https://my.svn/project/branches/featureX... and in the same way a tag is just another url like https://my.svn/project/tags/v1.2.3.

But this layout is not guaranteed, so you cannot infer where is branchXYZ given the path to trunk.

This comment has been minimized.

@lasote

lasote Jul 11, 2018

Contributor

You are right, now I remember. Thanks! it makes sense, the URL will define the "branch" and the revision (integer) will be unique.

cmd_command = "svn"

def __init__(self, runner=None, *args, **kwargs):
def runner_no_strip(command):

This comment has been minimized.

@memsharded

memsharded Jul 11, 2018

Contributor

Who is stripping the output? The regular runner?

output = self.run("status --no-ignore")
for it in output.splitlines():
if it[0] == 'I':
file = it[9:].strip()

This comment has been minimized.

@memsharded

memsharded Jul 11, 2018

Contributor

file is reserved built-in. Hint: Using some linter could help.

@jgsogo

Add more information where requested.
Change file as variable name (added commit).

super(SVN, self).__init__(runner=runner, *args, **kwargs)

def clone(self, url, branch=None, submodule=None):
assert branch is None, "This concept has no meaning for SVN"

This comment has been minimized.

@jgsogo

jgsogo Jul 11, 2018

Member

Assert? Raise exception? Ignore it?

This comment has been minimized.

@lasote

lasote Jul 11, 2018

Contributor

Remove the parameters. I prefer each class making sense itself. The model SCM class can call Git().clone and SVN().clone with the right parameters.

This comment has been minimized.

@jgsogo

jgsogo Jul 11, 2018

Member

I'll do it, but I often prefer classes within the same hierarchy to have identical arguments for functions with the same name (and I would declare it in the parent class). I think it is easier for the developer to understand the code and also useful for the IDE.

I'm thinking about something like this:

repo_class = {"git": Git, "svn": SVN}.get(self._data.type)
repo = repo_class(...)
repo.clone(branch="mybranch")

but this is another topic and should be treated apart.

cmd_command = "svn"

def __init__(self, runner=None, *args, **kwargs):
def runner_no_strip(command):

This comment has been minimized.

@jgsogo

jgsogo Jul 11, 2018

Member

If no runner is given, the implementation of SCMBase::run does (here):

         if not self._runner:
             return subprocess.check_output(command, shell=True).decode().strip()
         else:
             return self._runner(command)
super(SVN, self).__init__(runner=runner, *args, **kwargs)

def clone(self, url, branch=None, submodule=None):
assert branch is None, "This concept has no meaning for SVN"

This comment has been minimized.

@jgsogo

jgsogo Jul 11, 2018

Member

The concept of branch in SVN is just another place/path in the repository and it will be encoded in the URL. SVN is just a filesystem with historized files, branches and tags are just another directory and usually you have several projects (if not all) in the same repository.

The most common layout for a project inside a repo in SVN is to create three folders: trunk, branches and tags; so you will have master branch at https://my.svn/project/trunk a branch called featureX at https://my.svn/project/branches/featureX... and in the same way a tag is just another url like https://my.svn/project/tags/v1.2.3.

But this layout is not guaranteed, so you cannot infer where is branchXYZ given the path to trunk.

return self.run("info --show-item url").strip()

def get_revision(self):
wc_revision = self.run("info --show-item revision").strip()

This comment has been minimized.

@jgsogo

jgsogo Jul 11, 2018

Member

Its output is just the revision number, as I told you before there is no branch in SVN, it is just another directory/path inside the repository, so it will be encoded in the URL.

def get_revision(self):
wc_revision = self.run("info --show-item revision").strip()

# Check if working copy is consistent

This comment has been minimized.

@jgsogo

jgsogo Jul 11, 2018

Member

I would output a warning to let the user know (in case they care about those lines)... but I don't know what is the conan way to emit a warning: shall I log it, do I need to pass an output stream to write into it?

output = self.run("status -u -r {}".format(wc_revision))
offending_columns = [0, 1, 2, 3, 4, 6, 7, 8] # 5th column informs if the file is locked (7th is always blank)
it = iter(output.splitlines())
while True:

This comment has been minimized.

@jgsogo

jgsogo Jul 11, 2018

Member

I need to skip next line in case of a tree conflict, so I need an iterator:

if item[6] == 'C':
    next(it)  # Skip line as it contains information about the conflict.
@jgsogo

This comment has been minimized.

Member

jgsogo commented Jul 11, 2018

To start thinking about testing I need a way to create a repo to make requests against it. SVN allows to create a local repo and call it using file:// protocol, but we will need specific software to be installed in CI. Are svnadmin (and svn) command line programs available?

@lasote

This comment has been minimized.

Contributor

lasote commented Jul 12, 2018

Probably they are installed in the Linux slaves, don't worry, submit the test/tests you consider and if it fails in some slaves we will take care!

[SCM] Add tests related to SVN
It provides several tests and auxiliary classes to cover use cases for SVN implementation of SCM feature.

It requires `svnadmin` and `svn` command line tools to be installed in testing machine.
@jgsogo

This comment has been minimized.

Member

jgsogo commented Jul 15, 2018

@lasote, svnadmin not found 😭

I've added a lot of tests (same as scm/git plus some unittesting around SVN class). Test suite works (Windows/py3) and I think it is more or less complete, but it requires some more testing from real users in real scenarios. As it is an experimental feature and it doesn't interfere with Git implementation it should be safe to release it... but before it CI must be provided with a SVN installation.

@Aalmann

This comment has been minimized.

Aalmann commented Sep 28, 2018

@jgsogo
I played a bit around with a local repo (file:/// protocol) and the scm-auto feature. Great job so far.
Just one thing. I have seen, that you are adding the revision (in my case "6") but also the complete url (like "file:///my/u/r/l/trunk@6") which includes the branch and the revision.
I would suggest to separate them into "revision": "6", "branch": "trunk", "url": "file:///my/u/r/l" and to join them internally.

@jgsogo

This comment has been minimized.

Member

jgsogo commented Sep 28, 2018

@Aalmann

To build the URL for SVN we are using also the peg_revision (which may not be the same as the revision), it could be useful for some use cases. About the branch, SVN doesn't have the concept of branch like Git does, it is just a convention of a directory structure, so I think it is better to have the full URL and leave the "branch" as an information token (in fact, it is not used for SVN).

@Aalmann

This comment has been minimized.

Aalmann commented Sep 28, 2018

@jgsogo
Ok. But when running svn info you will get

Working Copy Root Path: D:\Work
URL: https://my/svn/repo/url/branches/development/my_feature
Relative URL: ^/branches/development/my_feature
Repository Root: https://my/svn/repo/url
Repository UUID: <UUID>
Revision: 9778
Node Kind: directory
Schedule: normal
Last Changed Author: <user-name>
Last Changed Rev: 9765
Last Changed Date: 2018-09-29 14:48:33 +0100 (Fr, 28 Sep 2018)

So probably "branch" is not correct, but "relative_url" would be, isn't it?
I'm not very familiar with the peg_revision issue, but I think it would be more obvious to the user when splitting everything into several parts especially when using the scm attribute in recipe with a lot of auto entries. Like:

scm = {
    "type": "svn",
    "url": "auto",
    "relative_url": "auto", # which is the trunk, branch or a tag
    "revision": "auto"
}
@jgsogo

This comment has been minimized.

Member

jgsogo commented Sep 28, 2018

Relative URL starts at the root of the SVN repo, isn't it? It makes sense if you are going to serve the repository at another URL, but Conan will hardcode the URL too. So having the full URL in a single item should be ok.

Also, we should consider modeling the url and relative_url also for Git, and I don't know if Git lets us know something like scm = {"url": "https://github.com/", "relative_url": "^/conan-io/conan"}. And I think that the scm should be the min common factor for SVN and Git (and others to come).

From the user point of view, the recipe will look like:

scm = {
     "type": "svn",
     "url": "auto",
     "revision": "auto",
     ...
}

there is nothing related to peg_revision or branches (neither in Git); and if the user wants to make it explicit, the peg_revision is not compulsory, and the url and revision are straighforward.

Maybe I'm not understanding your concerns.

try:
self.version = SVN.get_version()
except ConanException:
self.version = Version("1.10") # TODO: Go for a modern one, or raise?

This comment has been minimized.

@jgsogo

jgsogo Sep 28, 2018

Member

@lasote, what would you do here? What to do if the function to parse SVN version fails?

@Aalmann

This comment has been minimized.

Aalmann commented Sep 28, 2018

Relative URL starts at the root of the SVN repo, isn't it? It makes sense if you are going to serve the repository at another URL, but Conan will hardcode the URL too. So having the full URL in a single item should be ok.

Also, we should consider modeling the url and relative_url also for Git, and I don't know if Git lets us know something like scm = {"url": "https://github.com/", "relative_url": "^/conan-io/conan"}. And I think that the scm should be the min common factor for SVN and Git (and others to come).

From the user point of view, the recipe will look like:

scm = {
     "type": "svn",
     "url": "auto",
     "revision": "auto",
     ...
}

there is nothing related to peg_revision or branches (neither in Git); and if the user wants to make it explicit, the peg_revision is not compulsory, and the url and revision are straighforward.

Maybe I'm not understanding your concerns.

Yes URL is the SVN repo root, like the git url and the Relative URL is the branch, trunk or tag plus optional the child to checkout. From a technical point of view the only difference to git is, that you can checkout a "child" element in SVN by appending it to the url (like "trunk" or "trunk/src"). However, the relative url is nearly the same as the branch in git.

The question is: is it really required to map all existing scm-keys to all SCM implementations or does it make sense to have just the type, url and revision key shared by both SVN and Git (and probably other in later versions) and to have some special keys for the concrete tool implementations.
What I mean is, in Git you have the branch key which could be "reused" for SVN, but I think you don't like this. I'm completely Ok with it. So currently one can provide branch for SVN but it has no effect. So it is specialized to Git an IMHO it would make sense to have something similar for SVN.
Just think about later implementations like adding a switch command (from relative url = trunk to relative url = tags/1.2.3.

Concerning --trust-server-certs: I'm so sorry, but having a version check would be very nice esp. for our old build agents. 😃

@jgsogo

This comment has been minimized.

Member

jgsogo commented Sep 28, 2018

There is no branch key in SCM: https://docs.conan.io/en/latest/reference/conanfile/attributes.html?highlight=verify_ssl#scm, it is only implemented in the Git wrapper (also in the SVN one), but it doesn't appears in the SCM feature.

My last commit adds the check for SVN version 😉, defaulting to "1.10" if it fails.

@Aalmann

This comment has been minimized.

Aalmann commented Sep 28, 2018

Oh, you are right. I'm sorry for that mistake. However, probably the branch / relative_url could be something for the next releases. To have something like

...
base = python_requires("myBaseRecipes/1.2.3@user/channel")

class Module1(base.ConanBase):
...
scm = {
    "type": "svn",
    "url": "https://..."
    "relative_url": "trunk",
    "revision": "HEAD"
}
...
base = python_requires("myBaseRecipes/1.2.3@user/channel")

class Module2(base.ConanBase):
...
scm = {
    "type": "git",
    "url": "https://..."
    "branch": "feature/1-2-3",
    "revision": "HEAD"
}
# implementation of the base class
...
def source(self)
    scm = tools.SCM()
    scm.checkout() # whis uses the scm information provided by the scm dict

which would be very useful for recipe in different repo than sources and in combination with python_requires.

Concerning version check 🥇 😃 👍

@climblinne

This comment has been minimized.

Contributor

climblinne commented Sep 28, 2018

Concerning version check 🥇 😃 👍

This is already working. Please remember, that the scm parameters are only needed to checkout the source code. To get the additional information you can use the SCM instance. There is already the branch implemented and you can use it for versioning:

def get_svn_version(version):
    try:
        scm = tools.SVN()
        revision = scm.get_revision()
        if scm.is_pristine():
            dirty = ""
        else:
            dirty = ".dirty"
        return "%s-%s+%s.%s" % (version, revision, scm.get_branch(), dirty)
    except:
        return None

class BaseLibrary(ConanFile):
    version = get_svn_version("1.2.0")
@Aalmann

This comment has been minimized.

Aalmann commented Sep 28, 2018

Looks nice. I will think about it how to integrate in our recipe. Thanks for the hint.
But this makes only sense in case recipe is in source repo, isn't it?

@Aalmann

This comment has been minimized.

Aalmann commented Sep 28, 2018

Btw. it would make sense to have a good example for this at https://docs.conan.io/en/latest/creating_packages/package_repo.html#package-repo

Again, many thanks for your work. 👍

@jgsogo

This comment has been minimized.

Member

jgsogo commented Sep 29, 2018

With this last change, I think that the feature is ready to be packed and included in 1.8. Thanks a lot for your valuable feedback, suggestions and testing. It will be experimental, as there are a lot of use cases that we haven't think about and for sure there will be some bugs and work to be done, but it is time to test it for real 💪

Please, review your reviews 😸

@keithrob91

This comment has been minimized.

keithrob91 commented Oct 1, 2018

Btw. it would make sense to have a good example for this at https://docs.conan.io/en/latest/creating_packages/package_repo.html#package-repo

Agree with this!

@jgsogo jgsogo referenced this pull request Oct 1, 2018

Merged

Use XML output to compute SVN.is_pristine() #3653

5 of 7 tasks complete
@climblinne

climblinne approved these changes Oct 1, 2018 edited

From my side I think everything is okay. The is_pristine will be discussed in the other issue

@climblinne

This comment has been minimized.

Contributor

climblinne commented Oct 1, 2018

Btw. it would make sense to have a good example for this at https://docs.conan.io/en/latest/creating_packages/package_repo.html#package-repo

I generated an issue for it: conan-io/docs#861.
I am just on holiday, so I will write something in a week.

@@ -306,6 +314,60 @@ def create_local_git_repo(files=None, branch=None, submodules=None, folder=None)
return tmp.replace("\\", "/"), git.get_revision()


def handleRemoveReadonly(func, path, exc): # TODO: May promote to conan tools?

This comment has been minimized.

@SSE4

SSE4 Oct 3, 2018

Contributor

yes please, this often fails for me in various situations using conan commands with git repositories, so had to do removes manually

This comment has been minimized.

@climblinne

climblinne Oct 3, 2018

Contributor

Could we issue a ticket for it? It would be nice to get this code review into the develop branch...

This comment has been minimized.

@jgsogo

jgsogo Oct 8, 2018

Member

Yes, please, maybe open an issue or directly a PR on top on this.

@memsharded memsharded merged commit 59ee6b0 into conan-io:develop Oct 8, 2018

1 of 2 checks passed

continuous-integration/jenkins/pr-head This commit cannot be built
Details
license/cla Contributor License Agreement is signed.
Details

@wafflebot wafflebot bot removed the stage: review label Oct 8, 2018

@jgsogo jgsogo deleted the jgsogo:issue-3130 branch Oct 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment