ffi / ffi-compiler

Apache License 2.0
32 stars 10 forks source link

Breaking change between 1.3.1 and 1.0.1 #26

Closed ntkme closed 6 months ago

ntkme commented 6 months ago

It appears 1.3.1 was just released to rubygems in last hour, and it immediately broke the installation of google-protobuf-4.26.0-java on JRuby:

Successful run with 1.0.1: https://github.com/sass-contrib/sass-embedded-host-ruby/actions/runs/8271605607/job/22631761286 Broken run since 1.3.1 was released: https://github.com/sass-contrib/sass-embedded-host-ruby/actions/runs/8271884102/job/22632613467

ntkme commented 6 months ago

Linux:

Don't know how to build task 'x86_64-linux/libruby-upb.so' (See the list of available tasks with `rake --tasks`)
Did you mean?  x86_64-linux/ruby-upb.o

Mac:

Don't know how to build task 'x86_64-darwin/libruby-upb.bundle' (See the list of available tasks with `rake --tasks`)
Did you mean?  x86_64-darwin/ruby-upb.o

Windows:

Don't know how to build task 'x86_64-windows/ruby-upb.dll' (See the list of available tasks with `rake --tasks`)
Did you mean?  x86_64-windows/ruby-upb.o

I guess the tasks has been renamed in a way that is no backward compatible?

ntkme commented 6 months ago

Regression is introduced by https://github.com/ffi/ffi-compiler/commit/592f1ba64125cabccb8d2059bdd178ecba86c611

The issue is that in previous version when task is created under a rake namespace, the task would still be created at top level namespace, but in new version the task is now created under the namespace:

Old version:

$ rake -C vendor/bundle/jruby/3.1.0/gems/google-protobuf-4.26.0-java/ext/google/protobuf_c --tasks
rake aarch64-darwin                           # Compile UPB library for FFI
rake aarch64-darwin/libprotobuf_c_ffi.bundle  # Build dynamic library
rake aarch64-darwin/libruby-upb.bundle        # Build dynamic library
rake clean                                    # Remove any temporary products
rake clobber                                  # Remove any generated files

New version:

$ rake -C vendor/bundle/jruby/3.1.0/gems/google-protobuf-4.26.0-java/ext/google/protobuf_c --tasks
rake aarch64-darwin                                          # Compile UPB library for FFI
rake clean                                                   # Remove any temporary products
rake clobber                                                 # Remove any generated files
rake ffi-protobuf:aarch64-darwin/libprotobuf_c_ffi.bundle    # Build dynamic library
rake ffi-protobuf:ffi-upb:aarch64-darwin/libruby-upb.bundle  # Build dynamic library
ntkme commented 6 months ago

I can confirm that ffi-compiler 1.3.2 fixed this issue.

davispuh commented 6 months ago

Yeah I found that aswell. It's legacy all the way... FileTask is special in Rake as it doesn't respect namespaces at all... See https://github.com/ruby/rake/blob/master/lib/rake/file_task.rb#L52 That feature shouldn't have changed anything but who could have known this :smile:

I just pushed a fix and released ffi-compiler 1.3.2

Also by the way that looks like very hacky/fragile approach... https://github.com/protocolbuffers/protobuf/blob/main/ruby/lib/google/tasks/ffi.rake#L88 I mean relying on some global tasks. FFI-Compiler probably should have some API for this rather than going thru Rake. Especially because you're relying on side-effect since FFI::Compiler::CompileTask.new was already created/closed earlier.