Fix #101

[allenh1]

Fix #101, ros/ros-overlay#30

Please stand by while I correct the git history.

[allenh1]

So, the loop isn't useful yet, but upon the addition of other potential repo types, the structure makes more sense.

[allenh1]

Git history fixed.

[allenh1]

@dirk-thomas Any idea what's going on with the nose tests? They worked fine on my end...

[dirk-thomas]

The code should either use a loop to check for different hosting providers or make the url replacement conditional. But not both imo.

[allenh1]

So the reason I went with this structure has to do with the way GitLab urls work. They are, potentially, unidentifiable (they don't need to contain the word 'gitlab').

Maybe a loop over just the different types of extensions would be best?

[dirk-thomas]

I don't think that trying N providers is feasible. Maybe if the provider is not identifiable by the url there needs to be a way to configure the mapping manually. The same is likely already the case for GitHub enterprise.

[allenh1]

So I removed the loop and left only the appropriate extension for BitBucket/GitLab.

This would only be a temporary fix, however. I think it would be best to include a raw_infix in the repo variable. Wouuld this be an optional flag in release yamls?

[dirk-thomas]

So I removed the loop and left only the appropriate extension for BitBucket/GitLab.

I am not convinced that just trying these URLs is a good idea.

however. I think it would be best to include a raw_infix in the repo variable. Wouuld this be an optional flag in release yamls?

I don't understand what you mean. Can you please clarify.

[allenh1]

I'm having a horrible time trying to articulate the idea, so I'll just show you what I would prefer to patch what I currently have with:

diff --git a/src/rosdistro/rosdistro.py b/src/rosdistro/rosdistro.py
index baae642..4fb7dd1 100644
--- a/src/rosdistro/rosdistro.py
+++ b/src/rosdistro/rosdistro.py
@@ -197,9 +197,9 @@ class RosPackage:
 
     def _fetch_package_xml(self, rosdistro):
         repo = self.repository
-        if 'github.com' in repo.url:
-            url = repo.url
-            release_tag = 'release/{0}/{1}/{2}'.format(rosdistro, self.name, repo.version)
+        url = repo.url
+        release_tag = 'release/{0}/{1}/{2}'.format(rosdistro, self.name, repo.version)
+        if 'github.com' in url:
             url = url.replace('.git', '/{0}/package.xml'.format(release_tag))
             url = url.replace('git://', 'https://')
             url = url.replace('https://', 'https://raw.')
@@ -222,11 +222,11 @@ class RosPackage:
             self._release_tags[rosdistro] = release_tag
             return package_xml, release_tag
         else:
-            release_tag = 'release/{0}/{1}/{2}'.format(rosdistro, self.name, repo.version)
-            url = repo.url
+            if repo.raw_infix is not None:
+                url = url.replace('.git', '/{0}/{1}/package.xml'.format(repo.raw_infix, release_tag))
+            else:
+                raise Exception('No raw_infix provided for non-github release repo!')
             try:
-                # URL extension for GitLab/BitBucket repos
-                url = url.replace('.git', '/raw/{0}/package.xml'.format(release_tag))
                 package_xml = urlopen(url).read()
                 self._package_xmls[rosdistro] = package_xml
                 self._release_tags[rosdistro] = release_tag

[dirk-thomas]

You want raw_infix to come from the distribution.yaml file? I am not sure that is viable. The format of that file is standardized in REP 143 and since a lot of tooling uses it any change would be a lot of effort.

I am not sure if that is even the right place. In that case that many repos share e.g. the same hostname I don't think this raw_infix should be attached to each of the entries.

[allenh1]

You want raw_infix to come from the distribution.yaml file? I am not sure that is viable. The format of that file is standardized in REP 143 and since a lot of tooling uses it any change would be a lot of effort.

Ya, I figured as much.

I am not sure if that is even the right place. In that case that many repos share e.g. the same hostname I don't think this raw_infix should be attached to each of the entries.

I'm rather unsure about it myself. I'm starting to understand why this issue still exists!

@tfoote Do you have any ideas? I cannot think of a good way to generalize this.

[allenh1]

So the decision was to make a file in the .ros directory to handle this, but I need to decide on a format for said file. Should I file a REP?

[tfoote]

This won't need a rep, it will be an optional extension point. Not a standard/required interface. I'd suggest using ~/.vcs_api_access

Use sections for the url matching

[bitbucket.org]
original=.git
replacement='/raw/{0}/package.xml'.format(release_tag)

Looking at the github implementation a slight variation that supports multiple remappings might be worth using.

In the future API keys could potentially be added to the sections too.

[mikepurvis]

There's a bit of logic like this in rosinstall_generator as well, for getting repo tarballs. It's based on a very simple "does git URL contain string X heuristic", which was enough for our usage:

https://github.com/ros-infrastructure/rosinstall_generator/blob/12e91bb32c155463e74ebd3fd2c7ed59861c64e6/src/rosinstall_generator/distro.py#L151-L165

It'd be cool to have this kind of abstraction is some kind of common dependency, but maintaining such a thing sounds like it'd be a gruelling, thankless task (see some related discussion in #77).

[dirk-thomas]

I will go ahead and close this PR due to the long time of inactivity. Please feel free to comment and if necessary the ticket can be reopened.