Skip to content

Conversation

hyounesy
Copy link
Contributor

Proposed change(s)

changed the file mode for server_endpoint.py to executable on main. the file permission was already set correctly on dev, but seems like it was not merged properly.

Useful links (GitHub issues, JIRA tickets, forum threads, etc.)

Provide any relevant links here.

Types of change(s)

  • Bug fix
  • New feature
  • Code refactor
  • Documentation update
  • Other (please describe)

Testing and Verification

NA

Test Configuration:

NA

Checklist

  • Ensured this PR is up-to-date with the dev branch
  • Created this PR to target the dev branch
  • Followed the style guidelines as described in the Contribution Guidelines
  • Added tests that prove my fix is effective or that my feature works
  • Updated the documentation as appropriate

Other comments

@mrpropellers mrpropellers self-requested a review May 21, 2021 20:17
Copy link
Contributor

@mrpropellers mrpropellers left a comment

Choose a reason for hiding this comment

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

Sorry I approved too quickly, before fully reading the change. If this fix is already in dev but not in main, the solution is not to sneak the fix into main, but to go through the process of merging dev into main. Every time we do the former instead of the latter, it makes it that much harder for the next person to do it the correct way. Unless the change is high priority and time critical, it's not worth polluting the git history to push it in, no matter how simple.

@hyounesy
Copy link
Contributor Author

hyounesy commented May 21, 2021

Well, the problem is that the file mode is actually correct in dev, so I can't make any further change to it. and based on the git history, looks like somehow the file permission wasn't propagated to main in the last merge. The only alternative I can think of is to make some fake change and hope that in the next merge to main, the file mode will also transfer.

@hyounesy hyounesy requested a review from mrpropellers May 21, 2021 20:56
@mrpropellers
Copy link
Contributor

Hmmm so I inspected the history on this file and it looks like it doesn't stay consistently executable on dev:
https://github.com/Unity-Technologies/Unity-Robotics-Hub/blob/3ada90be450445fa6a286d8d3c986ee0c18feaac/tutorials/pick_and_place/ROS/src/niryo_moveit/scripts/server_endpoint.py (executable)
https://github.com/Unity-Technologies/Unity-Robotics-Hub/blob/ed5af6535d56725ab10d0a5109793d117f62c978/tutorials/pick_and_place/ROS/src/niryo_moveit/scripts/server_endpoint.py (not executable)
https://github.com/Unity-Technologies/Unity-Robotics-Hub/blob/f2dbd33473695db7e7fea894bfd57dba6e5da0c6/tutorials/pick_and_place/ROS/src/niryo_moveit/scripts/server_endpoint.py (executable again)

So looks like there's some issue with unix-based flags persisting through check-outs. This probably means that changing the flag directly on main isn't guaranteed to fix this. The more robust fix would be to add an explicit chmod +x for this file into the Dockerfile that builds this into the image, and potentially a note in the tutorial for users who are following the instructions with a native Ubuntu ROS install.

@iconnor
Copy link

iconnor commented May 27, 2021

I also encountered this bug - not sure if you fixed it and then it flipped back. If you are going to add a chmod +x to the docker file, can I suggest adding a check into the CI to fail a build that turns off the permission? You can then find the source of what is doing that (possibly a windows machine that is not configured properly?)

@hyounesy
Copy link
Contributor Author

added the chmod +x to the Dockerfile instead: #239

@hyounesy hyounesy closed this May 28, 2021
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.

3 participants