Changed order of the url check. #100

allenh1 · 2017-05-26T23:04:17Z

Fixes #99

mikaelarguedas · 2017-05-26T23:09:06Z

src/rosdistro/rosdistro.py

                    url = repo.url
                    release_tag = 'release/{0}/{1}/{2}'.format(rosdistro, self.name, repo.version)
                    tail = '/{0}/package.xml'.format(release_tag)
                    url = url.replace('.git', tail)
-                    url = url.replace('git://', 'https://')


Has this been removed intentionally ? If yes, why did this become unnecessary ?

It was not intentional. Good catch. Oddly, it works, but... I haven't tried it for indigo yet. One moment.

dirk-thomas

It makes sense to check the url of the new format first. It has been quite a while since this logic has been introduced (838b119) and by now the new format is the standard. So 👍 from me to invert the logic.

Instead of shuffling the logic I would suggest to just swap the lines 203 and 214.

dirk-thomas · 2017-05-26T23:23:27Z

My previous comment still applies: why shuffle all this code? Isn't it enough to swap these two lines:

rosdistro/src/rosdistro/rosdistro.py

Line 203 in d5f6f03

release_tag = 'release/{0}/{1}'.format(self.name, upstream_version)

and

rosdistro/src/rosdistro/rosdistro.py

Line 214 in d5f6f03

release_tag = 'release/{0}/{1}/{2}'.format(rosdistro, self.name, repo.version)

mikepurvis · 2017-05-26T23:32:58Z

src/rosdistro/rosdistro.py

+                    release_tag = 'release/{0}/{1}'.format(self.name, upstream_version)
+                    url = url.replace('.git', '/{0}/package.xml'.format(release_tag))
+                    url = url.replace('git://', 'https://')
+                    url = url.replace('https://', 'https://raw.')


This seems like a duplication of the more robust logic which is here:

https://github.com/ros-infrastructure/rosdistro/blob/master/src/rosdistro/manifest_provider/github.py#L65

Can we consolidate?

That would be a much better solution, yes. Let me do that.

Please keep the actual change swapping the order or urls fetched separate from code refactoring (like deduplicating logic). At least in separate commits - maybe even in separate PRs.

allenh1 · 2017-05-30T17:55:05Z

Upon further inspection, it appears that, in the function you linked, they call get_release_tag which calls
fetch package url.

https://github.com/ros-infrastructure/rosdistro/blob/master/src/rosdistro/rosdistro.py#L238

There seems to be some redundancy here in fetching the package.xml file. In particular, it happens twice. So I think the best consolidation here would be to modify the github_manifest_provider to not fetch the package.xml file, as it is already available.

mikepurvis · 2017-05-30T18:00:16Z

In that case you'd want to use the CachedManifestProvider, as in:

rosdistro/src/rosdistro/__init__.py

Lines 141 to 144 in d5f6f03

    
           dist = Distribution( 
        
               cache.distribution_file, 
        
               [CachedManifestProvider(cache, Distribution.default_manifest_providers if allow_lazy_load else None)], 
        
               [CachedSourceManifestProvider(cache, Distribution.default_source_manifest_providers if allow_lazy_load else None)])

However, looks like it might be some crazy legacy-supporting side logic, so let's wait for a verdict from @dirk-thomas about the right path forward before you try to change too much.

allenh1 · 2017-05-30T18:21:13Z

Will do. I was mistaken by the structure a little -- I will play around with it some, then bother dirk in a few.

allenh1 · 2017-05-30T19:05:35Z

@dirk-thomas Should be the correct format now.

@mikepurvis After digging through things for a good while, I've determined that the amount of refactoring needed to avoid duplication of this logic reaches beyond the bounds of this pull request.

dirk-thomas · 2017-05-30T19:36:37Z

src/rosdistro/rosdistro.py

-            upstream_version = repo.version.split('-')[0]
-            release_tag = 'release/{0}/{1}'.format(self.name, upstream_version)
-            url = url.replace('.git', '/{0}/package.xml'.format(release_tag))
+            # release_tag = get_release_tag(self.name)


This line should be removed.

dirk-thomas · 2017-05-30T19:37:40Z

src/rosdistro/rosdistro.py

            try:
                try:
+                    release_tag = 'release/{0}/{1}/{2}'.format(rosdistro, self.name, repo.version)
+                    url = url.format(release_tag)


Before format was only called on strings defined in the source code. Now format is being called on a user provided string which might contain characters like {} and therefore result in problems. Therefore please only use format on strings defined in the code directly.

Same below in line 218:

url = url.format(release_tag)

dirk-thomas · 2017-05-30T19:38:01Z

