Skip to content
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

[SCM] [SVN] auto feature behavior depends on how user has checked out local working copy #3831

Closed
3 tasks done
ahauan4 opened this issue Oct 23, 2018 · 20 comments · Fixed by #4088
Closed
3 tasks done

Comments

@ahauan4
Copy link

ahauan4 commented Oct 23, 2018

  • I've read the CONTRIBUTING guide.
  • I've specified the Conan version, operating system version and any tool that can be relevant.
  • I've explained the steps to reproduce the error or the motivation/use case of the question/suggestion.

Conan 1.8.2
Python 3.6.5
Windows 7
SVN 1.9.5

Our SVN repository structure is like this:

module1/<trunk|tags|branches>/conanfile.py
module2/<trunk|tags|branches>/conanfile.py
...

We have the following scm statement in our conanfile.py

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

The current behavior when calling conan create depends on whether someone has checked out all modules from SVN or only one module.

Scenario A:
User A has checked out only module1 from SVN repository https://svn_repo/module1/trunk to a local folder C:\svn\module1\trunk.
Location of .svn hidden folder: C:\svn\module1\trunk\.svn
Location of conan file: C:\svn\module1\trunk\conanfile.py
Conan command: conan create C:\svn\module1\trunk user/channel
In this case the conan SVN scm auto feature behaves like expected.
The conan cache source folder afterwards contains exactly the same content as the local SVN working copy folder for which conan create was called.

Scenario B:
User B has checked out all modules from SVN repository https://svn_repo to a local folder C:\svn.
Location of .svn hidden folder: C:\svn\.svn
Location of conan file: C:\svn\module1\trunk\conanfile.py
Conan command: conan create C:\svn\module1\trunk user/channel
In this case the conan SVN scm auto feature behaves not like expected.
The conan cache source folder afterwards contains all modules (module1, module2, ...) instead of only module1 trunk, for which conan create was called.
So it contains the working copy of the SVN base folder 'https://svn_repo' instead of 'https://svn_repo/module1/trunk'

Expected behavior:
The source folder in the local conan cache should contain all subfolders of the local SVN folder where the conanfile.py is located for which conan create was called, not all root folders including other modules checked out locally by the user.
The behavior should not depend on the location of the hidden .svn directory which depends on how a user has checked out his/her local working copy.

Currently we cannot use the SVN scm auto feature, because a lot of users check out their SVN working copy like in scenario B.

Could you please give us a statement on this topic.

Thank You!

@jgsogo
Copy link
Contributor

jgsogo commented Oct 24, 2018

Hi @ahauan4!

You are right, and it is the expected behavior. SCM feature for SVN uses the root of the working copy to compute url, revision and every function implemented in our SVN wrapper. The scenarios you described are not the only ones we have to deal with, as an example, there is another one which we come across quite often:

Some projects has its conanfile.py inside a subdirectory, so we cannot take it as the root of the repository, we also cannot take the current-working-dir... so the only consistent solution is to take the root of the working copy.


Nevertheless, you can explicitly write your url and leave the revision="auto", so you will always get the same checkout:

scm = {
    "type": "svn",
    "url": "https://svn_repo/module1/trunk",
    "revision": "auto",
}

but here you will face a different problem: the revision number retrieved for the working copy may be different if you checkout only a subtree of the repository or all of it.


I'm so sorry for this caveats, but IMHO opinion working with the root of the working copy is the best and most consistent compromise solution (without adding complexity and options to this feature).

Please, let me know what you think, we would really appreciate feedback, suggestions and any comment to improve this feature.

Thanks!

@jgsogo jgsogo self-assigned this Oct 24, 2018
@jgsogo jgsogo added this to the 1.10 milestone Oct 24, 2018
@jgsogo jgsogo changed the title SVN scm auto feature behavior depends on how user has checked out local working copy [SCM] [SVN] auto feature behavior depends on how user has checked out local working copy Oct 24, 2018
@ahauan4
Copy link
Author

ahauan4 commented Oct 24, 2018

Hello @jgsogo,

in my opinion the default behaviour should be to use the current directory where the conanfile is located.
If someone is not using the current directory, it should be configurable which directory is used relative to the conanfile.

The approach with detecting the base is a Git approach but with Subversion anybody can checkout the local working copy differently. So detecting the base is errorprone and inpredictible.

Your suggestion to set the url explicitely does not work.
It has the same effect as setting url to "auto". It only works if I also set revision to a value != "auto".
It seems that setting revision = "auto" also sets url = "auto".

But regardless of that, setting the url and/or revision to an explicit value is errorprone and makes the complete feature somewhat useless.

I hope you can help us use the SVN SCM feature as expected, because it depends a lot if we can use Conan in our company at all.

Thank You!

@jgsogo
Copy link
Contributor

jgsogo commented Oct 25, 2018

Ok, let us finish version 1.9 during this week and we will come back afterward with this. Changing this behavior will affect everyone using SCM with SVN right now, we need to ask for their feedback too.

