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

Allow spaces in path to Yarn binstub and only run on precompile if needed #40785

Merged
merged 3 commits into from
Dec 17, 2020

Conversation

doits
Copy link
Contributor

@doits doits commented Dec 10, 2020

Summary

Fixes #40783, #40795

Before the version check was introduced in bd4d8fd, the system call right below it failed silently in such cases and did nothing. Now the version check call raises Errno::ENOENT and acutally exposes the missing binary.

system("not present")
# => nil

`not present`
# Traceback (most recent call last):
#        2: from (irb):4
#        1: from (irb):4:in ``'
# Errno::ENOENT (No such file or directory - not)

One reason why it could be missing because Rails was upgraded but `rails
app:update` was not run.

Running `rails app:update:bin` should create it.

refs rails#40795
rescue Errno::ENOENT
$stderr.puts "bin/yarn was not found."
$stderr.puts "Please run `bundle exec rails app:update:bin` to create it."
exit 1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another issue #40795 popped up where apparently bin/yarn not exists. My assumption (to be confirmed) is that rails app:update was not run between Rails upgrades – which should create bin/yarn.

Therefore I added an error message to run rails app:update:bin in such cases where it should create bin/yarn.


One could also remove the exit 1 in such cases so it only prints out the warning but the pipeline continues. It was like this in before Rails 6.1, but I'm not sure if this was the best idea because if Rails requires Yarn to be present it should hard fail if there is something wrong.

@doits doits changed the title put yarn binary in quotes to allow spaces in path to it Allow spaces in path to Yarn binstub and add helpful error message if not found Dec 11, 2020
Based on discussion in rails#40795, it
looks like `yarn:install` is *always* run, even if the Rails project
disabled javascript and there is no `bin/yarn`.

Check for the existence of `bin/yarn` to decide if `yarn:install` should
be run or not.

The check for this is taken from `railties/lib/rails/app_updater.rb`,
where it does the same:

```ruby
         options[:skip_javascript] = !File.exist?(Rails.root.join("bin", "yarn"))
```
end
end

# Run Yarn prior to Sprockets assets precompilation, so dependencies are available for use.
if Rake::Task.task_defined?("assets:precompile")
if Rake::Task.task_defined?("assets:precompile") && File.exist?(Rails.root.join("bin", "yarn"))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

After further comments in #40795 it looks like yarn:install is always run, even if javascript was skipped in the Rails project and rails app:update:bin therefore does not generate bin/yarn.

I added this check to make sure bin/yarn exists. It is the same as done in the app updater:

options[:skip_javascript] = !File.exist?(Rails.root.join("bin", "yarn"))

Now it should really only be run automatically before compiling assets if bin/yarn is present.

@doits doits changed the title Allow spaces in path to Yarn binstub and add helpful error message if not found Allow spaces in path to Yarn binstub and only run on precompile if needed Dec 11, 2020
@doits
Copy link
Contributor Author

doits commented Dec 11, 2020

@rafaelfranca looks like bd4d8fd made some unintended noise for people updating to Rails 6.1 – I'm sorry for it – but I think I finally found a solution.

To summarize: When the task yarn:install was run and if something bad happened – e.g. no Yarn binstub was found – it failed silently before bd4d8fd, because only the system call was used. With the changes of bd4d8fd it now fails hard, because the version check uses backticks to capture the output which doesn't fail silently but raises an error in such cases.

First it was found out that the path to the Yarn binstub was not quoted correctly and failed if it had spaces in it (#40783) and the second issue came up where it runs yarn:install even though the Rails project was initialized with --skip-javascript and therefore does not have a Yarn binstub (#40795). Both those issues were present in Rails for a longer time but since yarn:install only failed silently nobody noticed before.

This PR fixes both issues by quoting the path to the Yarn binstub and by making assets:precompile to only run yarn:install if the Yarn binstub is present.

@mukinabis
Copy link

What temporary solution is suitable? I really need version 6.1, but because of this problem, I can't update.

@n-rodriguez
Copy link

n-rodriguez commented Dec 15, 2020

What temporary solution is suitable? I really need version 6.1, but because of this problem, I can't update.

create a bin/yarn bash script that returns true ;) (exit 0)

