-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Ruby: support conan v2 and expose more configuration options for native extension gems #18338
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
aad2096
to
2c50de6
Compare
This comment has been minimized.
This comment has been minimized.
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
2c50de6
to
67ca60d
Compare
This comment has been minimized.
This comment has been minimized.
67ca60d
to
0aec480
Compare
This comment has been minimized.
This comment has been minimized.
ffed62a
to
5e00d0e
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
# when --static-linked-ext is used, ruby defines EXTSTATIC as 1 | ||
# But when ruby itself is static there's nothing, so: | ||
# We define RUBY_STATIC_RUBY when ruby itself is static | ||
# We define RUBY_STATIC_LINKED_EXT when the ruby extensions are static (same as EXTSTATIC but clearer) | ||
defs = {} | ||
tested_options = self.dependencies[self.tested_reference_str].options | ||
if not tested_options.shared: | ||
defs["RUBY_STATIC_RUBY"] = 1 | ||
if tested_options.with_static_linked_ext: | ||
defs["RUBY_STATIC_LINKED_EXT"] = 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't these be added to self.cpp_info.defines
in package_info()
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I honestly have no idea... you tell me! What I'm trying to do is to pass it to the CMakeLists.txt so they are defined in the executable: https://github.com/jmarrec/conan-center-index/blob/e5c6c958b0b90067ec7d93a468405095f4eff8be/recipes/ruby/all/test_package/CMakeLists.txt#L14-L20
And then I can run different C++ code via a macro: https://github.com/jmarrec/conan-center-index/blob/e5c6c958b0b90067ec7d93a468405095f4eff8be/recipes/ruby/all/test_package/test_package.cpp#L17-L27
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@valgur is right, these would be better placed in the package_info() of the upstream recipe if this is something that is expected for consumer use - consumers shouldn't have to define the variables themselves if they're provided by upstream.
Having said that, if upstream doesn't provide these and you want to use dependency options to do this kind of thing, you shouldn't be trying to parse these in the build()
method anyway. Populating cmake with defines needs to happen in the generate()
method. To illustrate (for your awareness), passing defines to cmake should be done as follows:
def generate(self):
ruby_opts = self.dependencies[self.tested_reference_str].options
tc = CMakeToolchain(self)
tc.preprocessor_definitions["RUBY_STATIC_RUBY"] = not ruby_opts.shared
tc.preprocessor_definitions["RUBY_STATIC_LINKED_EXT"] = not ruby_opts.shared and ruby_opts.with_static_linked_ext
def build(self):
cmake = CMake(self)
cmake.configure()
cmake.build()
See the docs for CMakeToolchain for more info.
Note: I've assumed that you want to pass these as defines to the consumer build - as far as I can see, these might actually be better as cmake variables rather than defines, since I can't see a reason why a consumer library would ever want to do the following - I would imagine in most cases the code base itself wouldn't care whether the symbols were resolved statically or dynamically.
#ifdef RUBY_STATIC_RUBY
// do some stuff
#endif
If these don't need to be defines, then the above can change to the following to pass these options as cmake variables instead
def generate(self):
ruby_opts = self.dependencies[self.tested_reference_str].options
tc = CMakeToolchain(self)
tc.variables["RUBY_STATIC_RUBY"] = not ruby_opts.shared
tc.variables["RUBY_STATIC_LINKED_EXT"] = not ruby_opts.shared and ruby_opts.with_static_linked_ext
…/ zlibstaticd too
Note: ruby has a `--with-destdir=DESTDIR specify default directory to install` option, maybe I should use that?
For libffi and libyaml, starting at 3.2.x ruby supports: * Downloading the SOURCES to libyaml / libffi * Configure with --with-libyaml-source-dir / --with-libffi-source-dir * It then builds the libffi / libyaml itself https://bugs.ruby-lang.org/issues/18571
… to date, and don't do extract-extlibs
> post_package(): WARN: [APPLE RELOCATABLE SHARED LIBS (KB-H077)] install_name dir of these shared libs is not @rpath: libruby.3.1.dylib, libruby.dylib conan-io#18338 (comment)
During the build, miniruby is built, then it is actually used to build the extension libs. The c3i v2 runner on macOS is failing ``` linking miniruby dyld[16179]: Library not loaded: @rpath/libgmp.10.dylib Referenced from: <BC0A8868-CFAE-3B40-A112-FCAC741169E4> /Users/jenkins/w/prod-v2/bsr@2/80054/defbf/p/b/ruby1af0f3d020f64/b/build-release/miniruby Reason: tried: '/usr/local/lib/libgmp.10.dylib' (no such file), '/usr/lib/libgmp.10.dylib' (no such file, not in dyld cache) make: *** [exe/ruby] Abort trap: 6 make: *** Deleting file `exe/ruby' ```
f073d6f
to
7ddb85e
Compare
Conan v1 pipeline ✔️All green in build 26 (
Conan v2 pipeline ❌
The v2 pipeline failed. Please, review the errors and note this is required for pull requests to be merged. In case this recipe is still not ported to Conan 2.x, please, ping See details:Failure in build 26 (
Note: To save resources, CI tries to finish as soon as an error is found. For this reason you might find that not all the references have been launched or not all the configurations for a given reference. Also, take into account that we cannot guarantee the order of execution as it depends on CI workload and workers availability. |
Re the macOS V2 error that fails when shared: so we build miniruby, then we need to execute miniruby. Which is going to search for the dylibs. The conanrunenv does have the |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
This pull request has been automatically closed because it has not had recent activity. Thank you for your contributions. |
> post_package(): WARN: [APPLE RELOCATABLE SHARED LIBS (KB-H077)] install_name dir of these shared libs is not @rpath: libruby.3.1.dylib, libruby.dylib conan-io#18338 (comment)
Specify library name and version: ruby/3.1.0
Ruby: support conan v2 and expose more configuration options for native extension gems
Reboots:
Both PRs were opened more than a year ago and got stale and closed.
Changelog (old commit messages):
conan v2 support (work done by myself + a merge of @SpaceIm 's branch from PR ruby: conan v2 support #12208 to reconcile both)
Extend testing to require one of the native extensions
--with-opt-dir
instead of--with-xx-dir
that ruby isn't respecting.conanio/gccXX (eg 10) removed the libxxx-dev (eg libgmp-dev) from the image. This made me realize that the conan deps weren't being picked up by the build. The openssl was, because --with-openssl-dir is explicitly used in ruby config.
So here, we rely on --with-opt-dir.
cf: https://bugs.ruby-lang.org/issues/19014#change-99241 (--with-gmp-dir was added on September 14, 2022, wrote this in Sep 22, 2022)
https://gist.github.com/mrkn/6647630