Refactoring #67
Refactoring #67
Conversation
|
Ready for the review. All tests should be passing now. |
|
Turned out this isn't ready yet. Closing for now. |
|
Re-opening since it's ready for the review. I tested on my forked repo and I think it's working as the same as before:
I feel like asking for taking a look at those results (if looking at the code is too much to ask). Could @ipa-mdl take a look, confirm that it's working in the same way from user's perspective? |
130s
force-pushed
the
exper/metapkg
branch
4 times, most recently
from
6d3db43
to
f941b68
Compare
|
@shaun-edwards @gavanderhoorn For PRs like this where there's no direct benefit to the users, I'm wondering who and how I ask reviews. Could any of you give me any idea? |
|
@130s, Isaac I'm happy to do reviews as a last resort. @ipa-mdl is probably a better reviewer, but if he's not available, I'm happy to help out. |
|
@shaun-edwards appreciated. Then could you look at 130s/industrial_core#6? That's a PR in my forked repo of industrial_core where I used this PR to run the Travis jobs. Results are all green and as far as I looked into each job log, everything seems to have run as it does without this PR. |
| @@ -51,31 +30,15 @@ FAQ | |||
|
|
|||
| A- The `industrial_ci` still provides valuable checks; it ensures if your package builds without issues. Also installation rules if you define. Just as a headsup that making test cases are highly recommended as your ear may hurt. | |||
|
|
|||
| - Q- My package uses a custom Point Cloud Library (PCL) version or the `industrial_calibration <https://github.com/ros-industrial/industrial_calibration>`_ package, how do I make build work? | |||
There was a problem hiding this comment.
Is there a reason you cut out these questions?
There was a problem hiding this comment.
Looks like it got somehow reverted. I've recovered the latest.
|
@130s, I verified the travis builds look similar, except for the obvious differences that result from these changes. It wasn't clear which variables were renamed/removed? The |
|
Just a couple of comments/questions. |
|
Thank you for taking a time and taking a look deeply. It must have been hard because of directory change (I should've made better commits). Variables removed are mentioned inline, all of which are only internal purpose and weren't mentioned README anyway. |
|
@130s, this looks good to me ( +1 ). You might consider adding some smart bash script to accept the new and old arguments or at the very least print out an informative message when the old variables are used. Since these variables are probably not used, I don't think it's a requirement. |
Omit entire catkin build section when devel space is not choosed to be used. Directory structure sorted out. Add another pkg solely for testing only. Conflicts: README.rst travis.sh [.travis.yml] Allow failure for now the jade source build (see ros-industrial/industrial_core#144 (comment)). [.travis.yml] Some more test matrix. [Travis] Rename a file to avoid confusion. Kinetic should better be no longer 'allow_failure'. warehouse_ros from shadow-fixed should pass. [Travis] Remove less meaningful job (warehouse_ros prerelease). More Kinetic jobs.
… pkg has changed. Now client repos don't need to use the fixed dir name '.ci_config'.
|
Sorry, I somehow couldn't receive +1 notification I actually got. Merging. |
Do not review yet. Comments are welcomed.This is just refactoring so no major feature for users is added, but I hope this change adds flexibility for the future maintenance (in fact I want to add major features based on this soon afterwards).
New feature
develspace can be used.Impact for the users
.travis.yml, you can use any sub folder name (currently fixed name.ci_configmust be used).Benefit for the maintenance
industrial_ciis no longer a unary stack, which allows this repository to contain multiple ROS packages for any purpose.