#!/bin/bash

exit 0

end
end

# Run Yarn prior to Sprockets assets precompilation, so dependencies are available for use.
if Rake::Task.task_defined?("assets:precompile")
if Rake::Task.task_defined?("assets:precompile") && File.exist?(Rails.root.join("bin", "yarn"))
Copy link
Member

Choose a reason for hiding this comment

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

The only way of this happen is if yarn is not installed and sprockets is, but --skip-javascript doesn't install or even load sprockets. I think this check here is valid though.

Copy link

Choose a reason for hiding this comment

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

I'm confused. Sprockets is still loaded for CSS, right?

Copy link
Member

Choose a reason for hiding this comment

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

Right. --skip-javascript is not --skip-assets. I was mixing both

@rafaelfranca rafaelfranca merged commit 002e802 into rails:master Dec 17, 2020
rafaelfranca added a commit that referenced this pull request Dec 17, 2020
Allow spaces in path to Yarn binstub and only run on precompile if needed
@thebravoman
Copy link

Thank you for the fix and merge.

@breisig
Copy link

breisig commented Dec 22, 2020

We need this fix to be added to the 6.1.x branch.

@doits
Copy link
Contributor Author

doits commented Dec 24, 2020

@breisig it is there already

brand-it added a commit to brand-it/plex_the_ripper that referenced this pull request Jan 17, 2021
brand-it added a commit to brand-it/plex_the_ripper that referenced this pull request Jul 16, 2021
* Turning this into a rack app that you can interact with

* More Code Cleanup

* WIP

* I am going full rails, I can build it from scrach with rack but I just end up building rails

* Removed a bunch of ben stubs

* Did some extra work got the UI working with webpack

* Got the Client to The Movie DB working

* More Rubocop cleanup

* Added Pagination

* Added Rspec

* More testing improvements to the STI for config

* Added a nice default serializer

* Added simple form

* More Rubocop cleanup

* Added A nice input field for simple form

* Got the input for working for config settings to make it easy to add the API key

* Added tables to track disk info

* Improved the testing coverage

* More Features to handle communication with the movie db

* Ability to have users with there own configs and preferences

* Add Ability to search for movies and TV shows

* cleaned up the work a bit

* Added the ability to create a movie from the The Movie DB api

* Added in wisper to handle callback in a way that is much cleaner

* Bundle update

* Code Cleanup thanks to rubocop

* Added stimulus which and a lot of other fixes

* Adding Plex config

* Added in plex config

* Rubocop fixes

* Trying out a remote upload tool

* Handle HTTP connections using farday and use FTP to stablish a remote connection

* Added TV and better error handling

* Added some test to the callbacks when selecting a tv show

* Now you can select an season and it will build the episodes

* Added a way to now get the episodes

* Made everything look a little nicer and added in still pic to episodes

* Removed old code that was used to find movie details

* Added a way for the system to parse disk titles

* Added a smart parser for the MKV output

* Cleaned up the code base to make things simple

* Rubocop fixes

* More testing around MKV parsing

* Added new workflow state called selected

* Added in the abilty to cancel the work flow

* Added abilty to select multiple TV shows on disk

* Thank goodness for concurrent-ruby, I can now process the disk info and then notify the FE with websockets

* Ability to load disk in the background without slowing down the disk selection process

* Removed disk channel that is not being used

* Added in a way to rip mkv files as well as a better handling of jobs

* Upgrade gems

* Fixed up test and make the jobs code a bit cleaner

* Rubocop fixes

* Annotated Models to make it easier to know what columns exist on a model

* Added assocation to movies and disk title id

* Improved the workflow so you have to have a disk_title before you can rip

