Feature #13570: Using mkmf for ruby/spec C API specs

• Status: Closed
• Assignee: windows
• Target version: -

[Eregon (Benoit Daloze)]

Hello all,

I am thinking to use mkmf to compile the C API specs.

https://github.com/ruby/ruby/blob/trunk/spec/rubyspec/optional/capi/spec_helper.rb
is getting pretty complex and hard to maintain.

I have a few questions:

• Does mkmf works well on Windows?
• What is a good way to compile a single .c file with mkmf to a given library file in another directory?

I tried this but I am not sure it's correct:

def compile_extension(name)
  objdir = object_path
  ext = "#{name}_spec"
  lib = "#{objdir}/#{ext}.#{RbConfig::CONFIG['DLEXT']}"

  require 'mkmf' # TODO: probably best to use a subprocess to avoid polluting the namespace
  Dir.chdir(objdir) do
    $srcs = ["#{extension_path}/#{ext}.c"]
    $objs = ["#{extension_path}/#{ext}.o"] # should probably be in objdir but that does not seem to work
    create_makefile(ext)
    system "make"
  end

  lib
end

Alternatively, we can copy the needed files to a temporary directory, build there and copy the shared library back.
It's a bit more work but not a big deal either.

[MSP-Greg (Greg L)]

Does mkmf works well on Windows?

I'm not a c type. With MinGW builds, I've got 89 mkmf.log files in the build dir, and test-all results are consistent and reasonable low. I'm guessing that means yes.

FYI, I ran test-spec with -j on 58760, and it matched (or very closely matched) results without -j. test-all has the same results, but assertions and skips are different.

[Eregon (Benoit Daloze)]

• Status changed from Open to Closed

I made the change in https://github.com/ruby/spec/commit/84ea66ef61424ef87d03658cbc140d4b1af17c22
which was imported in r58939.

[Eregon (Benoit Daloze)]

• File spec_helper.rb spec_helper.rb added

I attach here the current version of the spec_helper.rb doing the compilation of extensions.
It seems to now work correctly for out-of-source builds,
and with an extra hack uses the right ruby in the Makefile
(RbConfig.ruby in ./rbconfig.rb is just incorrect for built but not installed ruby).

I found very confusing the effect that $extmf can have,
I wished MRI used a simpler mechanism to detect if ruby was installed or not.
Is it so important nowadays to be able to test without "make install"?

Could I ask some help for making this run correctly on Windows?
I would like if possible something not too hacky.
The old way to compile extensions was un-maintainable due to its complexity.

[Eregon (Benoit Daloze)]

• Status changed from Closed to Assigned
• Assignee set to windows

[MSP-Greg (Greg L)]

Benoit,

Sorry, never set this to 'watch'. I'll try a build with MinGW.

FWIW, I just got one of the spec c files to compile with only the following --

require 'mkmf'
Dir.chdir('src/build-x86_64/spec/rubyspec/optional/capi/temp') { |d|
  append_ldflags('-L /e/GitHub/ruby-loco/src/build-x86_64')
  create_makefile("bignum_spec")
  puts `make`
  require_relative 'src/build-x86_64/spec/rubyspec/optional/capi/temp/bignum_spec.so'
  puts CApiBignumSpecs.instance_methods(false)
}

Output:

E:\GitHub\ruby-loco>ruby make_test.rb
checking for whether -L /e/GitHub/ruby-loco/src/build-x86_64 is accepted as LDFLAGS... yes
creating Makefile
generating bignum_spec-x64-mingw32.def
compiling bignum_spec.c
linking shared-object bignum_spec.so

rb_big_pack_array
rb_big_pack_length
rb_big2dbl
rb_dbl2big
rb_big2ll
rb_big2long
rb_big2str
rb_big2ulong
rb_big_cmp
rb_big_pack

For the test, I only copied bignum_spec.c to the temp folder, along with rubyspec.h. I'm sure there's a setting to pick that up in mkmf.rb, along with several more that I'm probably missing. Again, I'm not a c type...