src/rosdistro/rosdistro.py

                    package_xml = urlopen(url).read()
-                except Exception as e:
+                except Exception as e:                    


Please remove the trailing whitespace.

dirk-thomas · 2017-05-30T19:39:52Z

src/rosdistro/rosdistro.py

            try:
                try:
+                    release_tag = 'release/{0}/{1}/{2}'.format(rosdistro, self.name, repo.version)


This line as well as the next one shouldn't be in the try / except block.

allenh1 · 2017-05-30T20:09:22Z

@dirk-thomas I restructured the logic a little to fix it. Should be good now.

dirk-thomas · 2017-05-30T20:38:53Z

src/rosdistro/rosdistro.py

-            upstream_version = repo.version.split('-')[0]
-            release_tag = 'release/{0}/{1}'.format(self.name, upstream_version)
-            url = url.replace('.git', '/{0}/package.xml'.format(release_tag))
+            tail = 'release/{0}/{1}/{2}/package.xml'.format(rosdistro, self.name, repo.version)


I would suggest to keep the variable name as release_tag since the string is the release tag.

dirk-thomas · 2017-05-30T20:40:48Z

src/rosdistro/rosdistro.py

-                    url = url.replace('https://', 'https://raw.')
-                    info("Trying to read from url '{0}' instead".format(url))
+                    upstream_version = repo.version.split('-')[0]
+                    legacy_tail = 'release/{0}/{1}/package.xml'.format(self.name, upstream_version)


I would suggest to rename the variable to legacy_release_tag.

@dirk-thomas you got it

Improved

dirk-thomas · 2017-05-31T18:49:51Z

The previous state seem to lack the leading slash in the replacement: see 1c6eb29#diff-65e54a8cd0a3220cd2b0b2091bd58708R202

Anyway I added two commits to this PR to minimize the changes and keep the semantic of the variables (release_tag and legacy_release_tag are actualy containing valid tag names of the repo). Please double check that the overall patch still works as expected.

allenh1 · 2017-06-01T16:38:25Z

@dirk-thomas All is working as expected!

dirk-thomas · 2017-06-01T18:46:19Z

Thanks!

mikaelarguedas reviewed May 26, 2017

View reviewed changes

dirk-thomas suggested changes May 26, 2017

View reviewed changes

allenh1 force-pushed the master branch from a64650c to 75dd4b6 Compare May 26, 2017 23:19

mikepurvis reviewed May 26, 2017

View reviewed changes

allenh1 force-pushed the master branch 2 times, most recently from 6141662 to fc89de1 Compare May 30, 2017 19:03

dirk-thomas previously requested changes May 30, 2017

View reviewed changes

allenh1 force-pushed the master branch from fc89de1 to f9c5ce3 Compare May 30, 2017 19:57

dirk-thomas reviewed May 30, 2017

View reviewed changes

Changed order of the url check.

1c6eb29

allenh1 force-pushed the master branch from f9c5ce3 to 1c6eb29 Compare May 30, 2017 20:43

dirk-thomas added 2 commits May 31, 2017 11:46

update

dd2d2c0

revert whitespace change

c2b9271

dirk-thomas added the enhancement label Jun 1, 2017

dirk-thomas merged commit c4f076d into ros-infrastructure:master Jun 1, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changed order of the url check. #100

Changed order of the url check. #100

allenh1 commented May 26, 2017

mikaelarguedas May 26, 2017

allenh1 May 26, 2017

dirk-thomas left a comment

dirk-thomas commented May 26, 2017

mikepurvis May 26, 2017

allenh1 May 30, 2017

dirk-thomas May 30, 2017

allenh1 commented May 30, 2017

mikepurvis commented May 30, 2017

allenh1 commented May 30, 2017

allenh1 commented May 30, 2017

dirk-thomas May 30, 2017

dirk-thomas May 30, 2017

dirk-thomas May 30, 2017

dirk-thomas May 30, 2017

allenh1 commented May 30, 2017

dirk-thomas May 30, 2017

dirk-thomas May 30, 2017

allenh1 May 30, 2017

dirk-thomas commented May 31, 2017

allenh1 commented Jun 1, 2017

dirk-thomas commented Jun 1, 2017

Changed order of the url check. #100

Changed order of the url check. #100

Conversation

allenh1 commented May 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dirk-thomas left a comment

Choose a reason for hiding this comment

dirk-thomas commented May 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allenh1 commented May 30, 2017

mikepurvis commented May 30, 2017

allenh1 commented May 30, 2017

allenh1 commented May 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allenh1 commented May 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dirk-thomas commented May 31, 2017

allenh1 commented Jun 1, 2017

dirk-thomas commented Jun 1, 2017