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

set zip_safe to avoid warning during installation #29

Merged
merged 1 commit into from
Mar 19, 2018
Merged

Conversation

dirk-thomas
Copy link
Contributor

@mikaelarguedas
Copy link
Contributor

Thanks these warnings were one on the only thing polluting stderr during builds.
Did you happen to run CI with this change to confirm everything work as zip ? or did you focus on changing the packages that would obviously work, to then (as a follow-up) do a more thorough investigation for the other python packages?

@dirk-thomas
Copy link
Contributor Author

When the flag is not specified the warning indicates what is being chosen based on the package content (e.g. usage of __file__). I only set the values based on the stderr output to what was already being determined by setuptools. Therefore I didn't run CI for the change since there shouldn't be any change - except the warnings going away for these packages.

@mikaelarguedas
Copy link
Contributor

Looking at the nightlies, packages in other repositories (ros2cli or examples) produce similar console pollution, but they are not covered by this set of PRs. So the question of how the packages targeted by this set of PRs were chosen still stands.

I'm fine merging those without review as without a job confirming the disappearance of the warnings or a summary of the console outputs that motivated this change; it is hard to review as it forces the reviewer to go dig them out from previous jobs output and compare the flag with what is set in this set of PRs.

@dirk-thomas
Copy link
Contributor Author

dirk-thomas commented Mar 17, 2018

There is always more to fix. I addressed the ones I notices. I won't spend time right now looking for more of these. But whenever I stumble over additional warnings in the future (as with any other problem) I will create more PRs.

Please feel free to run CI if you think it help in the review process. I don't think it will have any effect on passing / failing the build or the test results.

@mikaelarguedas
Copy link
Contributor

I opened the missing PRs as it made it easier to review the existing ones with a full job console output.
Job from master: https://ci.ros2.org/job/ci_turtlebot-demo_linux/115

  • 33 occurences of "zip_safe flag not set; analyzing archive"
  • 8 packages generating warnings (search for module references) saying that the flag should be set to False:
    • ament_flake8
    • ament_pep8
    • ament_copyright
    • ament_clang_format
    • ament_tools
    • ament_uncrustify
    • ros2cli
    • ros2pkg

Job after this set of PR: https://ci.ros2.org/job/ci_turtlebot-demo_linux/116

  • 0 occurences of "zip_safe flag not set; analyzing archive" meaning that we addressed all python packages that were missing the flag

@dirk-thomas
Copy link
Contributor Author

Awesome 👍

@dirk-thomas dirk-thomas merged commit fe52841 into master Mar 19, 2018
@dirk-thomas dirk-thomas deleted the zip_safe branch March 19, 2018 15:45
@dirk-thomas dirk-thomas removed the in review Waiting for review (Kanban column) label Mar 19, 2018
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