@ahauan4
Copy link
Author

ahauan4 commented Oct 25, 2018

I see that you may not want to change that behavior because it has already been released, although it's still stated as "Experimental".

A possible way to support both methods:

  1. base svn url
  2. and svn url folder containing the conanfile

would be to e. g. support an additional option like

scm = {
    "type": "svn",
    "url": "auto",
    "revision": "auto",
    "use_svn_base_url": False, # this could also be the default if not set
}

... or a different option like auto e. g.

scm = {
    "type": "svn",
    "url": "auto_base",
    "revision": "auto_base",
}

Problem here is that someone could mix the behaviour of url and revision retrieval. Maybe revision should be set automatically to the same method like url if revision is not set (if revision=None)

There is also the subFolder parameter.
Can you please explain what this parameter exactly does when using SCM type SVN?
Can you select the relative folder maybe with this parameter?

It would be nice if you could have a look at this ideas and provide us some feedback.

Thank You!

@lasote lasote removed this from the 1.10 milestone Nov 19, 2018
@jgsogo
Copy link
Contributor

jgsogo commented Nov 20, 2018

Sorry for the delay. We've been traveling past weeks and preparing some events (like MeetingC++), and now we have to work on all the issues we left behind.

The subfolder parameters states where the repository should be cloned when building the package (it is not related to what we are talking here).

I still think that the best solution will be to have a fixed url (so it will be the same regardless of the checked out root), and yes, we should address the issue you commented related to that:

Your suggestion to set the url explicitely does not work.
It has the same effect as setting url to "auto". It only works if I also set revision to a value != "auto".
It seems that setting revision = "auto" also sets url = "auto".

