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

Remove several unused packages #143

Merged
merged 5 commits into from
Feb 7, 2022
Merged

Conversation

SyntaxColoring
Copy link
Contributor

@SyntaxColoring SyntaxColoring commented Feb 5, 2022

Overview

These commits deselect a bunch of unused packages in an attempt to free up some filesystem space. (See Opentrons/opentrons#8184.)

Unfortunately, I still don't think this is enough for a top-level make push to succeed, so Opentrons/opentrons#8184 will stay open.

Changelog

  • Removed Python packages for JSON-RPC.
    • Per @sfoster:

      it was a first attempt at making a nice network-transparent interface to the hardware controller for test engineering and others who want to directly interact with it

      i think we're moving away from the concept, you can remove it

    • Also removed chardet, a dependency of this.
  • Removed pyserial-asyncio. I couldn't find any instance of import serial_asyncio in Opentrons/opentrons.
  • Removed json-c, a library that isn't mandated by anything else that we have installed.
  • Removed a bunch of nginx modules that appeared unused to me, judging by our nginx.conf.
  • Removed sl. 😢

Review requests

Do any of the removed packages strike you as something that we should definitely not remove?

@SyntaxColoring SyntaxColoring added the robot-svcs Robot Services label Feb 5, 2022
@SyntaxColoring
Copy link
Contributor Author

Ideas for further work:

  • This PR added git, curl, and wget.

    Do we really need git? Do we really need both curl and wget, or can we just pick one?

  • Busybox has its own implementations of curl and/or wget (can't remember which, exactly). So we have two implementations installed. Can we get rid of one? Are we accidentally doing this with any other commands?

  • Should we strip test files from Python packages, sort of automating the workaround in bug: robot's filesystem is out of space,make push sometimes fails opentrons#8184?

Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

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

This looks good but I think there's more we can do like,

  • Strip out all the bluetooth stuff. this was a temporary experiment that at this point I think is not going to happen
  • if busybox has a curl let's use that; wget can still be useful, but i don't really care whether it happens or not
  • i do not remember at all why we added git, let's not
  • i doubt we need screen or tmux but they're also tiny so whatever

Stripping as much non-critical content from the python stuff is absolutely a good idea. We may also find it useful to install them as gzipped wheels to save disk space.

@@ -91,7 +85,29 @@ BR2_PACKAGE_DROPBEAR=y
BR2_PACKAGE_IW=y
BR2_PACKAGE_NETWORK_MANAGER=y
BR2_PACKAGE_NGINX=y
BR2_PACKAGE_NGINX_HTTP_SSL_MODULE=y
# BR2_PACKAGE_NGINX_HTTP_CHARSET_MODULE is not set
Copy link
Member

Choose a reason for hiding this comment

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

huh, stuff like this usually doesn't make it into a defconfig, wonder if nginx is doing something weird to parse these. would expect them to be BR2_PACKAGE_NGINX_HTTP_GZIP_MODULE=n and so on

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it is weird and mysterious.

@SyntaxColoring SyntaxColoring merged commit e9054c3 into opentrons-develop Feb 7, 2022
@SyntaxColoring SyntaxColoring deleted the fs_space branch February 7, 2022 15:22
@SyntaxColoring
Copy link
Contributor Author

Aggregating those further ideas in #144.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
robot-svcs Robot Services
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants