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.

Sign up for GitHub

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

Jump to bottom

Fix tmp path for extern controllers #6113

Merged
merged 4 commits into from
May 2, 2023
Merged

Fix tmp path for extern controllers #6113

merged 4 commits into from
May 2, 2023

Conversation

ygoumaz
Copy link
Contributor

@ygoumaz ygoumaz commented May 1, 2023

This update addresses a previously missed case in issue #6103. Specifically, when connecting an external controller and the WEBOTS_CONTROLLER_URL is either undefined or only the robot name is provided, the Webots instance information (in particular the port number) needs to be determined. To achieve this, we search for open instances of Webots and select the first available instance. This process relies on the tmp directory, which has undergone changes on Linux and macOS since the implementation of #6103.

@ygoumaz ygoumaz added the bug Something isn't working label May 1, 2023
@ygoumaz ygoumaz self-assigned this May 1, 2023
@ygoumaz ygoumaz added this to the R2023b milestone May 1, 2023
@ygoumaz ygoumaz added test suite Start the test suite test sources Start the sources test on all platforms labels May 1, 2023
@ygoumaz ygoumaz marked this pull request as ready for review May 1, 2023 12:49
@ygoumaz ygoumaz requested a review from a team as a code owner May 1, 2023 12:49
@BenjaminDeleze BenjaminDeleze requested a review from omichel May 1, 2023 12:50
omichel
omichel requested changes May 1, 2023
View reviewed changes
src/controller/c/robot.c Show resolved Hide resolved
@ygoumaz ygoumaz requested a review from omichel May 1, 2023 13:37
omichel
omichel previously approved these changes May 1, 2023
View reviewed changes
Copy link
Member

@omichel omichel left a comment

Choose a reason for hiding this comment

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

Thank you.

@ygoumaz ygoumaz added test suite Start the test suite and removed test suite Start the test suite labels May 1, 2023
@ygoumaz
Copy link
Contributor Author

ygoumaz commented May 1, 2023

I can reproduce the test suite failure locally, but it doesn't seem to be related to this PR, as I can also reproduce it on the develop branch.

@ygoumaz ygoumaz mentioned this pull request May 2, 2023
Merged
9 tasks
@ygoumaz ygoumaz removed the test suite Start the test suite label May 2, 2023
@ygoumaz ygoumaz added the test suite Start the test suite label May 2, 2023
@ygoumaz
Copy link
Contributor Author

ygoumaz commented May 2, 2023

Test suite was broken since #6103 for two reasons:

  • The required permissions in WbVideoRecorder to record movies were removed.
  • The extern test was hanging indefinitely because it could not find the tmp directory.

@ygoumaz ygoumaz requested a review from omichel May 2, 2023 08:48
omichel
omichel approved these changes May 2, 2023
View reviewed changes
Copy link
Member

@omichel omichel left a comment

Choose a reason for hiding this comment

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

Great, thank you.

@ygoumaz ygoumaz merged commit 0969372 into develop May 2, 2023
@ygoumaz ygoumaz deleted the fix-tmp-path-extern-controllers branch May 2, 2023 09:01
@ygoumaz ygoumaz linked an issue May 2, 2023 that may be closed by this pull request
Check the permissions of the tmp folder on macOS #6099
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@omichel omichel omichel approved these changes

Assignees

@ygoumaz ygoumaz

Labels
bug Something isn't working test sources Start the sources test on all platforms test suite Start the test suite
Milestone
R2023b
Development

Successfully merging this pull request may close these issues.

Check the permissions of the tmp folder on macOS
2 participants
@ygoumaz @omichel