[MSP-Greg (Greg L)]

Benoit,

After screwing around with quite a bit of make code, I found the (simple) issue. The code in Object.cp does not properly copy binary files on Windows.

Hence, since we know the src and dest are on the same partition/drive, I used File.rename in spec_helper.rb. Below is a diff/patch file:

--- spec_helper_orig.rb	Mon May 29 13:34:22 2017
+++ spec_helper.rb	Wed May 31 15:47:17 2017
@@ -18,11 +18,7 @@
 
 def compile_extension(name)
   debug = false
-  run_mkmf_in_process = false
-
-  if RUBY_NAME == 'truffleruby'
-    run_mkmf_in_process = true
-  end
+  run_mkmf_in_process = RUBY_NAME == 'truffleruby'
 
   ext = "#{name}_spec"
   lib = "#{object_path}/#{ext}.#{RbConfig::CONFIG['DLEXT']}"
@@ -61,7 +57,7 @@
       raise "make failed:\n#{output}" unless $?.success?
       $stderr.puts output if debug
 
-      cp File.basename(lib), lib
+      File.rename File.basename(lib), lib
     end
   ensure
     rm_r tmpdir unless debug

Tests run with -j both via make test-spec and running mspec. Results of both are at Ruby MinGW Test Results.

Thanks for the work.

[hsbt (Hiroshi SHIBATA)]

• Related to Bug #13611: MinGW spec/rubyspec/optional/capi compile/link issues added

[Eregon (Benoit Daloze)]

MSP-Greg (Greg L) wrote:

Benoit,

After screwing around with quite a bit of make code, I found the (simple) issue. The code in Object.cp does not properly copy binary files on Windows.

Oh, wow, thanks and nice find!
MSpec's #cp should use "rb"/"wb" modes obviously.
Would it work with that?

Hence, since we know the src and dest are on the same partition/drive, I used File.rename in spec_helper.rb. Below is a diff/patch file:

I am not sure that's always the case, but thanks for the patch, I will try it.

Tests run with -j both via make test-spec and running mspec. Results of both are at Ruby MinGW Test Results.

Great!

[MSP-Greg (Greg L)]

I checked on MinGW, and there is not an issue with File.rename with src and dest being on different drives. Where I got that from, I don't know...

Hence, on MinGW/Windows, rename will work in all situations.

MSpec's #cp should use "rb"/"wb" modes obviously.
Would it work with that?

Yes and haven't tried. Seems odd to copy a file when a rename will do...

[Eregon (Benoit Daloze)]

MSP-Greg (Greg L) wrote:

I checked on MinGW, and there is not an issue with File.rename with src and dest being on different drives. Where I got that from, I don't know...

It's the semantics of the rename(2) syscall on Linux, which throws EXDEV if oldpath and newpath are not on the same mounted filesystem.

And indeed, on Linux:

> File.write "foo","abc"
> File.rename "foo", "/tmp/bla"
Errno::EXDEV: Invalid cross-device link @ rb_file_s_rename - (foo, /tmp/bla)

So we would need some special handling to move files across filesystems on non-Windows, or use FileUtils.mv

MSpec's #cp should use "rb"/"wb" modes obviously.
Would it work with that?

Yes and haven't tried. Seems odd to copy a file when a rename will do...

Indeed, moving sounds nicer in principle.
The reason I like the copy is to help debugging, the .so is still in tmpdir used for compilation along other files.
But it's a small thing, looking for the moved file is fine as well.

[nobu (Nobuyoshi Nakada)]

Eregon (Benoit Daloze) wrote:

MSpec's #cp should use "rb"/"wb" modes obviously.

Why not IO.copy_stream?

[Eregon (Benoit Daloze)]

Sounds good, I did not know IO.copy_stream can be used with 2 path Strings as well. It's a bit of a strange name for "cp".
But I think I slightly prefer the current implementation, it uses only well-known basic functionality such as File.open+read/write.
I would guess early Ruby implementations don't implement IO.copy_stream.

[Eregon (Benoit Daloze)]

• File spec_helper.rb spec_helper.rb added

Here is a version of the spec_helper.rb working on MRI 2.2 to trunk on Linux (and I would expect macOS as well).

@greg (Grégory Duchatelet) and @nobu (Nobuyoshi Nakada):
Could you test it on Windows?

Then should I commit it?
Or do we have a way to test CI (Travis + rubyci.org) before committing?

[MSP-Greg (Greg L)]

I'll try a MinGW build shortly.

Re IO.copy_stream, I believe it has existed since Ruby 1.9.3, which should be far enough back for current test code. Also, I've only paid attention to the files copied for capi, but all are small enough that I'd prefer one function call to the call in fs.rb that creates two blocks in ruby, then loops (in ruby) 300+ times for the copy.

I patched both fs.rb and spec_helper.rb with IO.copy_stream, and both ran successfully.

I'll start the build.

[MSP-Greg (Greg L)]

Ran a MinGW build on ruby 2.5.0dev 2017-06-02 58998. Ran tests three ways:

1. make test-spec - pkg/install bin folder in path
2. make test-spec - pkg/install bin folder not in path
3. mspec - pkg/install bin folder in path

All three ran with expected results. Although I'd prefer that IO.copy_stream be used, neither fs.rb or spec_helper.rb were patched to use it.

Hence, good job and thanks...

[MSP-Greg (Greg L)]

Eregon (Benoit Daloze) wrote:

Or do we have a way to test CI (Travis + rubyci.org) before committing?

As you know, Travis will run on a PR.

A few people seem to state that trunk builds and tests fine on mswin, but Appveyor doesn't run test-all or test-spec/test-rubyspec. I'd be interested to know why...

[usa (Usaku NAKAMURA)]

MSP-Greg (Greg L) wrote:

A few people seem to state that trunk builds and tests fine on mswin, but Appveyor doesn't run test-all or test-spec/test-rubyspec. I'd be interested to know why...

Our hero nobu is testing running test-all and test-spec on appveyor.
I've heard that there was a little problem, but I believe that he will merge it into trunk in near future.

[nobu (Nobuyoshi Nakada)]

test-spec runs fine on AppVeyor, test/ruby/test_rubyoptions.rb has a failure when locale and filesystem encodings differ.
Another problem is that it consumes 15 min, vs 8 min without tests.

[MSP-Greg (Greg L)]

nobu (Nobuyoshi Nakada) wrote:

test-spec runs fine on AppVeyor, test/ruby/test_rubyoptions.rb has a failure when locale and filesystem encodings differ.
Another problem is that it consumes 15 min, vs 8 min without tests.

Thank you for working on that (in addition to everything else you do).

MinGW has the same failure, as discussed previously. In my test results log, failures jumped from 5 to 6. That's when I decided to remove the chcp and RUBYOPT changes from my build code.

Re my MinGW test-all, I guess my goals were/are:

1. It completes
2. Consistent failures/errors
3. Minimize skips
4. Consistent tests/assertions
5. WHY DOES THIS TAKE SO LONG / parallel test

I haven't gotten to 5...

[Eregon (Benoit Daloze)]

I tested and adapted the spec_helper.rb so it now also works on mswin (https://github.com/ruby/ruby/pull/1649) and mingw (on the Travis of ruby/spec).
Therefore I close this with r59093.
Please fix if new failures appear when building with mkmf.

[Eregon (Benoit Daloze)]

• Status changed from Assigned to Closed

[Eregon (Benoit Daloze)]

@nobu (Nobuyoshi Nakada) cherry-picked the IO.copy_stream patch in r66147, and I now upstreamed it to ruby/mspec.
I mostly forgot about it, and it's easy enough for early Ruby implementations to shim IO.copy_stream.