* Got the create mkv sort of working

* Removed ruby-version file

* Fixed borken tests

* Update ruby.yml

* Run Yarn Install on testing

* Added Yarn Install to the workflow

* UGH

* Created a UI to get info on the worker status

* Assocate the Disk Title to the progress so we can store messages on the title

* Added messages to titles so we can print some useful info

* Trying out caching dependency in github actions

* Caching is built into the ruby/setup-ruby

* did not need to run bundle handle by ruby setup

* Added Rubocop into the mix to see what happens

* Cool it has badges nifity

* It works damit

* Added in some improbments to the boot process

* Update Gemfile

* Update .tool-versions

* code cleanup

* stimulus update

* stimulus cleanup

* Added Stimulus Reflex

* Improved the progress code and layouts

* Trying to make a better setup script

* Building out a MKV downloader to get the latest updates

* More Code Cleanup and better disk workers

* More Code Cleanup and upgrades

* Got the tests passing

* Got rubocop all happy, much cleaner

* More rubocop cleanup

* So much code cleanup needed

* Added github actions formater for rspec

* Added MKV Installer for Mac

* Fixed Yarn Packages use the newest versions

* Added a really easy to use setup script

* Added enable cache as part of the setup

* Don't use rake to enable cacke

* More syntax errors thanks to yours

* Going super low tech with adding this file

* Improved colors and link desc

* More database fixes along with a large number of tests

* Found more missed files that needed testing

* config_make_mkv now points to a stubbed version of makemkvcon

* More Tests more code quality improvements

* Made the readme a bit nicer and made it test a less

* renamed workerflow from ruby to status

* Renamed it to status

* Improved the validation of the config

* Fixed more code issues

* Improved DSL for config settings

* Movies & TV are the same thing so they got consolidate under Videos

* Merged Movies and Tvs into Vidoes table, really they are the same thing with super small diifs

* Tried out a new setup script

* Added a startup script and install ruby script

* Forgot to add fileutils

* Not giong to load Rails application

* change rake from a command to a another process it just simpler

* Make sure we boot the app in production mode

* Fixed MKV Downloader

* Yarn Install was not able to deal with space but bundle update fixed it rails/rails#40785

* Allow searcing of static assets

* Added quick start feature, saving application boot time

* Added error handling when a API key is invalid plus only have a single selected title

* Rubocop fixes

* Added a check to make sure selected does not effect other videos

* Broke up the tests a bit to make things a bit easier to read

* Added a system to select moves and improved query tools for searching the movie db

* Improved Video models and testing around videos

* Added caching around the search requests

* Got the progress bar working when selecting a movie

* Working out some better data tracking for movies

* Found some more test I needed to create

* Make the progress bar more useful and less generic

* Failed to make changed to the view component

* Switching over to hotwire turbo

* Full VCR update and move some code around to help cleanup the processing

* Changed the column time for movie runtime

* Removed more complexity with the state tracking

* Performance improvements around get request to the movie db

* Worked out a way to better cache and store movie titles

* More Improvements

* Get tests passing again

* Run Bundle update

* upgraded ruby to 2.7.3

* Upgraded to ruby 2.7.3 and fixed rubocop errors

* Guard against nil values when convert_min_to_seconds

* Pagination using show hotwire

* Reduce unused space

* Compact the UI more

* Improved Caching and UI to show more data

* Reduce the total results per page to just 4

* Made the pagination system more complex. UGH

* Got the disk list working again and removed the pagination tools

* Huge Upgrade and got progress bar working

* Create dependabot.yml

* Update dependabot.yml

* Disk Space warning along with checks to make sure files get created correctly

* Added Encryption for json config settings

* Fixed Broken tests and made the configuration tools gooder

* Use build in caching to avoid network calls for image data

* Cleanup and rspec tests

* Got plex ripper working

* Got it all working

* fixed rubocop
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Railsties yarn.rake fails when the Rails.root folder name has spaces in its name.
7 participants