Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecation utils #285

Merged
merged 3 commits into from
Jul 11, 2018
Merged

Conversation

mathias-luedtke
Copy link
Member

No description provided.

@mathias-luedtke mathias-luedtke requested a review from 130s June 3, 2018 17:40
Copy link
Member

@130s 130s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this looks to make the code cleaner and result more readable. Just a few concerns left in-line.

@@ -4,15 +4,12 @@ ABICHECK_VERSION
ADDITIONAL_DEBS
AFTER_SCRIPT
BEFORE_SCRIPT
BUILDER
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure this is no longer needed? If so please update the document as well.

$ git log -1
commit 0333531cf1fcbcc3fb678c3abd17de23b2185ad5
Merge: a9dbd03 d6d5cbd
Author: Isaac I.Y. Saito <[email protected]>
Date:   Tue May 8 09:21:02 2018 -0700

$ ack-grep -i "BUILDER" .
doc/index.rst
172:Note that some of these currently tied only to a single option, but we still leave them for the future when more options become available (e.g. ament with BUILDER).
181:* **BUILDER** (default: catkin): Currently only `catkin` is implemented (and with that `catkin_tools` is used instead of `catkin_make`. See `this discussion <https://github.com/ros-industrial/industri
al_ci/issues/3>`_).
204:* **ROS_PARALLEL_JOBS** (default: -j8): Maximum number of packages to be built in parallel by the underlining build tool. As of Jan 2016, this is only enabled with `catkin_tools` (with `make` as an un
derlining builder).
205:* **ROS_PARALLEL_TEST_JOBS** (default: -j8): Maximum number of packages which could be examined in parallel during the test run by the underlining build tool. If not set it's filled by `ROS_PARALLEL_J
OBS`. As of Jan 2016, this is only enabled with `catkin_tools` (with `make` as an underlining builder).

industrial_ci/src/docker.env
7:BUILDER

industrial_ci/src/tests/source_tests.sh
43:BUILDER=catkin
178:if [ "$BUILDER" == catkin ]; then catkin build $OPT_VI --summarize  --no-status $BUILD_PKGS_WHITELIST $CATKIN_PARALLEL_JOBS --make-args $ROS_PARALLEL_JOBS            ; fi
184:    if [ "$BUILDER" == catkin ]; then
190:    if [ "$BUILDER" == catkin ]; then
196:    if [ "$BUILDER" == catkin ]; then
203:            if [ "$BUILDER" == catkin -a -e $ROS_LOG_DIR ]; then catkin_test_results --all $ROS_LOG_DIR || error; fi
204:            if [ "$BUILDER" == catkin -a -e $CATKIN_WORKSPACE/build/ ]; then catkin_test_results --all $CATKIN_WORKSPACE/build/ || error; fi
205:            if [ "$BUILDER" == catkin -a -e ~/.ros/test_results/ ]; then catkin_test_results --all ~/.ros/test_results/ || error; fi
219:    if [ "$BUILDER" == catkin ]; then
$

Copy link
Member Author

@mathias-luedtke mathias-luedtke Jun 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

100% sure, it get's hardcoded in line 43 anyway.
However, I want to implement this later. That's why I opted to keep the doc entry as-is.

@@ -31,7 +28,6 @@ PRERELEASE_REPONAME
PYTHONUNBUFFERED
ROSDEP_SKIP_KEYS
ROSINSTALL_FILENAME
ROSWS
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about this removal. If this is legit please update the document as well.

$ ack-grep -i "ROSWS" .
doc/index.rst
210:* **ROSWS** (default: wstool): Currently only `wstool` is available.

industrial_ci/src/docker.env
34:ROSWS

industrial_ci/src/tests/source_tests.sh
44:ROSWS=wstool
87:ici_time_start setup_rosws
94:  $ROSWS init $CATKIN_WORKSPACE/src
104:        $ROSWS merge -t $CATKIN_WORKSPACE/src file://$TARGET_REPO_PATH/$ROSINSTALL_FILENAME.$ROS_DISTRO
107:        $ROSWS merge -t $CATKIN_WORKSPACE/src file://$TARGET_REPO_PATH/$ROSINSTALL_FILENAME
113:    $ROSWS merge -t $CATKIN_WORKSPACE/src $UPSTREAM_WORKSPACE
120:    (cd $CATKIN_WORKSPACE/src; $ROSWS rm $TARGET_REPO_NAME 2> /dev/null \
121:     && echo "$ROSWS ignored $TARGET_REPO_NAME found in $CATKIN_WORKSPACE/src/.rosinstall file. Its source fetched from your repository is used instead." || true) # TODO: add warn function
122:    $ROSWS update -t $CATKIN_WORKSPACE/src
137:ici_time_end  # setup_rosws

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The doc entry slipped through.
Why do you want to keep it?

@130s 130s merged commit 7b916f7 into ros-industrial:master Jul 11, 2018
@mathias-luedtke mathias-luedtke deleted the feature/deprecation branch July 11, 2018 19:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants