From 581baf6e3775ccb9c3b59385aaf6b9350f321e24 Mon Sep 17 00:00:00 2001 From: Ahmad Amireh Date: Wed, 16 Sep 2020 16:01:56 -0600 Subject: [PATCH] highlight dependencies in canvas:compile_assets refs FOO-966 as we're working on extracting Gradebook into its own client_app, the canvas:compile_assets rake task had to be adjusted to build it too, then i realized that the canvas_quizzes app has to be built _before_ we generate i18n phrases while Gradebook (and canvas) have to go _after_ that task the rake task did not appreciate any more complexity, so instead I tried to focus on making the dependencies between these sub-tasks clearer and at the same time maintain the parallel execution changes: - it's now possible to build a specific client_app via rake tasks - the symlink needed for canvas_quizzes client app is now restored correctly in case of a failure during the i18n rake task. Previously, you had to restore it by hand before running any of the tasks again - in case a client_app does not implement a `script/build`, we call its "npm build" script instead (eg `yarn run build`) test plan: - run "RAILS_LOAD_ALL_LOCALES=1 rake canvas:compile_assets" before you check out the patch and note the time taken - check out the patch and re-run the task, verify it still works and that the time taken is not longer than what it was (if anything, it should be a little faster since we're batching more work) Change-Id: I65e675ceeea93643a1c679ff29039ed5ef71b53d Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/247902 Reviewed-by: Simon Williams QA-Review: Simon Williams Product-Review: Simon Williams Tested-by: Service Cloud Jenkins --- gems/canvas_i18nliner/js/main.js | 2 +- lib/rake/task_graph.rb | 148 +++++++++++++++++++++++++++++++ lib/tasks/canvas.rake | 127 +++++++++++++------------- lib/tasks/js.rake | 47 +++++----- spec/lib/rake/task_graph_spec.rb | 98 ++++++++++++++++++++ 5 files changed, 338 insertions(+), 84 deletions(-) create mode 100644 lib/rake/task_graph.rb create mode 100644 spec/lib/rake/task_graph_spec.rb diff --git a/gems/canvas_i18nliner/js/main.js b/gems/canvas_i18nliner/js/main.js index 36bb2b257f4..7e20c1cf594 100644 --- a/gems/canvas_i18nliner/js/main.js +++ b/gems/canvas_i18nliner/js/main.js @@ -98,7 +98,7 @@ module.exports = { // the unlink/symlink uglieness is a temporary hack to get around our circular // symlinks. we should just remove the symlinks fs.unlinkSync('./public/javascripts/symlink_to_node_modules') - Commands.run(argv._[0], argv) || process.exit(1); + Commands.run(argv._[0], argv) || (process.exitCode = 1); fs.symlinkSync('../../node_modules', './public/javascripts/symlink_to_node_modules') } diff --git a/lib/rake/task_graph.rb b/lib/rake/task_graph.rb new file mode 100644 index 00000000000..5f927868e4b --- /dev/null +++ b/lib/rake/task_graph.rb @@ -0,0 +1,148 @@ +# +# Copyright (C) 2020 - present Instructure, Inc. +# +# This file is part of Canvas. +# +# Canvas is free software: you can redistribute it and/or modify it under +# the terms of the GNU Affero General Public License as published by the Free +# Software Foundation, version 3 of the License. +# +# Canvas is distributed in the hope that it will be useful, but WITHOUT ANY +# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR +# A PARTICULAR PURPOSE. See the GNU Affero General Public License for more +# details. +# +# You should have received a copy of the GNU Affero General Public License along +# with this program. If not, see . + +module Rake + # An API for running a large number of Rake tasks that may have dependencies + # between them. The graph resolves the dependencies, orders the tasks and + # partitions them in arrays, allowing you to run each batch in parallel + # or all tasks serially. + # + # The API tries to stay as close to Rake's as possible. + # + # == Usage + # + # batches = Rake::TaskGraph.draw do + # task 'a' => [] + # task 'b' => ['a'] + # task 'c' => [] + # task 'd' => ['c','b'] + # end + # # => [ ['a','c'], ['b'], ['d'] ] + # + # # run all tasks in a batch in parallel: + # batches.each do |tasks| + # Parallel.each(tasks) { |name| Rake::Task[name].invoke } + # end + # + # # or, run all tasks serially and in the right order: + # batches.flatten.each do |task| + # Rake::Task[task].invoke + # end + # + # == Options + # + # You can transform a "node", which is a string by default, into a different + # value by passing a block that the graph will yield to when it's time to + # insert the node into a batch: + # + # TaskGraph.draw do + # task 'a' do + # 5 + # end + # end.to_a + # # => [ 5 ] + # + class TaskGraph + IDENTITY = ->(x) { x } + + attr_reader :nodes, :transformers + + def self.draw(&builder) + new.tap { |x| x.instance_exec(&builder) }.batches + end + + def initialize + @nodes = {} + @transformers = {} + end + + def task(name_and_deps, &transformer) + name, deps = if name_and_deps.is_a?(Hash) + name_and_deps.first + else + [name_and_deps, []] + end + + @nodes[name] = deps + @transformers[name] = transformer || IDENTITY + end + + def batches + ensure_all_nodes_are_defined! + + to_take = nodes.keys + + [].tap do |batches| + to_take.size.times do # cap iterations just in case + batch = to_take.reduce([]) do |acc, node| + take_or_resolve(node, acc, to_take) + end + + if batch.empty? + break # we're done + else + to_take -= batch + batches << batch.map { |x| @transformers[x][x] } + end + end + end + end + + private + + def ensure_all_nodes_are_defined! + undefined = nodes.reduce([]) do |errors, (_node, deps)| + errors + deps.select { |dep| !nodes.key?(dep) } + end + + if undefined.any? + fail <<~ERR + + The following nodes are listed as dependents but were not defined: + + - #{undefined.uniq.join("\n - ")} + + ERR + end + end + + def take_or_resolve(node, batch, to_take, visited = []) + if visited.include?(node) + fail "node \"#{node}\" has a self or circular dependency" + end + + # don't dupe if we already took it this pass (e.g. as a dep): + if batch.include?(node) + return batch + end + + unresolved_deps = to_take & nodes[node] + + # assign to this batch if all deps are satisfied: + if unresolved_deps.empty? + return batch.push(node) + end + + # try to resolve as many of the deps as possible in this pass and retry + # ourselves in the next: + unresolved_deps.reduce(batch) do |acc, dep| + # take_or_resolve(dep, acc, to_take, visited.push(node)) + take_or_resolve(dep, acc, to_take, visited + [node]) + end + end + end +end diff --git a/lib/tasks/canvas.rake b/lib/tasks/canvas.rake index c15c8a1774d..a75b7ea4682 100644 --- a/lib/tasks/canvas.rake +++ b/lib/tasks/canvas.rake @@ -1,3 +1,6 @@ +require 'rake/task_graph' +require 'parallel' + $canvas_tasks_loaded ||= false unless $canvas_tasks_loaded $canvas_tasks_loaded = true @@ -5,7 +8,7 @@ $canvas_tasks_loaded = true def log_time(name, &block) puts "--> Starting: '#{name}'" time = Benchmark.realtime(&block) - puts "--> Finished: '#{name}' in #{time}" + puts "--> Finished: '#{name}' in #{time.round(2)}s" time end @@ -18,80 +21,78 @@ end namespace :canvas do desc "Compile javascript and css assets." task :compile_assets do |t, args| - # opt out npm_install = ENV["COMPILE_ASSETS_NPM_INSTALL"] != "0" - compile_css = ENV["COMPILE_ASSETS_CSS"] != "0" - build_styleguide = ENV["COMPILE_ASSETS_STYLEGUIDE"] != "0" - build_webpack = ENV["COMPILE_ASSETS_BUILD_JS"] != "0" build_api_docs = ENV["COMPILE_ASSETS_API_DOCS"] != "0" + build_css = ENV["COMPILE_ASSETS_CSS"] != "0" + build_styleguide = ENV["COMPILE_ASSETS_STYLEGUIDE"] != "0" + build_js = ENV["COMPILE_ASSETS_BUILD_JS"] != "0" + build_prod_js = ENV['RAILS_ENV'] == 'production' || ENV['USE_OPTIMIZED_JS'] == 'true' || ENV['USE_OPTIMIZED_JS'] == 'True' + # build dev bundles even in prod mode so you can debug with ?optimized_js=0 + # query string (except for on jenkins where we set JS_BUILD_NO_UGLIFY anyway + # so there's no need for an unminified fallback) + build_dev_js = !build_prod_js || ENV['JS_BUILD_NO_UGLIFY'] == "1" - if npm_install - log_time('Making sure node_modules are up to date') { - Rake::Task['js:yarn_install'].invoke - } - end - - raise "Error running gulp rev" unless system('yarn run gulp rev') - - if compile_css - # public/dist/brandable_css/brandable_css_bundles_with_deps.json needs - # to exist before we run handlebars stuff, so we have to do this first - Rake::Task['css:compile'].invoke - end - - require 'parallel' - tasks = Hash.new - - if build_styleguide - tasks["css:styleguide"] = -> { - Rake::Task['css:styleguide'].invoke - } - end - - Rake::Task['js:build_client_apps'].invoke - - generate_tasks = [] - generate_tasks << 'i18n:generate_js' if build_webpack - build_tasks = [] - if build_webpack - # build dev bundles even in prod mode so you can debug with ?optimized_js=0 query string - # (except for on jenkins where we set JS_BUILD_NO_UGLIFY anyway so there's no need for an unminified fallback) - build_prod = ENV['RAILS_ENV'] == 'production' || ENV['USE_OPTIMIZED_JS'] == 'true' || ENV['USE_OPTIMIZED_JS'] == 'True' - dont_need_dev_fallback = build_prod && ENV['JS_BUILD_NO_UGLIFY'] == "1" - build_tasks << 'js:webpack_development' unless dont_need_dev_fallback - build_tasks << 'js:webpack_production' if build_prod - end - - msg = "run " + (generate_tasks + build_tasks).join(", ") - tasks[msg] = -> { - if generate_tasks.any? - Parallel.each(generate_tasks, in_processes: parallel_processes) do |name| - log_time(name) { Rake::Task[name].invoke } - end + batches = Rake::TaskGraph.draw do + task 'css:compile' if build_css + task 'css:styleguide' if build_styleguide + task 'doc:api' if build_api_docs + task 'js:yarn_install' if npm_install + task 'gulp:rev' do |name| + { name: name, runner: -> { system('yarn run gulp rev') } } end - if build_tasks.any? - Parallel.each(build_tasks, in_threads: parallel_processes) do |name| - log_time(name) { Rake::Task[name].invoke } - end - end - } + task 'i18n:generate_js' => [ + # canvas_quizzes is quirky in that we can only extract its phrases from + # its build artifacts and not from its source! this is unlike other + # client apps + 'js:build_client_app[canvas_quizzes]' + ] if build_js - if build_api_docs - tasks["Generate documentation [yardoc]"] = -> { - Rake::Task['doc:api'].invoke - } + task 'js:build_client_app[canvas_quizzes]' => [ 'css:compile' ] do |name| + { + name: name, + runner: -> { Rake::Task['js:build_client_app'].invoke('canvas_quizzes') } + } + end if build_js + + task 'js:webpack_development' => [ + # public/dist/brandable_css/brandable_css_bundles_with_deps.json needs + # to exist before we run handlebars stuff, so we have to do this first + 'css:compile', + 'i18n:generate_js', + 'js:build_client_app[canvas_quizzes]', + ] if build_js && build_dev_js + + task 'js:webpack_production' => [ + 'css:compile', + 'i18n:generate_js', + 'js:build_client_app[canvas_quizzes]', + ] if build_js && build_prod_js end - times = nil + batch_times = [] real_time = Benchmark.realtime do - times = Parallel.map(tasks, :in_processes => parallel_processes) do |name, lamduh| - log_time(name) { lamduh.call } + batches.each do |tasks| + batch_times += Parallel.map(tasks, :in_processes => parallel_processes) do |task| + name, runner = if task.is_a?(Hash) + task.values_at(:name, :runner) + else + [task, ->() { Rake::Task[task].invoke }] + end + + log_time(name, &runner) + end end end - combined_time = times.reduce(:+) - puts "Finished compiling assets in #{real_time}. parallelism saved #{combined_time - real_time} (#{real_time.to_f / combined_time.to_f * 100.0}%)" + + combined_time = batch_times.reduce(:+) + + puts ( + "Finished compiling assets in #{real_time.round(2)}s. " + + "Parallelism saved #{(combined_time - real_time).round(2)}s " + + "(#{(real_time.to_f / combined_time.to_f * 100.0).round(2)}%)" + ) end desc "Just compile css and js for development" diff --git a/lib/tasks/js.rake b/lib/tasks/js.rake index 485182d6893..73430101637 100644 --- a/lib/tasks/js.rake +++ b/lib/tasks/js.rake @@ -4,38 +4,45 @@ namespace :js do desc "Build client_apps" task :build_client_apps do + Dir.glob("./client_apps/*/").each do |app_dir| + Rake::Task['js:build_client_app'].invoke(File.basename(app_dir)) + end + end + + desc "Build a specific client_app" + task :build_client_app, [:app_name] do |t, app_name:| require 'config/initializers/client_app_symlinks' npm_install = ENV["COMPILE_ASSETS_NPM_INSTALL"] != "0" - Dir.glob('./client_apps/*/').each do |app_dir| - app_name = File.basename(app_dir) + Dir.chdir("./client_apps/#{app_name}") do + puts "Building client app '#{app_name}'" - Dir.chdir(app_dir) do - puts "Building client app '#{app_name}'" - - if npm_install && File.exists?('./package.json') - output = system 'yarn install --pure-lockfile || yarn install --pure-lockfile --network-concurrency 1' - unless $?.exitstatus == 0 - puts "INSTALL FAILURE:\n#{output}" - raise "Package installation failure for client app #{app_name}" - end - end - - puts "\tRunning 'yarn run build'..." - output = `./script/build` + if npm_install && File.exists?('./package.json') + output = system 'yarn install --pure-lockfile || yarn install --pure-lockfile --network-concurrency 1' unless $?.exitstatus == 0 - puts "BUILD FAILURE:\n#{output}" - raise "Build script failed for client app #{app_name}" + puts "INSTALL FAILURE:\n#{output}" + raise "Package installation failure for client app #{app_name}" end - - puts "Client app '#{app_name}' was built successfully." end + + puts "\tRunning 'yarn run build'..." + output = if File.exists?('./script/build') + `./script/build` + else + `yarn run build` + end + + unless $?.exitstatus == 0 + puts "BUILD FAILURE:\n#{output}" + raise "Build script failed for client app #{app_name}" + end + + puts "Client app '#{app_name}' was built successfully." end maintain_client_app_symlinks end - desc "Build development webpack js" task :webpack_development do require 'config/initializers/plugin_symlinks' diff --git a/spec/lib/rake/task_graph_spec.rb b/spec/lib/rake/task_graph_spec.rb new file mode 100644 index 00000000000..b2d3c85eaf0 --- /dev/null +++ b/spec/lib/rake/task_graph_spec.rb @@ -0,0 +1,98 @@ +require 'spec_helper' +require 'rake/task_graph' + +describe Rake::TaskGraph do + it 'works' do + batches = described_class.draw { task 'a' } + expect(batches.length).to eq(1) + expect(batches[0]).to eq(['a']) + end + + it 'resolves deps' do + batches = described_class.draw do + task 'a' + task 'b' => ['a'] + task 'c' + task 'd' => ['c','b'] + end + + expect(batches).to eq([ + ['a','c'], + ['b'], + ['d'] + ]) + end + + it 'does not dupe nodes' do + batches = described_class.draw do + task 'a' => [] + task 'b' => ['a','a'] + task 'c' + task 'd' => ['c','b'] + task 'e' => ['a'] + end + + expect(batches).to eq([ + ['a','c'], + ['b','e'], + ['d'] + ]) + end + + it 'is pure' do + subject.task 'a' + subject.task 'b' => ['a'] + subject.task 'c' => [] + subject.task 'd' => ['a','b','c'] + + expect(subject.batches).to eq(subject.batches) + end + + it 'loses no nodes in a sequence' do + batches = described_class.draw do + task 'a' + task 'b' => ['a'] + task 'c' => ['b'] + task 'd' => ['c'] + end + + expect(batches).to eq([ + ['a'], + ['b'], + ['c'], + ['d'], + ]) + end + + it 'transforms a node' do + batches = described_class.draw do + task 'b' => ['a'] + task 'a' do + 5 + end + end + + expect(batches).to eq([ [5], ['b'] ]) + end + + it 'whines on self-deps' do + expect { + described_class.draw { task 'a' => ['a'] } + }.to raise_error(/has a self or circular dependency/) + end + + it 'whines on circular deps' do + expect { + described_class.draw do + task 'a' => ['b'] + task 'b' => ['a'] + end + }.to raise_error(/has a self or circular dependency/) + end + + it 'whines if a dependency is undefined' do + expect { + described_class.draw { task 'a' => ['b'] } + }.to raise_error(/but were not defined/) + end +end