Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Catkin plugin: Use in-snap python instead of OS-provided. #232
Conversation
elopio
reviewed
Jan 14, 2016
| + pattern = re.compile(r'#!.*python') | ||
| + for root, directories, files in os.walk(self.rosdir): | ||
| + for fileName in files: | ||
| + with open(os.path.join(root, fileName), 'r+') as f: |
elopio
Jan 14, 2016
Member
first, the code inside the second for.
Then, maybe the two fors too, but this might be too much.
elopio
reviewed
Jan 14, 2016
| @@ -67,6 +68,8 @@ def build(self): | ||
| os.chmod(service_wrapper, 0o755) | ||
| + self._finish_build() |
elopio
Jan 14, 2016
Member
This feels like a bad name. It doesn't say what it does, it has many statements and no docstring.
Maybe something like _patch_snap_python? I'm not sure if that's better.
kyrofa
Jan 14, 2016
Member
Well that's sort of the point though-- there are two generic functions, _prepare_build() and _finish_build(). The names indicate when they run, not what they do (since what they do is kind of a smattering of things).
elopio
Jan 14, 2016
Member
Personally, I would find it more readable if you called that smattering of things one by one instead of grouping them in a function that will have more than one responsibility and with a name that will force you to read the code to understand what it's doing.
But I'm just leaving this as a suggestion, I'm not blocking your PR on this. I understand why you have to patch things afterwards, and I'm not expecting that to be pretty at all.
kyrofa
Jan 15, 2016
Member
I'll consider this in a future PR, but it's too heavy of a change right here.
elopio
reviewed
Jan 14, 2016
| + replaced = pattern.sub(r'python', f.read()) | ||
| + f.seek(0) | ||
| + f.truncate() | ||
| + f.write(replaced) |
elopio
Jan 14, 2016
Member
I think this is clearly composed of two blocks, so I would extract two methods. One like _fix_shebangs or _patch_shebangs. The other something like _use_snap_python.
If you find really nice names for them, maybe you can get rid of the call to _finish_build (which has a name that doesn't say much) by directly calling these methods.
I like small methods with nice names. Lots of them. Sometimes I go too far, so feel free to disagree :)
elopio
reviewed
Jan 14, 2016
| + with open(ros_profile, 'r') as f: | ||
| + self.assertEqual(f.read(), 'python foo', | ||
| + 'The absolute path to python was not replaced as ' | ||
| + 'expected') |
kyrofa
Jan 15, 2016
Member
I instead refactored the test into two, one testing the shebang rewrite and the other testing the absolute path rewrite. It can be refactored further at some point if we get rid of _finish/_prepare_build(), but that's for a different PR.
|
|
Once that is fixed you will run into: Due to the global regexing (I guess) |
|
@sergiusens oops, knew I did that refactor too quickly. And no, the reason roscore has that clean step as well is to avoid just that. |
added a commit
that referenced
this pull request
Jan 15, 2016
sergiusens
merged commit fe9ca74
into
snapcore:master
Jan 15, 2016
kyrofa
deleted the
kyrofa:bugfix/1533297/catkin_run_on_rolling
branch
Jan 21, 2016
pushed a commit
to kalikiana/snapcraft
that referenced
this pull request
Apr 6, 2017
kyrofa commentedJan 14, 2016
This works pre-Xenial since python2 is provided on the image. However, it's been removed in Xenial which uncovered this bug. The fix simply rewrites all /usr/bin/python shebangs to use /usr/bin/env python, and also fixes up 10.ros.sh to use python from the path instead of /usr/bin/python.
LP: #1533297