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.

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

Swig 3.0.8 breaks SVN Ruby bindings #602

Closed
voxik opened this Issue Feb 2, 2016 · 6 comments

Comments

Projects
None yet
3 participants
@voxik

voxik commented Feb 2, 2016

It seems that Swigh 3.0.8 breaks SVN Ruby bindings. I have tried to compile Subversion against Ruby 2.2.4 and 2.3.0 and in both case, the Subversions test suite breaks:

if [ "LD_LIBRARY_PATH" = "DYLD_LIBRARY_PATH" ]; then for d in /builddir/build/BUILD/subversion-1.9.3/subversion/bindings/swig/ruby/libsvn_swig_ruby /builddir/build/BUILD/subversion-1.9.3/subversion/bindings/swig/ruby/../../../libsvn_*; do if [ -n "$DYLD_LIBRARY_PATH" ]; then LD_LIBRARY_PATH="$LD_LIBRARY_PATH:$d/.libs"; else LD_LIBRARY_PATH="$d/.libs"; fi; done; export LD_LIBRARY_PATH; fi; \
cd /builddir/build/BUILD/subversion-1.9.3/subversion/bindings/swig/ruby; \
          if [ "2" -eq 1 -a "2" -lt 9 ] ; then \
            /usr/bin/ruby -I /builddir/build/BUILD/subversion-1.9.3/subversion/bindings/swig/ruby \
              /builddir/build/BUILD/subversion-1.9.3/subversion/bindings/swig/ruby/test/run-test.rb \
      --verbose=verbose; \
          else \
    /usr/bin/ruby -I /builddir/build/BUILD/subversion-1.9.3/subversion/bindings/swig/ruby \
      /builddir/build/BUILD/subversion-1.9.3/subversion/bindings/swig/ruby/test/run-test.rb; \
          fi
Loaded suite test
Started
...............................................................................
...............................................F
===============================================================================
Failure:
test_create(SvnFsTest)
/builddir/build/BUILD/subversion-1.9.3/subversion/bindings/swig/ruby/test/test_fs.rb:65:in `assert_create'
/builddir/build/BUILD/subversion-1.9.3/subversion/bindings/swig/ruby/test/test_fs.rb:74:in `test_create'
     71:   end
     72: 
     73:   def test_create
  => 74:     assert_create do |method, args, callback|
     75:       Svn::Fs.__send__(method, *args, &callback)
     76:     end
     77:   end

<Svn::Error::FsAlreadyClose> expected but was
<ObjectPreviouslyDeleted(<Expected argument 0 of type svn_fs_t *, but got SWIG::TYPE_p_svn_fs_t #<SWIG::TYPE_p_svn_fs_t:0x0055...
    in SWIG method 'svn_fs_path'>)>

diff:
+ ObjectPreviouslyDeleted(<Expected argument 0 of type svn_fs_t *, but got SWIG::TYPE_p_svn_fs_t #<SWIG::TYPE_p_svn_fs_t:0x0055...
?     S             vn::Error::FsAlreadyClose
?   in  WIG method 's  _f         _p   th'>)  
===============================================================================
F
===============================================================================
Failure:
test_create_for_backward_compatibility(SvnFsTest)
/builddir/build/BUILD/subversion-1.9.3/subversion/bindings/swig/ruby/test/test_fs.rb:65:in `assert_create'
/builddir/build/BUILD/subversion-1.9.3/subversion/bindings/swig/ruby/test/test_fs.rb:80:in `test_create_for_backward_compatibility'
     77:   end
     78: 
     79:   def test_create_for_backward_compatibility
  => 80:     assert_create do |method, args, callback|
     81:       Svn::Fs::FileSystem.__send__(method, *args, &callback)
     82:     end
     83:   end

<Svn::Error::FsAlreadyClose> expected but was
<ObjectPreviouslyDeleted(<Expected argument 0 of type svn_fs_t *, but got SWIG::TYPE_p_svn_fs_t #<SWIG::TYPE_p_svn_fs_t:0x0055...
    in SWIG method 'svn_fs_path'>)>

