mattn / mruby-onig-regexp

mrbgem of 鬼雲's Regular Expression
31 stars 36 forks source link

Always set compile option #57

Closed ksss closed 7 years ago

ksss commented 7 years ago

In most cases, It is good as it is now. But when we edit itself, It get a compile error.

So, It should run block always when src/mruby_onig_regexp.c needed compile.

mattn commented 7 years ago

I'm okay to merge. For my interesting, what error did it occur?

take-cheeze commented 7 years ago

I think with file method the block won't be executed when onigmo library is already built since the dependency would be already solved.

ksss commented 7 years ago

@mattn This is build error on macOS with clang.

master[f8b31a0d] ~/src/github.com/ksss/mruby
$ rake
PKG-CONFIG onigmo
PKG-CONFIG oniguruma
CC    ../mruby-onig-regexp/src/mruby_onig_regexp.c -> build/test/mrbgems/mruby-onig-regexp/src/mruby_onig_regexp.o
/Users/ksss/src/github.com/ksss/mruby-onig-regexp/src/mruby_onig_regexp.c:43:10: fatal error: 'oniguruma.h' file not found
#include "oniguruma.h"
         ^
1 error generated.
/Users/ksss/src/github.com/ksss/mruby-onig-regexp/src/mruby_onig_regexp.c:43:10: fatal error: 'oniguruma.h' file not found
#include "oniguruma.h"
         ^
1 error generated.
rake aborted!
Command failed with status (1): [gcc -g -std=gnu99 -O3 -Wall -Werror-implic...]
/Users/ksss/src/github.com/ksss/mruby/tasks/mruby_build_commands.rake:31:in `_run'
/Users/ksss/src/github.com/ksss/mruby/tasks/mruby_build_commands.rake:36:in `rescue in _run'
/Users/ksss/src/github.com/ksss/mruby/tasks/mruby_build_commands.rake:32:in `_run'
/Users/ksss/src/github.com/ksss/mruby/tasks/mruby_build_commands.rake:89:in `run'
/Users/ksss/src/github.com/ksss/mruby/tasks/mruby_build_commands.rake:113:in `block (2 levels) in define_rules'
Command failed with status (1): ["gcc" -g -std=gnu99 -O3 -Wall -Werror-impl...]
/Users/ksss/src/github.com/ksss/mruby/tasks/mruby_build_commands.rake:33:in `_run'
/Users/ksss/src/github.com/ksss/mruby/tasks/mruby_build_commands.rake:89:in `run'
/Users/ksss/src/github.com/ksss/mruby/tasks/mruby_build_commands.rake:113:in `block (2 levels) in define_rules'
Tasks: TOP => default => all => /Users/ksss/src/github.com/ksss/mruby/build/test/lib/libmruby.flags.mak => /Users/ksss/src/github.com/ksss/mruby/build/test/lib/libmruby.a => /Users/ksss/src/github.com/ksss/mruby/build/test/mrbgems/mruby-onig-regexp/src/mruby_onig_regexp.o
(See full trace by running task with --trace)

@take-cheeze Thank you for reviewing. Do you have any ideas? I am not good at build system.

ksss commented 7 years ago

Oh, This patch doesn't resolve issue. Please let me think.

ksss commented 7 years ago

My misunderstanding. It seems to be working fine.

mattn commented 7 years ago

file is defined as below in minirake.

    def task(args, &block)
      MiniRake::Task.define_task(args, &block)
    end

    # Declare a file task.
    def file(args, &block)
      MiniRake::FileTask.define_task(args, &block)
    end
  class FileTask < Task
    # Is this file task needed?  Yes if it doesn't exist, or if its time
    # stamp is out of date.
    def needed?
      return true unless File.exist?(name)
      prerequisites = @prerequisites.collect{ |n| n.is_a?(Proc) ? n.call(name) : n }.flatten
      latest_prereq = prerequisites.collect{|n| Task[n].timestamp}.max
      return false if latest_prereq.nil?
      timestamp < latest_prereq
    end

    # Time stamp for file task.
    def timestamp
      return Time.at(0) unless File.exist?(name)
      stat = File::stat(name.to_s)
      stat.directory? ? Time.at(0) : stat.mtime
    end
  end

I guess this works well for this case.

ksss commented 7 years ago

In this patch, rake works well. But minirake doesn't. This is why I misunderstanding.

This is minirake's issue, maybe.

Should we keep it working with current(unfixed) minirake for compatibility?

ksss commented 7 years ago

See also https://github.com/mruby/mruby/pull/3524

mattn commented 7 years ago

Now, this is closable?

ksss commented 7 years ago

No, it isn't. This patch will save 2 case with no side effects.

rake minirake before https://github.com/mruby/mruby/pull/3524 minirake after https://github.com/mruby/mruby/pull/3524
before merge - - -
after merge -

As yet another idea is that

diff --git a/mrbgem.rake b/mrbgem.rake
index 986a553..695db65 100644
--- a/mrbgem.rake
+++ b/mrbgem.rake
@@ -94,10 +94,11 @@ MRuby::Gem::Specification.new('mruby-onig-regexp') do |spec|

     file libmruby_a => Dir.glob("#{libonig_objs_dir}/*#{objext}") if File.exists? oniguruma_lib

-    file "#{dir}/src/mruby_onig_regexp.c" => oniguruma_lib do
+    task :mruby_onig_regexp_with_compile_option do
       cc.include_paths << oniguruma_dir
       cc.defines += ['HAVE_ONIGMO_H']
     end
+    file "#{dir}/src/mruby_onig_regexp.c" => [:mruby_onig_regexp_with_compile_option, oniguruma_lib]
   end

   if spec.respond_to? :search_package and spec.search_package 'onigmo'

This patch will save all cases. But one named task increases as side effect.

mattn commented 7 years ago

On my environment, I can't reproduce this. How to reproduce this?

mattn commented 7 years ago

I could reproduce this on Linux, But I couldn't repro on Windows. Okay, I want to merge your changes. Could you please rebase this patch?

mattn commented 7 years ago

merged https://github.com/mattn/mruby-onig-regexp/commit/fb8dd7e805092913ccb54ed87ddc8100b2b6a6ee

ksss commented 7 years ago

Thank you so much!