From 83af3e594f561c4f1c3c35b59f34089b2843a887 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Mon, 17 Jun 2013 13:49:40 -0400 Subject: [PATCH 1/8] Use built-in rakelibdir feature Rake by default imports all .rake files in the rakelib dir, so we can use that rather than doing our own import loop. --- rakefile | 6 +----- {rakefiles => rakelib}/assets.rake | 0 {rakefiles => rakelib}/deploy.rake | 0 {rakefiles => rakelib}/deprecated.rake | 0 {rakefiles => rakelib}/django.rake | 0 {rakefiles => rakelib}/docs.rake | 0 {rakefiles => rakelib}/helpers.rb | 0 {rakefiles => rakelib}/i18n.rake | 0 {rakefiles => rakelib}/jasmine.rake | 0 {rakefiles => rakelib}/prereqs.rake | 2 -- {rakefiles => rakelib}/quality.rake | 0 {rakefiles => rakelib}/tests.rake | 0 {rakefiles => rakelib}/workspace.rake | 0 13 files changed, 1 insertion(+), 7 deletions(-) rename {rakefiles => rakelib}/assets.rake (100%) rename {rakefiles => rakelib}/deploy.rake (100%) rename {rakefiles => rakelib}/deprecated.rake (100%) rename {rakefiles => rakelib}/django.rake (100%) rename {rakefiles => rakelib}/docs.rake (100%) rename {rakefiles => rakelib}/helpers.rb (100%) rename {rakefiles => rakelib}/i18n.rake (100%) rename {rakefiles => rakelib}/jasmine.rake (100%) rename {rakefiles => rakelib}/prereqs.rake (98%) rename {rakefiles => rakelib}/quality.rake (100%) rename {rakefiles => rakelib}/tests.rake (100%) rename {rakefiles => rakelib}/workspace.rake (100%) diff --git a/rakefile b/rakefile index 20101a14db..3fcd16f995 100644 --- a/rakefile +++ b/rakefile @@ -1,10 +1,6 @@ require 'json' require 'rake/clean' -require './rakefiles/helpers.rb' - -Dir['rakefiles/*.rake'].each do |rakefile| - import rakefile -end +require './rakelib/helpers.rb' # Build Constants REPO_ROOT = File.dirname(__FILE__) diff --git a/rakefiles/assets.rake b/rakelib/assets.rake similarity index 100% rename from rakefiles/assets.rake rename to rakelib/assets.rake diff --git a/rakefiles/deploy.rake b/rakelib/deploy.rake similarity index 100% rename from rakefiles/deploy.rake rename to rakelib/deploy.rake diff --git a/rakefiles/deprecated.rake b/rakelib/deprecated.rake similarity index 100% rename from rakefiles/deprecated.rake rename to rakelib/deprecated.rake diff --git a/rakefiles/django.rake b/rakelib/django.rake similarity index 100% rename from rakefiles/django.rake rename to rakelib/django.rake diff --git a/rakefiles/docs.rake b/rakelib/docs.rake similarity index 100% rename from rakefiles/docs.rake rename to rakelib/docs.rake diff --git a/rakefiles/helpers.rb b/rakelib/helpers.rb similarity index 100% rename from rakefiles/helpers.rb rename to rakelib/helpers.rb diff --git a/rakefiles/i18n.rake b/rakelib/i18n.rake similarity index 100% rename from rakefiles/i18n.rake rename to rakelib/i18n.rake diff --git a/rakefiles/jasmine.rake b/rakelib/jasmine.rake similarity index 100% rename from rakefiles/jasmine.rake rename to rakelib/jasmine.rake diff --git a/rakefiles/prereqs.rake b/rakelib/prereqs.rake similarity index 98% rename from rakefiles/prereqs.rake rename to rakelib/prereqs.rake index ff8b4b8784..e06d411435 100644 --- a/rakefiles/prereqs.rake +++ b/rakelib/prereqs.rake @@ -1,5 +1,3 @@ -require './rakefiles/helpers.rb' - PREREQS_MD5_DIR = ENV["PREREQ_CACHE_DIR"] || File.join(REPO_ROOT, '.prereqs_cache') CLOBBER.include(PREREQS_MD5_DIR) diff --git a/rakefiles/quality.rake b/rakelib/quality.rake similarity index 100% rename from rakefiles/quality.rake rename to rakelib/quality.rake diff --git a/rakefiles/tests.rake b/rakelib/tests.rake similarity index 100% rename from rakefiles/tests.rake rename to rakelib/tests.rake diff --git a/rakefiles/workspace.rake b/rakelib/workspace.rake similarity index 100% rename from rakefiles/workspace.rake rename to rakelib/workspace.rake From d9268acd6bb9b19175315998de623818e0a799f1 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Mon, 17 Jun 2013 13:52:00 -0400 Subject: [PATCH 2/8] Provide instructions of ruby imports fail in rake `rake install_prereqs` requires a minimal level of ruby and rake already installed. If it doesn't exist, then print out a helpful message indicating next steps. --- CHANGELOG.rst | 2 ++ rakefile | 12 +++++++++--- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 7fc07a3f19..9c7af1520b 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -5,6 +5,8 @@ These are notable changes in edx-platform. This is a rolling list of changes, in roughly chronological order, most recent first. Add your entries at or near the top. Include a label indicating the component affected. +Common: Make rake provide better error messages if packages are missing. + Common: Repairs development documentation generation by sphinx. LMS: Problem rescoring. Added options on the Grades tab of the diff --git a/rakefile b/rakefile index 3fcd16f995..96bd4c2e96 100644 --- a/rakefile +++ b/rakefile @@ -1,6 +1,12 @@ -require 'json' -require 'rake/clean' -require './rakelib/helpers.rb' +begin + require 'json' + require 'rake/clean' + require './rakelib/helpers.rb' +rescue LoadError => error + puts "Import faild (#{error})" + puts "Please run `bundle install` to bootstrap ruby dependencies" + exit 1 +end # Build Constants REPO_ROOT = File.dirname(__FILE__) From d99ad53ae90bebc0fb2c099b42a2ffa1c64ab4b8 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Wed, 19 Jun 2013 15:30:47 -0400 Subject: [PATCH 3/8] Add system and env arguments to asset tasks The preprocess task requires system and env arguments in order to correctly load up the django environment to preprocess sass files to inject themes. In order for that task to recieve the arguments, all tasks that depend on it also have to accept that same set of arguments. This will all go away once the next evolution of themes arrives, which will remove the preprocessing needed to inject theme names. --- rakelib/assets.rake | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/rakelib/assets.rake b/rakelib/assets.rake index 764d049a68..0c58047bc2 100644 --- a/rakelib/assets.rake +++ b/rakelib/assets.rake @@ -55,8 +55,9 @@ def sass_cmd(watch=false, debug=false) "#{watch ? '--watch' : '--update'} -E utf-8 #{sass_watch_paths.join(' ')}" end +# This task takes arguments purely to pass them via dependencies to the preprocess task desc "Compile all assets" -multitask :assets => 'assets:all' +task :assets, [:system, :env] => 'assets:all' namespace :assets do @@ -80,8 +81,9 @@ namespace :assets do {:xmodule => [:install_python_prereqs], :coffee => [:install_node_prereqs, :'assets:coffee:clobber'], :sass => [:install_ruby_prereqs, :preprocess]}.each_pair do |asset_type, prereq_tasks| + # This task takes arguments purely to pass them via dependencies to the preprocess task desc "Compile all #{asset_type} assets" - task asset_type => prereq_tasks do + task asset_type, [:system, :env] => prereq_tasks do |t, args| cmd = send(asset_type.to_s + "_cmd", watch=false, debug=false) if cmd.kind_of?(Array) cmd.each {|c| sh(c)} @@ -90,7 +92,8 @@ namespace :assets do end end - multitask :all => asset_type + # This task takes arguments purely to pass them via dependencies to the preprocess task + multitask :all, [:system, :env] => asset_type multitask :debug => "assets:#{asset_type}:debug" multitask :_watch => "assets:#{asset_type}:_watch" From 2e480f6404b2ae640623266a4cbd9f91dfa067c6 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Mon, 17 Jun 2013 15:00:55 -0400 Subject: [PATCH 4/8] Switch to standard coffee watcher Using `ulimit -n` to set the limit much higher than the default of 256 in Darwin seems to avoid the `EMFILE` error that was plaguing our Mac developers. --- CHANGELOG.rst | 2 ++ doc/development.md | 19 +++++++++++++++++++ rakelib/assets.rake | 26 +++++++++----------------- 3 files changed, 30 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 9c7af1520b..99accb83cb 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -5,6 +5,8 @@ These are notable changes in edx-platform. This is a rolling list of changes, in roughly chronological order, most recent first. Add your entries at or near the top. Include a label indicating the component affected. +Common: Use coffee directly when watching for coffeescript file changes. + Common: Make rake provide better error messages if packages are missing. Common: Repairs development documentation generation by sphinx. diff --git a/doc/development.md b/doc/development.md index c99e99f906..e6ab650002 100644 --- a/doc/development.md +++ b/doc/development.md @@ -63,6 +63,25 @@ To get a full list of available rake tasks, use: rake -T +### Troubleshooting + +#### Reference Error: XModule is not defined (javascript) +This means that the javascript defining an xmodule hasn't loaded correctly. There are a number +of different things that could be causing this: + +1. See `Error: watch EMFILE` + +#### Error: watch EMFILE (coffee) +When running a development server, we also start a watcher process alongside to recompile coffeescript +and sass as changes are made. On Mac OSX systems, the coffee watcher process takes more file handles +than are allowed by default. This will result in `EMFILE` errors when coffeescript is running, and +will prevent javascript from compiling, leading to the error 'XModule is not defined' + +To work around this issue, we use `Process::setrlimit` to set the number of allowed open files. +Coffee watches both directories and files, so you will need to set this fairly high (anecdotally, +8000 seems to do the trick on OSX 10.7.5, 10.8.3, and 10.8.4) + + ## Running Tests See `testing.md` for instructions on running the test suite. diff --git a/rakelib/assets.rake b/rakelib/assets.rake index 0c58047bc2..10dfb14f18 100644 --- a/rakelib/assets.rake +++ b/rakelib/assets.rake @@ -6,6 +6,8 @@ if USE_CUSTOM_THEME THEME_SASS = File.join(THEME_ROOT, "static", "sass") end +MINIMAL_DARWIN_NOFILE_LIMIT = 8000 + def xmodule_cmd(watch=false, debug=false) xmodule_cmd = 'xmodule_assets common/static/xmodule' if watch @@ -21,24 +23,14 @@ def xmodule_cmd(watch=false, debug=false) end def coffee_cmd(watch=false, debug=false) - if watch - # On OSx, coffee fails with EMFILE when - # trying to watch all of our coffee files at the same - # time. - # - # Ref: https://github.com/joyent/node/issues/2479 - # - # So, instead, we use watchmedo, which works around the problem - "watchmedo shell-command " + - "--command 'node_modules/.bin/coffee -c ${watch_src_path}' " + - "--recursive " + - "--patterns '*.coffee' " + - "--ignore-directories " + - "--wait " + - "." - else - 'node_modules/.bin/coffee --compile .' + if watch && Launchy::Application.new.host_os_family.darwin? + available_files = Process::getrlimit(:NOFILE)[0] + if available_files < MINIMAL_DARWIN_NOFILE_LIMIT + Process.setrlimit(:NOFILE, MINIMAL_DARWIN_NOFILE_LIMIT) + + end end + "node_modules/.bin/coffee --compile #{watch ? '--watch' : ''} ." end def sass_cmd(watch=false, debug=false) From f4200127bcc91fc30eca2f727a21c6a71139fbfd Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Fri, 14 Jun 2013 21:46:27 -0400 Subject: [PATCH 5/8] Make coffee watch message more informative When running under watchmedo, coffee doesn't display any useful information when it recompiles a changed file, so we make watchmedo echo that information instead. --- rakelib/assets.rake | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rakelib/assets.rake b/rakelib/assets.rake index 0c58047bc2..c80b275c27 100644 --- a/rakelib/assets.rake +++ b/rakelib/assets.rake @@ -30,7 +30,7 @@ def coffee_cmd(watch=false, debug=false) # # So, instead, we use watchmedo, which works around the problem "watchmedo shell-command " + - "--command 'node_modules/.bin/coffee -c ${watch_src_path}' " + + "--command 'echo \">>> Change detected to ${watch_src_path}\" && node_modules/.bin/coffee -c ${watch_src_path}' " + "--recursive " + "--patterns '*.coffee' " + "--ignore-directories " + From eb3e94660bf166b75a34601549b7093507c46bf7 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Fri, 14 Jun 2013 21:47:22 -0400 Subject: [PATCH 6/8] Don't delete generated files from xmodule-assets xmodule-assets creates coffeescript files in the output directories. On its next run, it used to delete the javascript files compiled from those coffee files. Now it doesn't which should make coffee have to do less work. Fixes LMS-451 --- CHANGELOG.rst | 4 ++++ common/lib/xmodule/xmodule/static_content.py | 16 ++++++++++++---- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 9c7af1520b..fbc007949c 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -5,6 +5,10 @@ These are notable changes in edx-platform. This is a rolling list of changes, in roughly chronological order, most recent first. Add your entries at or near the top. Include a label indicating the component affected. +XModule: Don't delete generated xmodule asset files when compiling (for +instance, when XModule provides a coffeescript file, don't delete +the associated javascript) + Common: Make rake provide better error messages if packages are missing. Common: Repairs development documentation generation by sphinx. diff --git a/common/lib/xmodule/xmodule/static_content.py b/common/lib/xmodule/xmodule/static_content.py index 4c4827e0aa..42fef65b11 100755 --- a/common/lib/xmodule/xmodule/static_content.py +++ b/common/lib/xmodule/xmodule/static_content.py @@ -121,15 +121,23 @@ def _write_js(output_root, classes): type=filetype) contents[filename] = fragment - _write_files(output_root, contents) + _write_files(output_root, contents, {'.coffee': '.js'}) return [output_root / filename for filename in contents.keys()] -def _write_files(output_root, contents): +def _write_files(output_root, contents, generated_suffix_map=None): _ensure_dir(output_root) - for extra_file in set(output_root.files()) - set(contents.keys()): - extra_file.remove_p() + to_delete = set(file.basename() for file in output_root.files()) - set(contents.keys()) + + if generated_suffix_map: + for output_file in contents.keys(): + for suffix, generated_suffix in generated_suffix_map.items(): + if output_file.endswith(suffix): + to_delete.discard(output_file.replace(suffix, generated_suffix)) + + for extra_file in to_delete: + (output_root / extra_file).remove_p() for filename, file_content in contents.iteritems(): (output_root / filename).write_bytes(file_content) From cd57e281f5eab2d7616e47d4d9b95adf46efda1e Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Fri, 14 Jun 2013 22:27:24 -0400 Subject: [PATCH 7/8] Only write to xmodule asset files that have changed Use md5 checksumming to verify that we only write out xmodule asset files whose contents differ from what we are about to write. This minimizes thrashing of the other watchers. Fixes LMS-452 --- CHANGELOG.rst | 2 ++ common/lib/xmodule/xmodule/static_content.py | 12 +++++++++++- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index fbc007949c..d74816990b 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -5,6 +5,8 @@ These are notable changes in edx-platform. This is a rolling list of changes, in roughly chronological order, most recent first. Add your entries at or near the top. Include a label indicating the component affected. +XModule: Only write out assets files if the contents have changed. + XModule: Don't delete generated xmodule asset files when compiling (for instance, when XModule provides a coffeescript file, don't delete the associated javascript) diff --git a/common/lib/xmodule/xmodule/static_content.py b/common/lib/xmodule/xmodule/static_content.py index 42fef65b11..7662499c16 100755 --- a/common/lib/xmodule/xmodule/static_content.py +++ b/common/lib/xmodule/xmodule/static_content.py @@ -4,6 +4,7 @@ This module has utility functions for gathering up the static content that is defined by XModules and XModuleDescriptors (javascript and css) """ +import logging import hashlib import os import errno @@ -15,6 +16,9 @@ from path import path from xmodule.x_module import XModuleDescriptor +LOG = logging.getLogger(__name__) + + def write_module_styles(output_root): return _write_styles('.xmodule_display', output_root, _list_modules()) @@ -140,7 +144,13 @@ def _write_files(output_root, contents, generated_suffix_map=None): (output_root / extra_file).remove_p() for filename, file_content in contents.iteritems(): - (output_root / filename).write_bytes(file_content) + output_file = output_root / filename + + if not output_file.isfile() or output_file.read_md5() != hashlib.md5(file_content).digest(): + LOG.debug("Writing %s", output_file) + output_file.write_bytes(file_content) + else: + LOG.debug("%s unchanged, skipping", output_file) def main(): From 64912a774199e40a68cab3020d3e98ad9bb8c6e5 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Fri, 14 Jun 2013 21:17:08 -0400 Subject: [PATCH 8/8] Make assets watchers run as singletons Previously, multiple copies of the watchers started from the different shells would run simultaneously, which left the possiblity of zombie watchers, increased resource consumption, and incorrect results. This fixes that problem by only starting a watcher if that same command isn't already in the process list. Fixes LMS-499 --- CHANGELOG.rst | 3 +++ Gemfile | 1 + rakelib/assets.rake | 4 ++-- rakelib/helpers.rb | 12 ++++++++++++ 4 files changed, 18 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 9c7af1520b..18855e82ae 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -5,6 +5,9 @@ These are notable changes in edx-platform. This is a rolling list of changes, in roughly chronological order, most recent first. Add your entries at or near the top. Include a label indicating the component affected. +Common: Make asset watchers run as singletons (so they won't start if the +watcher is already running in another shell). + Common: Make rake provide better error messages if packages are missing. Common: Repairs development documentation generation by sphinx. diff --git a/Gemfile b/Gemfile index 7f7b146978..1ad685c34d 100644 --- a/Gemfile +++ b/Gemfile @@ -4,3 +4,4 @@ gem 'sass', '3.1.15' gem 'bourbon', '~> 1.3.6' gem 'colorize', '~> 0.5.8' gem 'launchy', '~> 2.1.2' +gem 'sys-proctable', '~> 0.9.3' diff --git a/rakelib/assets.rake b/rakelib/assets.rake index 0c58047bc2..c935b0d53b 100644 --- a/rakelib/assets.rake +++ b/rakelib/assets.rake @@ -114,9 +114,9 @@ namespace :assets do task :_watch => (prereq_tasks + ["assets:#{asset_type}:debug"]) do cmd = send(asset_type.to_s + "_cmd", watch=true, debug=true) if cmd.kind_of?(Array) - cmd.each {|c| background_process(c)} + cmd.each {|c| singleton_process(c)} else - background_process(cmd) + singleton_process(cmd) end end end diff --git a/rakelib/helpers.rb b/rakelib/helpers.rb index 4b10bef709..3373214a19 100644 --- a/rakelib/helpers.rb +++ b/rakelib/helpers.rb @@ -1,4 +1,6 @@ require 'digest/md5' +require 'sys/proctable' +require 'colorize' def find_executable(exec) path = %x(which #{exec}).strip @@ -84,6 +86,16 @@ def background_process(*command) end end +# Runs a command as a background process, as long as no other processes +# tagged with the same tag are running +def singleton_process(*command) + if Sys::ProcTable.ps.select {|proc| proc.cmdline.include?(command.join(' '))}.empty? + background_process(*command) + else + puts "Process '#{command.join(' ')} already running, skipping".blue + end +end + def environments(system) Dir["#{system}/envs/**/*.py"].select{|file| ! (/__init__.py$/ =~ file)}.map do |env_file| env_file.gsub("#{system}/envs/", '').gsub(/\.py/, '').gsub('/', '.')