add script to check duplicates entry in path #15263

k-okada · 2017-06-17T10:22:41Z

as suggested at ros-infrastructure/rosdep#520 (review)
added argumetns to allow take local files

./scripts/check_duplicates.py rosdep/*.yaml

I also added this to test, but I'm afraid this is too aggresive

@tfoote

mikaelarguedas · 2017-06-17T19:50:08Z

please rebase on top on current master, to provide a PR with a single commit 67963d8

Please see the travis log to see the reason of the failure

k-okada · 2017-06-18T05:47:32Z

fixed and passed test, but that's strange, because the test should be failed.
may be we do need not run rosdep init & rosdep update to enable this test code.

k-okada · 2017-06-18T08:23:57Z

ok, fixed, now it fails as expected, https://travis-ci.org/ros/rosdistro/builds/244168788?utm_source=github_status&utm_medium=notification
need to merge #15260 #15259 to pass this test.


checking with ['indigo', 'ubuntu', 'trusty']
checking sources
- file:///home/travis/build/ros/rosdistro/indigo/distribution.yaml
- flie://home/travis/build/ros/rosdistro/rosdep/base.yaml
- flie://home/travis/build/ros/rosdistro/rosdep/gentoo.yaml
- flie://home/travis/build/ros/rosdistro/rosdep/python.yaml
- flie://home/travis/build/ros/rosdistro/rosdep/ruby.yaml
checking duplicates
* file:///home/travis/build/ros/rosdistro/indigo/distribution.yaml
* flie://home/travis/build/ros/rosdistro/rosdep/base.yaml
  ERR: pocketsphinx is multiply defined in
	file:///home/travis/build/ros/rosdistro/indigo/distribution.yaml and 
	flie://home/travis/build/ros/rosdistro/rosdep/base.yaml
* flie://home/travis/build/ros/rosdistro/rosdep/gentoo.yaml
* flie://home/travis/build/ros/rosdistro/rosdep/python.yaml
  ERR: xdot is multiply defined in
	file:///home/travis/build/ros/rosdistro/indigo/distribution.yaml and 
	flie://home/travis/build/ros/rosdistro/rosdep/python.yaml

k-okada · 2017-07-08T00:23:13Z

@mikaelarguedas @tfoote ik, now we passed the travis test so ready fo a reivew,
I confirmed this PR failes if we revert commit 2993ff6

* flie://home/travis/build/ros/rosdistro/rosdep/ruby.yaml
* flie://home/travis/build/ros/rosdistro/rosdep/python.yaml
  ERR: xdot is multiply defined in
	file:///home/travis/build/ros/rosdistro/indigo/distribution.yaml and 
	flie://home/travis/build/ros/rosdistro/rosdep/python.yaml
* sources.list
checking with ['jade', 'ubuntu', 'trusty']
checking sources

https://travis-ci.org/ros/rosdistro/builds/251355160

I have to install python-rosdep to run this feature, but I am not confident in this implementation, if someone know better way, let me know.

dirk-thomas · 2017-07-08T01:48:15Z

scripts/check_duplicates.py

+
+
+def print_err(msg):
+ printc(' ERR: ' + msg, 'red')


I would rather avoid adding this colorization logic just for this test. None of the other tests does it either.

dirk-thomas · 2017-07-08T01:52:01Z

scripts/check_duplicates.py

+ print_test('checking with %s'%(matcher.tags))
+ sources = [x for x in sources if matcher.matches(x)]
+ ret = ret & check_duplicates(sources)
+ return ret


Please return an integer from the main function which represents an error code or 0 if successful.

revert this commit 00fd688
It seems nosetest need to return True or False, not integer,

I am not sure I can follow: how is the main function being called from nosetests? nosetests should only pick up the test functions defined in rosdep_duplicates_test.py.

@dirk-thomas yes and I called main as check_duplicatesat the last of rosdep_duplicates_test.py
https://github.com/ros/rosdistro/pull/15263/files#diff-857189dd46c3a32ffe6a1424eaf41c5fR5

I just followed https://github.com/ros/rosdistro/blob/master/test/rosdep_formatting_test.py and https://github.com/ros/rosdistro/blob/master/scripts/check_rosdep.py style, so please let me know your recommended test and script code, so that I can follow your direction.

dirk-thomas · 2017-07-08T01:52:20Z

scripts/check_duplicates.py

+ parser.add_argument('infiles', nargs='*', help='input rosdep YAML file')
+ args = parser.parse_args()
+ if not main(args.infiles):
+ sys.exit(1)


This can just be sys.exit(main(...)) then.

dirk-thomas · 2017-07-08T01:53:33Z

test/rosdep_duplicates_test.py

+
+def test():
+ files = os.listdir('rosdep')
+ files = filter(lambda x: x.endswith('.yaml'), files) # accept only file ends with .yaml


Please use a list comprehension for this instead. Also please follow PEP 8 for the comments (two spaces + # + one space + comment text)

dirk-thomas · 2017-07-08T01:53:49Z

test/rosdep_duplicates_test.py

+ files = filter(lambda x: x.endswith('.yaml'), files) # accept only file ends with .yaml
+ files = [os.path.join('rosdep', x) for x in files] # use relative path
+ with Fold() as fold:
+ print("""Running 'scripts/check_duplicates.py' on all '*.yaml' in the 'rosdep' directory.


Please use single " here. No need to use """.

@dirk-thomas thank you for comment, I think I have fixed all parts

…de or 0 if successful.

…ext)

dirk-thomas · 2017-07-08T03:11:36Z

scripts/check_duplicates.py

+ rosdep_data = yaml.load(f.read())
+ # osx-homebrow uses xos tag
+ tag = 'osx' if 'osx-' in filepath else ''
+ model = CachedDataSource('yaml', 'flie:/'+filepath, [tag], rosdep_data)


Spelling: flie

dirk-thomas

CI is currently failing.

…error code or 0 if successful." This reverts commit 97c33af.

k-okada · 2017-07-15T01:38:24Z

@dirk-thomas thank you!!

dirk-thomas · 2017-07-15T02:17:54Z

Thank you for your effort coming up with the patch!

k-okada force-pushed the add_check_scripts branch 5 times, most recently from e922faa to 6e4c29a Compare June 18, 2017 05:46

k-okada force-pushed the add_check_scripts branch 7 times, most recently from 2307ccf to a8bddd7 Compare July 7, 2017 23:52

k-okada and others added 5 commits July 8, 2017 09:02

add script to check duplicates entry in path

5f25abc

install rosdep just for check_duplicates.py

b6bd704

update check_duplicates.py to use local yaml files

e6b7cb1

fix spaces scripts/check_duplicates.py

9055b37

forget to return result on check_duplicates.py

c68da8d

k-okada force-pushed the add_check_scripts branch from a8bddd7 to c68da8d Compare July 8, 2017 00:02

dirk-thomas reviewed Jul 8, 2017

View reviewed changes

k-okada added 5 commits July 8, 2017 11:34

avoid colorization logic

279f59f

return an integer from the main function which represents an error co…

97c33af

…de or 0 if successful.

follow PEP 8 for the comments (two spaces + # + one space + comment t…

ae3d34b

…ext)

use list comprehension

58330c5

use single ", No need to use """

9344f16

dirk-thomas reviewed Jul 8, 2017

View reviewed changes

k-okada and others added 5 commits July 8, 2017 12:14

fix typo flie -> file

0d68ecf

Revert "return an integer from the main function which represents an …

00fd688

…error code or 0 if successful." This reverts commit 97c33af.

minor nameing / style

15ee69d

style

808885a

more style

c35605f

dirk-thomas approved these changes Jul 14, 2017

View reviewed changes

dirk-thomas merged commit 3a6a34f into ros:master Jul 14, 2017

k-okada deleted the add_check_scripts branch July 15, 2017 01:38

dirk-thomas mentioned this pull request Jul 24, 2017

add python-pathtools, python-watchdog on xenial #15578

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add script to check duplicates entry in path #15263

add script to check duplicates entry in path #15263

k-okada commented Jun 17, 2017

mikaelarguedas commented Jun 17, 2017

k-okada commented Jun 18, 2017

k-okada commented Jun 18, 2017 •

edited

Loading

k-okada commented Jul 8, 2017

dirk-thomas Jul 8, 2017

dirk-thomas Jul 8, 2017

k-okada Jul 8, 2017

dirk-thomas Jul 10, 2017

k-okada Jul 11, 2017 •

edited

Loading

dirk-thomas Jul 8, 2017

dirk-thomas Jul 8, 2017

dirk-thomas Jul 8, 2017

k-okada Jul 8, 2017

dirk-thomas Jul 8, 2017

dirk-thomas left a comment

k-okada commented Jul 15, 2017

dirk-thomas commented Jul 15, 2017

add script to check duplicates entry in path #15263

add script to check duplicates entry in path #15263

Conversation

k-okada commented Jun 17, 2017

mikaelarguedas commented Jun 17, 2017

k-okada commented Jun 18, 2017

k-okada commented Jun 18, 2017 • edited Loading

k-okada commented Jul 8, 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

k-okada Jul 11, 2017 • edited Loading

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

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

k-okada commented Jul 15, 2017

dirk-thomas commented Jul 15, 2017

k-okada commented Jun 18, 2017 •

edited

Loading

k-okada Jul 11, 2017 •

edited

Loading