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

Consolidate Tools into a single Binary #274

Merged
merged 15 commits into from
Feb 28, 2019

Conversation

charlespierce
Copy link
Contributor

Info

  • Currently, we are building separate binary packages for each of the core tools that we support (node, npm, npx, yarn), as well as for launchbin and launchscript.
  • Each of these binaries includes most of the core of the Notion functionality, so they are fairly redundant with each other.
  • As a result of including so many redundant binaries, the installer is quite large (~65 MB on Unix currently)
  • We can save installer space as well as make Notion more adaptable to new tools going forward by consolidating these into a single executable.

Changes

  • Updated the Tool trait and implementations to support receiving the command-line arguments as inputs, instead of getting them internally.
  • Updated Tool::exec to return a result, instead of exiting.
  • Added execute_tool function that parses the command-line arguments to determine which tool should be executed, then passes control over to that tool.
  • Removed redundant binaries in favor of a single shim binary.
  • Updated Unix installer to create symlinks to the shim binary named node, npm, npx, and yarn.
  • Updated Unix installer to handle the case of existing shims, by updating the symlink to point to shim instead of launchbin.
  • Refactored error display to reduce the duplication of if err.is_user_friendly() blocks.

Notes

  • Windows installer work wasn't tackled here. If Fix Windows Shim Issues and Add Full Windows Installer #268 is merged first then I will update this to include Windows support, if this is merged first I will update that one.
  • After this update, the Unix installer is ~21.5 MB, so it's roughly one-third the size.

Copy link
Contributor

@chriskrycho chriskrycho left a comment

Choose a reason for hiding this comment

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

This is a fantastic simplification and cleanup. I find it way easier to follow this way. None of the things I noticed here require changing, so I'm giving this a 👍.

Cargo.toml Show resolved Hide resolved
crates/notion-core/src/path/unix.rs Show resolved Hide resolved
crates/notion-core/src/style.rs Show resolved Hide resolved
crates/notion-core/src/tool/mod.rs Show resolved Hide resolved
crates/notion-core/src/tool/mod.rs Outdated Show resolved Hide resolved
crates/notion-core/src/tool/mod.rs Outdated Show resolved Hide resolved
crates/notion-core/src/tool/mod.rs Outdated Show resolved Hide resolved
@@ -100,6 +100,10 @@ pub fn execute_tool(session: &mut Session) -> Fallible<ExitStatus> {
let mut args = args_os();
let exe = get_tool_name(&mut args)?;

// There is some duplication in the calls to `.exec` here.
Copy link
Contributor

Choose a reason for hiding this comment

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

The explanatory comment you added here is great!

Copy link
Collaborator

@dherman dherman left a comment

Choose a reason for hiding this comment

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

This is a nice improvement! Love when the "-" count exceeds the "+" count :D

@dherman dherman merged commit 08fa783 into volta-cli:master Feb 28, 2019
@charlespierce charlespierce deleted the consolidate_binaries branch March 5, 2019 17:09
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.

4 participants