Skip to content

fix(build): increase packing performance #28

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.

Sign up for GitHub

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

Open
traa wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix(build): increase packing performance #28

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 22 additions & 7 deletions build.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -354,7 +354,6 @@ def update_files_based_on_timestamp(source_files, target_dir)

def update_sources()
puts "[INFO] Converting from gff to yml (this may take a while)..."

remove_deleted_files(GFF_CACHE_DIR, SOURCES.sub(/\.yml$/, ''))
system "rake", "--rakefile", EXTRACT_RAKE.to_s, "flat=#{FLAT_LAYOUT}", "SRC_DIR=#{SRC_DIR}", "GFF_CACHE_DIR=#{GFF_CACHE_DIR}", "SCRIPTS_DIR=#{SCRIPTS_DIR}", "ENCODING=#{ENCODING}"
update_files_based_on_timestamp(FileList[to_forward_slash GFF_CACHE_DIR.join("*.nss")], NSS_DIR)
Expand All @@ -364,11 +363,20 @@ def update_gffs()
puts "[INFO] Converting from yml to gff (this may take a while)..."

gffs = FileList[to_forward_slash GFF_CACHE_DIR.join("*")].exclude(/\.ncs$/)
srcs = FileList[to_forward_slash SRC_DIR.join("**", "*.*")].sub(/\.yml$/, '')

gffs.each do |gff|
FileUtils.rm(gff) unless srcs.detect{|src| File.basename(gff) == File.basename(src)}
# extension is a name of the subfolder
type = File.extname(gff).delete('.')
Copy link
Owner

@jakkn jakkn Mar 27, 2019

Choose a reason for hiding this comment

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

build.rb supports flat folder layout and the file matching here makes the below code blow up when running

nwn-build --flat extract
nwn-build pack-only

nwn-devbase/build.rb

Line 195 in 649c161

FLAT_LAYOUT = options[:flat] || (SOURCES.size > 0 && Pathname.glob(to_forward_slash SRC_DIR.join("*/")).size == 0) || false # Assume flat layout only on -f or if the source folder contains files but no directories

Array lookups, though slow, were safe in this regard. A quickfix could be an optional join on type, and to pass the value of FLAT_LAYOUT to pack.rake and apply the same logic there. A prettier fix might be to use a hash map, but that would require even more changes and I'm hesitant to rewrite large parts at this point due to the lack of test coverage.

# remove from gff cache if there is no associated file in src directory
# (taking into account subfolder with type and that nss files don't have an yml extension)
unless File.exist?(SRC_DIR.join(type, "#{File.basename(gff)}#{unless type == "nss" then ".yml" end }"))
puts "[INFO] %s removed from cache since it doesn't have a source" % File.basename(gff)
FileUtils.rm(gff)
end
end

system "rake", "--rakefile", PACK_RAKE.to_s, "SRC_DIR=#{SRC_DIR}", "GFF_CACHE_DIR=#{GFF_CACHE_DIR}", "ENCODING=#{ENCODING}"

return update_files_based_on_timestamp(FileList[to_forward_slash NSS_DIR.join("*.nss")], GFF_CACHE_DIR)
end

Expand Down Expand Up @@ -449,10 +457,17 @@ def extract_all()
def pack_all(skip_compile=false)
verify_executables
init_directories()
should_compile = update_gffs() && !skip_compile
puts "[INFO] No change in nss sources detected. Skipping compilation." unless should_compile && !skip_compile
puts "[INFO] Skipping compilation due to configuration" if skip_compile
compile_nss(MODULE_FILE) if should_compile

if skip_compile
puts "[INFO] Skipping compilation due to configuration"
end

should_compile = update_gffs()
if should_compile && !skip_compile
puts "[INFO] No change in nss sources detected. Skipping compilation."
end

compile_nss(MODULE_FILE) unless skip_compile
Copy link
Owner

@jakkn jakkn Mar 27, 2019

Choose a reason for hiding this comment

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

The logic above doesn't look right. This function was a mess already, and I think it needs to be redone in a more sensible way. The two flags make it complex, and there's something not right about conditionals that log an action without actually doing anything.

I would suggest replacing L461-468 with

  nss_changed = update_gffs()
  should_compile = nss_changed && !skip_compile

And this line to

Suggested change
compile_nss(MODULE_FILE) unless skip_compile
if should_compile
compile_nss(MODULE_FILE)
else
if skip_compile
puts "[INFO] Skipping compilation due to configuration"
else
puts "[INFO] No change in nss sources detected. Skipping compilation."
end
end

Or perhaps you see something more logical and/or compact.

update_cache(GFF_CACHE_DIR, TMP_CACHE_DIR)
pack_module(MODULE_FILE)

Expand Down
12 changes: 10 additions & 2 deletions pack.rake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ require 'nwn/all'
require 'fileutils'
require 'pathname'


def to_forward_slash(path=Pathname.getwd)
return path.to_s.gsub(File::ALT_SEPARATOR || File::SEPARATOR, File::SEPARATOR)
end
Expand All @@ -22,11 +23,18 @@ task :gff => [GFF_CACHE_DIR.to_s, :yml2gff]

multitask :yml2gff => GFF_TARGETS

rule( /\.(?!yml)[\w]+$/ => ->(f){ source_for_gff(f) }) do |t|
rule( /\.(?!yml)[\w]+$/ => ->(f){
source_for_gff(f)
}) do |t|
puts "[INFO] packing changed file: %s" % File.basename(t.name)
# -i IN Input file [default: -]
# -o OUT Output file [default: -]
# -k OUTFORMAT Output format [default: autodetect]
Copy link
Owner

@jakkn jakkn Mar 27, 2019

Choose a reason for hiding this comment

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

This reads nicely and is useful. Thumbs up.

system "nwn-gff", "-i", "#{t.source}", "-o", "#{t.name}", "-kg", "--encoding", "#{ENCODING}"
FileUtils.touch "#{t.name}", :mtime => File.mtime("#{t.source}")
end

def source_for_gff(gff)
YML_SOURCES.detect{|yml| File.basename(gff) == File.basename(yml, ".*")}
type = File.extname(gff).delete('.')
SRC_DIR.join(type, "#{File.basename(gff)}.yml")
end