diff:
+ ObjectPreviouslyDeleted(<Expected argument 0 of type svn_fs_t *, but got SWIG::TYPE_p_svn_fs_t #<SWIG::TYPE_p_svn_fs_t:0x0055...
?     S             vn::Error::FsAlreadyClose
?   in  WIG method 's  _f         _p   th'>)  
===============================================================================
...................................F
===============================================================================
Failure:
test_create(SvnReposTest)
/builddir/build/BUILD/subversion-1.9.3/subversion/bindings/swig/ruby/test/test_repos.rb:110:in `test_create'
     107:     end
     108: 
     109:     assert(repos.closed?)
  => 110:     assert_raises(Svn::Error::ReposAlreadyClose) do
     111:       repos.fs
     112:     end
     113: 

<Svn::Error::ReposAlreadyClose> expected but was
<ObjectPreviouslyDeleted(<Expected argument 0 of type svn_repos_t *, but got SWIG::TYPE_p_svn_repos_t #<SWIG::TYPE_p_svn_repos_t:0x0...
    in SWIG method 'svn_repos_fs_wrapper'>)>

diff:
+ ObjectPreviouslyDeleted(<Expected argument 0 of type svn_repos_t *, but got SWIG::TYPE_p_svn_repos_t #<SWIG::TYPE_p_svn_repos_t:0x0...
?     S             vn::Error::ReposAl   r   eadyClose
?   in  WIG method 's  _             _fs_w app r'>)    
===============================================================================
..........................................................

Finished in 152.675316325 seconds.

222 tests, 5546 assertions, 3 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
98.6486% passed

Build against Swig 3.0.7 works just fine. So I tried revert to Lib/ruby/rubyrun.swg to this version [1] and it worked just fine again. I suspect that 146252f is the cause of the issues.

Just FTR, the particular bits of the test suite can be executed as follows:

$ cd subversion/bindings/swig/ruby/
$ ruby -I.libs/ test/run-test.rb -t /SvnFsTest/ -n /^test_create$/

[1] https://github.com/swig/swig/blob/3d1e20248f8422681382be8a91c91d456d4595e7/Lib/ruby/rubyrun.swg

@wsfulton

This comment has been minimized.

Show comment
Hide comment
@wsfulton

wsfulton Feb 3, 2016

Member

Any chance you can provide a cut down test or is it a no brainer to build svn?

Member

wsfulton commented Feb 3, 2016

Any chance you can provide a cut down test or is it a no brainer to build svn?

@wsfulton

This comment has been minimized.

Show comment
Hide comment
@wsfulton

wsfulton May 16, 2016

Member

I am looking at this now and am building svn-1.9.3 now. I see:

checking swig version... 3.0.8
configure: WARNING: Detected SWIG version 3.0.8
configure: WARNING: Subversion requires SWIG >= 1.3.24 and < 3.0.0 

Is there something more fundamentally wrong with using swig-3.0.0+ for svn wrappers?

Member

wsfulton commented May 16, 2016

I am looking at this now and am building svn-1.9.3 now. I see:

checking swig version... 3.0.8
configure: WARNING: Detected SWIG version 3.0.8
configure: WARNING: Subversion requires SWIG >= 1.3.24 and < 3.0.0 

Is there something more fundamentally wrong with using swig-3.0.0+ for svn wrappers?

@notroj

This comment has been minimized.

Show comment
Hide comment
@notroj

notroj May 17, 2016

@wsfulton - nothing really fundamental, but they kept hitting issues with 3.0.x (like #379 and #377), so they disabled SWIG 3.0 support until somebody could work that out. It seemed to be working for 3.0.6 and 3.0.7.

notroj commented May 17, 2016

@wsfulton - nothing really fundamental, but they kept hitting issues with 3.0.x (like #379 and #377), so they disabled SWIG 3.0 support until somebody could work that out. It seemed to be working for 3.0.6 and 3.0.7.

@wsfulton

This comment has been minimized.

Show comment
Hide comment
@wsfulton

wsfulton May 17, 2016

Member

I'm a bit frustrated that I can't get the test suite to run. Calls to Test::Unit::AutoRunner.run in run-test-rb fail without any error message and I don't have the time to debug this any further. I've been using Ubuntu 16.04 with ruby-2.3. Maybe this OS too bleeding edge. I've got plenty of versions of Ubuntu lying around, is the test-suite known to work with one of them?

Member

wsfulton commented May 17, 2016

I'm a bit frustrated that I can't get the test suite to run. Calls to Test::Unit::AutoRunner.run in run-test-rb fail without any error message and I don't have the time to debug this any further. I've been using Ubuntu 16.04 with ruby-2.3. Maybe this OS too bleeding edge. I've got plenty of versions of Ubuntu lying around, is the test-suite known to work with one of them?

@wsfulton wsfulton closed this in 763827c May 24, 2016

@wsfulton

This comment has been minimized.

Show comment
Hide comment
@wsfulton

wsfulton May 24, 2016

Member

The subversion-1.9.3 Ruby test-suite works out the box on Ubuntu-14.04 and recreates this reported bug with ruby-1.9.3p484.

The problem is peculiar to Subversion's pattern of using manually written proxy classes to wrap an opaque pointer and deleting them using 'DATA_PTR(self) = 0' in the manually written svn_fs_close function:

static VALUE
svn_fs_swig_rb_close(VALUE self)
{
  if (!DATA_PTR(self)) {
    svn_swig_rb_raise_svn_fs_already_close();
  }

  svn_swig_rb_destroy_internal_pool(self);
  DATA_PTR(self) = NULL;

  return Qnil;
}

146252f is indeed where the regression was introduced and this needs tweaking slightly to revert to the old behaviour and not throw SWIG_ObjectPreviouslyDeletedError. It isn't clear to me which approach is better in this corner case, so I've gone with the old behaviour for backwards compatibility.

The subversion ruby tests now pass with the change I've committed.

Member

wsfulton commented May 24, 2016

The subversion-1.9.3 Ruby test-suite works out the box on Ubuntu-14.04 and recreates this reported bug with ruby-1.9.3p484.

The problem is peculiar to Subversion's pattern of using manually written proxy classes to wrap an opaque pointer and deleting them using 'DATA_PTR(self) = 0' in the manually written svn_fs_close function:

static VALUE
svn_fs_swig_rb_close(VALUE self)
{
  if (!DATA_PTR(self)) {
    svn_swig_rb_raise_svn_fs_already_close();
  }

  svn_swig_rb_destroy_internal_pool(self);
  DATA_PTR(self) = NULL;

  return Qnil;
}

146252f is indeed where the regression was introduced and this needs tweaking slightly to revert to the old behaviour and not throw SWIG_ObjectPreviouslyDeletedError. It isn't clear to me which approach is better in this corner case, so I've gone with the old behaviour for backwards compatibility.

The subversion ruby tests now pass with the change I've committed.

@voxik

This comment has been minimized.

Show comment
Hide comment
@voxik

voxik May 27, 2016

Thx for the fix. It appears it fixes the issues with Subversion:

https://bugzilla.redhat.com/show_bug.cgi?id=1299502#c6

voxik commented May 27, 2016

Thx for the fix. It appears it fixes the issues with Subversion:

https://bugzilla.redhat.com/show_bug.cgi?id=1299502#c6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment