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

adding check to prevent tool cross talk #214

Merged
merged 4 commits into from
Dec 9, 2015
Merged

adding check to prevent tool cross talk #214

merged 4 commits into from
Dec 9, 2015

Conversation

wjwwood
Copy link
Member

@wjwwood wjwwood commented Jun 26, 2015

@tfoote
Copy link
Contributor

tfoote commented Jun 27, 2015

+1

The unit tests will need to be rerun once catkin_pkg is released with the new version.

@jbohren
Copy link
Contributor

jbohren commented Jun 27, 2015

Should this be done here or in build_isolated_workspace() in build.py?

@wjwwood
Copy link
Member Author

wjwwood commented Jun 27, 2015

I wanted to have the check occur before the dry_run function, now I realize that the mark_space_as_built_by functions should also be after the dry_run function.

@jbohren
Copy link
Contributor

jbohren commented Jun 28, 2015

Ok then we should probably move the other check here too.

@dirk-thomas
Copy link
Contributor

This has been addressed in catkin_pkg and catkin which have both been released since then. I have closed the related tickets in these repos. Once catkin_tools merges and releases this check it would actually work.

@@ -142,6 +143,8 @@ def prepare_arguments(parser):
help='Adds a build summary to the end of a build; defaults to on with --continue-on-failure, off otherwise')
add('--no-summarize', '--no-summary', action='store_false', dest='summarize',
help='explicitly disable the end of build summary')
add('--override-build-tool-check',
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't there be , action='store_true', default=False,?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, 7b7ffc0.

@NikolausDemmel
Copy link
Member

Ok then we should probably move the other check here too.

which check exactly?

I wanted to have the check occur before the dry_run function, now I realize that the mark_space_as_built_by functions should also be after the dry_run function.

Why does it need to be moved after dry_run?

@NikolausDemmel
Copy link
Member

Could one of the owners retrigger the tests, please?

@wjwwood
Copy link
Member Author

wjwwood commented Dec 9, 2015

Why does it need to be moved after dry_run?

Because otherwise running catkin build -n on an empty workspace will result in the creation of a build and devel folder marked as catkin build's but with nothing else in them.

So now this is the case:

  • Run catkin_make, succeeds
  • Run catkin build, fails
  • Run catkin build -n, fails
  • Run catkin build -n --override-build-tool-check, warns but doesn't build
  • Run catkin build -n --override-build-tool-check, still warns but doesn't build
  • Run catkin build --override-build-tool-check, warns but builds
  • Run catkin build --override-build-tool-check, no warning and builds
  • rm -rf build devel
  • Run catkin build -n, no build or devel folder created
  • Run catkin build , builds and therefore build and devel folder created

I've tested this with both catkin_make and catkin_make_isolated locally. Works for me. I also removed the extra checks which I think was what @jbohren was talking about, but he can comment if that wasn't the case.

wjwwood added a commit that referenced this pull request Dec 9, 2015
adding check to prevent tool cross talk
@wjwwood wjwwood merged commit 5de9934 into master Dec 9, 2015
@wjwwood wjwwood deleted the cross_tool_check branch December 9, 2015 18:01
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.

5 participants