And then there will be one problem left: in SVN the revision is computed taking into account the checkout root, so here we need a new paramater (I don't like having parameters specific for one SCM), or you could provide a function to retrive the revison using the desired root folder:

(This is just a proposal, not tested, and it may require some changes)

def get_svn_revision():
    here = os.path.dirname(__file__)
    svn = SVN(here)
    revision = svn._show_item('revision', target=here)
    return revision

class MyPackage(ConanFile):
    scm = {"type": "svn",
           "url": "path/to/package/",
           "revision": get_svn_revision(),
           }

@jgsogo
Copy link
Contributor

jgsogo commented Dec 4, 2018

Hi @ahauan4, did you test what I suggested to you on my last comment? I think it is the best option: fixed URL and also an auto revision through a custom function, so you can get the revision having control of the path you are using to get the revision from.

@ahauan4
Copy link
Author

ahauan4 commented Dec 5, 2018

Hello @jgsogo, your suggestion seems promising, but I didn't have the time to test it yet.
Would it be possible to detect the url also?
I will test it and report it as soon as possible

@jgsogo
Copy link
Contributor

jgsogo commented Dec 5, 2018

Hi! You can write a custom function, sure, but I cannot see the advantage between writing the URL or a function that will always retrieve the same URL... unless you want to reuse that function in other recipes through python_requires.

@ahauan4
Copy link
Author

ahauan4 commented Dec 6, 2018

Hello, yes I was thinking about python_requires

@ahauan4
Copy link
Author

ahauan4 commented Dec 7, 2018

Hello @jgsogo,

I tested your code and retrieving the revision seems to work.
But when I try to get the url the same way, I get the following Exception when calling conan create.
svn: E155007: 'D:\.conan\data\ConanTestModule\999.0.0\test\testing\export' is not a working copy

@jgsogo
Copy link
Contributor

jgsogo commented Dec 7, 2018

If it is a working copy when working on the revision, it should be a working copy too for the url... Can you copy here the functions you have written? I'll try to reproduce the issue with a minimum example in order to work on it.

@ahauan4
Copy link
Author

ahauan4 commented Dec 7, 2018

Here's my code:

def get_scm():
    here = os.path.dirname(__file__)
    svn = conans.client.tools.scm.SVN(here)
    return {
        "type": "svn",
        "url": svn._show_item('url', target=here),
        "revision": svn._show_item('revision', target=here),
    }

class ConanRecipe(conans.ConanFile):
    scm = get_scm()

@jgsogo
Copy link
Contributor

jgsogo commented Dec 7, 2018

Very interesting issue/bug indeed. I think I have reproduced the root cause in #4088, let me explain it a little bit more:

We are evaluating the scm attribute only when there are the auto keywords in it. We need to do it before exporting the recipe because we need access to the working copy in order to evaluate remote url and commit to substitute the auto keywords. In your case, we need to run it too in the working copy because your functions need the working copy too.... the PR changes the behavior so SCM is evaluated (and substituted) always.

@michaelmaguire
Copy link

michaelmaguire commented Dec 17, 2018

It seems that setting revision = "auto" also sets url = "auto".

I got burned by this too! I had url set to the specific subdirectory relevant to the package, but revision = "auto" and it checked out a whole huge multi-GB repo. Also my build() instructions didn't work because assumed I was in the specific subdirectory.

I tried the suggestion in #3831 (comment):

# Work around https://github.com/conan-io/conan/issues/3831
def get_svn_revision():
    here = os.path.dirname(__file__)
    svn = tools.SVN(here)
    revision = svn._show_item('revision', target=here)
    return revision
[...]
    scm = {
            "type": "svn",
            "url" :"svn+ssh://devsvn/develop/xercesc-3.1.1",
            "revision": get_svn_revision(),
            "subfolder": "checkoutdir"
        }

But get error:

svn: E155007: 'E:\DevelopBp\mmaguir1\.conan\xercesc\3.1.1\PORT\stable\export' is not a working copy
ERROR: Unable to load conanfile in E:\DevelopBp\mmaguir1\.conan\xercesc\3.1.1\PORT\stable\export\conanfile.py
  File "<frozen importlib._bootstrap>", line 684, in _load
  File "<frozen importlib._bootstrap>", line 665, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 678, in exec_module
  File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
  File "E:\DevelopBp\mmaguir1\.conan\xercesc\3.1.1\PORT\stable\export\conanfile.py", line 12, in <module>
    class XercescConan(ConanFile):
  File "E:\DevelopBp\mmaguir1\.conan\xercesc\3.1.1\PORT\stable\export\conanfile.py", line 25, in XercescConan
    "revision": get_svn_revision(),
  File "E:\DevelopBp\mmaguir1\.conan\xercesc\3.1.1\PORT\stable\export\conanfile.py", line 8, in get_svn_revision
    revision = svn._show_item('revision', target=here)
  File "conan\conans\client\tools\scm.py", line 215, in _show_item
  File "conan\conans\client\tools\scm.py", line 211, in run
  File "conan\conans\client\tools\scm.py", line 40, in run
  File "conan\conans\client\tools\scm.py", line 182, in runner_no_strip
  File "subprocess.py", line 336, in check_output
  File "subprocess.py", line 418, in run
subprocess.CalledProcessError: Command 'svn info --show-item revision "E:\DevelopBp\mmaguir1\.conan\xercesc\3.1.1\PORT\stable\export"  --no-auth-cache --non-interactive' returned non-zero exit status 1.

@ghost ghost removed the stage: review label Dec 18, 2018
@memsharded
Copy link
Member

This has been fixed by #4088. It will be released in conan 1.11, but if you want to give it a try using test.pypi.org repository to install development (non-stable) Conan versions:

$ pip install --index-url https://test.pypi.org/simple/ conan

It is in 1.11.0.dev1545127100

@ahauan4
Copy link
Author

ahauan4 commented Dec 19, 2018

The workaround seems to work now with version 1.11.0.dev1545127100. Thanks for solving!

Still the following error message is shown each time before building and after Copying sources to build folder is written to the console: svn: E155007: 'C:\.conan\qoas3sls\1' is not a working copy

To get rid of it, we wrote the following function, which we import from an external package via python_requires:

def is_folder_a_svn_working_copy(folder="."):
    return not(subprocess.check_output('svn info "%s"' % folder))

def get_scm(type="svn", url="auto", revision="auto", subfolder="."):
    scm = {
        "type": type,
        "url": url,
        "revision": revision,
        "subfolder": subfolder
    }
    if type == "svn" and (url == "auto" or revision == "auto":) and is_folder_a_svn_working_copy():
        svn = conans.client.tools.scm.SVN()
        if url == "auto":
            scm["url"] = svn._show_item('url')
        if revision == "auto":
            scm["revision"] = svn._show_item('revision')
    return scm

This is an acceptable workaround now and we are able to work with it. But it's not what we would expect from the auto feature. The auto feature behavior still depends on how the user has checked out his/her local working copy

@jgsogo
Copy link
Contributor

jgsogo commented Dec 24, 2018

Just a final comment on this issue. I'm investigating the is not a working copy warning and it is expected: when Conan needs a certain revision in a directory it has to check if there is already a repository there in order to checkout + update or only update the working copy. At this moment we are performing a svn info to check if there is a repository there already, and this command prints the warning... I cannot get rid of this message if I use SVN, I could look for a .svn folder, but I think it is safer to do this check calling SVN.

Any idea on this will be welcome. Thanks.

@ahauan4
Copy link
Author

ahauan4 commented Jan 5, 2019

Maybe you could redirect the stderr stream when calling svn info

@ahauan4
Copy link
Author

ahauan4 commented Jan 8, 2019

Here is a working function to suppress the output of svn info:

def is_folder_a_svn_working_copy(folder="."):
    process = subprocess.Popen('svn info "%s"' % folder, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
    process.communicate()
    return not(process.returncode)

@jgsogo
Copy link
Contributor

jgsogo commented Jan 9, 2019

Thanks! 😀 I will create a PR with this snippet, but the credit is all yours

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

Successfully merging a pull request may close this issue.

5 participants