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

[Windows] execute the output of _setup_util.py in place #1116

Merged
merged 1 commit into from
Aug 26, 2020

Conversation

seanyen
Copy link
Contributor

@seanyen seanyen commented Aug 16, 2020

When different isolated setup.bat are invoked at the same time (for example, when building in parallel with colcon), the _SETUP_TMP has chances to end up with the same file path (the file name is generated by %Time: =0% and it doesn't give a good quality for uniqueness). And in this case, the file write (creation) and deletion can race together and result in unexpected errors.

This pull request is to address this problem by removing the need of the intermediate temp file and execute the generated batch commands in place directly.

Also, please consider to back-port to kinetic-devel if it makes sense. Thanks!

@seanyen
Copy link
Contributor Author

seanyen commented Aug 16, 2020

This is ready for review.

@seanyen
Copy link
Contributor Author

seanyen commented Aug 26, 2020

@dirk-thomas Please excuse me pinging you on this. This is ready for review and merge. We have seen some folks using colcon on noetic builds and this addition would be an improvement to the first time experience (so people don't need to limit the parallel build to 1 to workaround this.)

@dirk-thomas
Copy link
Member

Thanks for the patch.

@dirk-thomas dirk-thomas merged commit 044f53e into ros:noetic-devel Aug 26, 2020
@dirk-thomas
Copy link
Member

Cherry-picked to kinetic-devel in debf482.

@seanyen
Copy link
Contributor Author

seanyen commented Aug 26, 2020

Thanks for the quick response!

@seanyen seanyen deleted the patch-3 branch August 26, 2020 21:55
seanyen added a commit to ms-iot/catkin that referenced this pull request Aug 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants