Vendor speedscope with gem

[Envek]

Resolves #35

On gem release (via rake build) download built speedscope release and put it into vendor/speedscope. If there is no speedscope there for some reason, fallback to npx as before.

Caution

It makes built gem to increase in weight from ≈10kB to ≈230kB

Warning

Although latest speedscope release at the moment is 1.25.0 it doesn't include archive for built speedscope for some reason, so I set it to 1.24.0 for now. See jlfwong/speedscope#546

[dduugg]

PR Review: "Vendor speedscope with gem"

Goal

Resolves #35. Bundle a pre-built speedscope release directly into the gem so users don't need Node.js/npx at runtime. Falls back to npx speedscope if the vendored copy is absent.

---

Implementation

• Rakefile: Adds a speedscope:vendor task that downloads the speedscope release ZIP from GitHub, unpacks it into vendor/speedscope/, and hooks into rake build so it runs automatically. A speedscope:clobber task cleans it up.
• lib/singed/speedscope.rb (new): A Singed::Speedscope module encapsulating version constant, path detection, and two open strategies — vendored (no Node) and npx fallback.
• lib/singed/flamegraph.rb: open and open_command delegate to Singed::Speedscope instead of hardcoding npx.
• singed.gemspec: vendor/speedscope/**/* added to spec.files so it ships with the gem.
• Gemfile / Gemfile.lock: rubyzip added as a dev dependency for ZIP extraction during the build task.
• .gitignore: vendor/speedscope ignored (it's downloaded at build time, not checked in).

---

Risk Assessment

Low–Medium risk overall. A few things worth flagging:

1. open_command and open are inconsistent (potential confusion)

open_command builds a URL with #localProfilePath=<profile_path> pointing at the profile directly:

"#{os_open_command} file://#{bundled_index_html}#localProfilePath=#{profile_path}"

But open_with_bundled_speedscope writes a .js file and uses #localProfilePath=<js_path> pointing at the JS file instead. These two methods describe different behaviors, which is confusing and means open_command (used for the user-facing log message) doesn't match what open actually does.

2. Spaces and special characters in paths not escaped

open_command interpolates profile_path directly into a shell string without quoting:

"#{os_open_command} file://#{bundled_index_html}#localProfilePath=#{profile_path}"

Paths with spaces will break the command. The open method uses system(cmd, arg) form (safe), but open_command would be unsafe if passed to system directly.

3. rubyzip is a dev dependency but is required in Rakefile

rubyzip is added to Gemfile but not to the gemspec's add_development_dependency. This is fine since it's only needed by maintainers doing rake build, but worth a clarifying comment for contributors.

4. Gem size increase is significant but acknowledged

~10 kB → ~230 kB. Reasonable tradeoff for eliminating the Node.js requirement, and clearly noted in the PR.

5. Pinned to speedscope 1.24.0 due to upstream issue

1.25.0 doesn't include the ZIP artifact (upstream issue). Worth tracking — bump to 1.25.0+ once the upstream issue is resolved.

6. No tests for the new Singed::Speedscope module

The new module has meaningful branching logic (vendored vs. npx path, OS detection, HTML wrapper for macOS/Windows) with no spec coverage. Worth adding at minimum unit tests for open_command and the OS detection logic.

---

Summary

Solid, well-scoped improvement. Main items before merging:

• Fix the inconsistency between open_command (uses localProfilePath=<profile>) and open (uses localProfilePath=<js_file>)
• Add specs for Singed::Speedscope
• Consider quoting paths in open_command for robustness
• Add a TODO to bump to speedscope 1.25.0+ once the upstream ZIP issue is resolved

[Envek]

1. open_command and open are inconsistent (potential confusion)
2. Spaces and special characters in paths not escaped

Singed::Speedscope.open_command is only used to inform user on what is going on via puts in Object#flamegraph from the kernel_ext.rb so it should be readable, but shouldn't be 100% accurate

3. rubyzip is a dev dependency but is required in Rakefile
rubyzip is added to Gemfile but not to the gemspec's add_development_dependency.

Seems that specifying dev dependencies in gemspec is kind of obsolete and not really used in the wild: everything goes to the Gemfile

See discussions at ruby/rubygems#7799 and rubocop/rubocop#11469

5. Pinned to speedscope 1.24.0 due to upstream issue

Added a note to the speedscope version constant

6. No tests for the new Singed::Speedscope module

Added a spec

[dduugg]

PR Review (Updated): "Vendor speedscope with gem"

Goal

Resolves #35. Bundle a pre-built speedscope release directly into the gem so users don't need Node.js/npx at runtime. Falls back to npx speedscope if the vendored copy is absent.

---

What's Been Addressed Since the Last Review

• Tests added — spec/singed/speedscope_spec.rb covers open (both paths) and os_open_command with OS stubbing. Good coverage.
• open_command / open inconsistency — the author's clarification is reasonable: open_command is display-only (shown in puts in kernel_ext.rb), so being human-readable rather than mechanically accurate is acceptable. No change needed.
• Speedscope version note — added directly to the VERSION constant.

---

Remaining Issues

rubyzip dev dependency is inconsistent with the gemspec

The author argues that add_development_dependency in the gemspec is obsolete and everything should live in the Gemfile. That's a defensible position — but the gemspec for this repo still declares two dev dependencies:

# singed.gemspec
spec.add_development_dependency "rake", "~> 13.0"
spec.add_development_dependency "rspec"

So rubyzip (and standard, which is also Gemfile-only) are left out of the gemspec while rake and rspec remain in it. The inconsistency is the problem, not which approach is chosen. There are two clean options:

1. Add rubyzip (and standard) to gemspec dev deps — keeps everything in one place as the existing convention for this repo dictates.
2. Remove rake and rspec from the gemspec and consolidate all dev deps under group :development in Gemfile — aligns with the author's stated preference and the modern community direction.

Either is fine; the current half-and-half state is what should be resolved.

---

Leading space on frozen_string_literal magic comment

lib/singed/speedscope.rb:1:

 # frozen_string_literal: true   ← leading space

The magic comment must appear at the very first column to take effect. With the leading space, the file is not actually frozen. StandardRB will likely catch this, but it should be fixed explicitly.

---

Temp files are never cleaned up

lib/singed/speedscope.rb:55–60: The .js (and on macOS/Windows, .html) temp files written to Dir.tmpdir are never removed. They accumulate across profiling sessions. Using Tempfile (which auto-cleans on GC) or an ensure block that unlinks the file after the system call would be cleaner.

---

Summary

The core implementation is solid and the new specs are a nice addition. Three things left:

• Resolve the rubyzip dev dependency inconsistency — either add it to the gemspec alongside rake/rspec, or migrate all dev deps out of the gemspec into Gemfile
• Fix the leading space before # frozen_string_literal: true in speedscope.rb
• Clean up temp files after open_with_bundled_speedscope completes

[dduugg]

Suggested change

	# frozen_string_literal: true
	# frozen_string_literal: true

(minor, but i think this may actually impact freezing)