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

Make executables local #1286

Merged
merged 7 commits into from
Feb 23, 2018
Merged

Conversation

rossta
Copy link
Member

@rossta rossta commented Feb 22, 2018

Fixes #1281

Potentially breaking change as previously installed binstubs may be referencing a gem executable that may not be installed on the machine. Developers should re-run the install task to update their binstubs. They may also want to uninstall and reinstall the gem to remove the gem executables from their machines.

Replaces previous behavior of installing from bundle binstubs which rely
on globally installed executables that shadowed npm packages by the same
names.
@rossta rossta mentioned this pull request Feb 22, 2018
@@ -1,7 +1,7 @@
namespace :webpacker do
desc "Verifies that webpack & webpack-dev-server are present."
task :check_binstubs do
unless Bundler.which(Gem.win_platform? ? "webpack.bat" : "webpack")
unless File.exist?("bin/webpack") && File.exist?("bin/webpack-dev-server")
Copy link
Member

Choose a reason for hiding this comment

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

@rossta I think it's fine to leave out check for dev server since people might like to .gitignore it for production.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, right, I was reverting to older behavior.

@gauravtiwari
Copy link
Member

gauravtiwari commented Feb 22, 2018

Thanks for the PR, looks great. There are couple of places we need to replace bundle exec webpack call as well - compiler.rb and docker.md readme

@rossta
Copy link
Member Author

rossta commented Feb 22, 2018

@gauravtiwari I can update those references if you'd prefer them to use ./bin/webpack, but bundle exec webpack will actually still work within projects since Bundler will look in the bin directory first for a matching executable.

@gauravtiwari
Copy link
Member

gauravtiwari commented Feb 22, 2018

Ahh right but getting this:

(cached executable call)

bundler: failed to load command: webpack (/Users/admin/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/bin/webpack)
Gem::Exception: can't find executable webpack for gem webpacker
  /Users/admin/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/bundler-1.16.1/lib/bundler/rubygems_integration.rb:458:in `block in replace_bin_path'
  /Users/admin/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/bundler-1.16.1/lib/bundler/rubygems_integration.rb:478:in `block in replace_bin_path'
  /Users/admin/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/bin/webpack:23:in `<top (required)>'

which makes sense because we have removed the executables from gemspec so bundler has no way to know that an executable exists, no?

If I remove the file, then running the same command gives,

rbenv: webpack: command not found

The `webpack' command exists in these Ruby versions:
  2.4.2
  2.4.3

Note: I have copied over new binstubs in the bin/* folder

@rossta
Copy link
Member Author

rossta commented Feb 23, 2018

@gauravtiwari I forgot that I'm using the technique described here to add the local bin directory to $PATH.

@gauravtiwari
Copy link
Member

Thanks for the update - the last bit is prefixing all the ./bin/webpack or dev server calls (if any) with RbConfig.ruby so it works on windows too.

Open3.capture3(webpack_env, "#{RbConfig.ruby} ./bin/webpack")

@rossta
Copy link
Member Author

rossta commented Feb 23, 2018

@gauravtiwari Good call. I believe the only functional usage is the one I just updated.

@gauravtiwari gauravtiwari merged commit a5d3250 into rails:master Feb 23, 2018
@gauravtiwari
Copy link
Member

Looks good. Thanks @rossta 🍰 👍

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