Package up YARP as a gem #1110

flavorjones · 2023-06-30T20:33:19Z

Summary

Wrap YARP as a gem
- add a gemspec
- add a "rake build" task to package the gem
- add a "rake check_manifest" task to validate the gem packaging
- extconf.rb knows how to make librubyparser
- add CI jobs to test gem packaging and installation
Add test coverage for symbol visibility in static archives and shared objects
Switch the compile-time default to NOT export symbols on non-windows platforms, superseding Define YP_EXPORTED_FUNCTION as empty #1111.

Notable change

The 'compile' rake task is no longer a dependency of the 'test' task, to allow us to run the test suite against the installed gem in CI. CONTRIBUTING.md has been updated. Developers can still run bundle exec rake to do all the things.

Notable design choice

"autoconf" is run during gem packaging, and the "configure" script is in the gem. This reduces the requirements on the installation system.

ext/yarp/extconf.rb

eregon · 2023-07-01T14:11:53Z

I'd like to merge #1088 first to avoid conflicts if possible, I fixed more things in the Makefile.
Also I'd be happy to review this PR next week

flavorjones · 2023-07-01T15:29:29Z

Right now the failing tests on ruby head are because we don't have a way to specify "load the gem's yarp.rb and yarp.so" and the stdlib versions are being loaded (which causes tests to fail). @k0kubun mentioned he has some ideas on how to handle this.

eregon · 2023-07-03T14:39:27Z

I'd like to merge #1088 first to avoid conflicts if possible, I fixed more things in the Makefile.

Done

eregon · 2023-07-03T14:56:43Z

A notable design choice:

* "autoconf" is run during gem packaging, and the "configure" script is in the gem. This reduces the requirements on the installation system.

Definitely the best :) I did the same for TruffleRuby BTW because our macOS CI doesn't have autotools installed/in PATH by default.

flavorjones · 2023-07-03T15:01:12Z

I'm rebasing now.

eregon

Looks great!

tasks/check_manifest.rake

yarp.gemspec

k0kubun · 2023-07-03T16:30:51Z

@k0kubun mentioned he has some ideas on how to handle this.

Today I locally confirmed that non-builtin yarp.rb and yarp.so files could be loaded if you modify $LOAD_PATH. It means that technically it's not mandatory to gemify YARP in the interpreter to replace it with a gem.

I was thinking of somehow doing that somewhere, but since a gemspec is not evaluated when Gemfile loads yarp.gem IIUC, there's no way to make it happen automatically on Gemfile unless you add some hack to the interpreter, rubygems, or bundler.

So, just like the existing convention, I think the YARP that is shipped as part of CRuby should be gemified to allow replacing the YARP implementation with a gem. Of course, the implementation used by the interpreter should be part of the interpreter and not be replaceable by a gem though.

flavorjones · 2023-07-09T17:37:10Z

@tenderlove @k0kubun @jemmaissroff I've updated this branch with:

test coverage of symbol visibility,
switch the compile-time default to hide symbols (superseding Define YP_EXPORTED_FUNCTION as empty #1111)

Note that the "gem install" tests are still failing on ruby-head because of the gem loading issue described above. That failure should go away once ruby stdlib yarp is gemified and contains these commits.

Please let me know what you think.

yarp.gemspec

tenderlove

This seems great to me! Same question as @jemmaissroff though (not a show stopper obviously).

flavorjones · 2023-07-10T21:38:22Z

@tenderlove I answered above, but for posterity: the files listed explicitly in the gemspec which overlap with the globs are generated files, and are not in source control -- so we can't assume they exist. Explicitly listing them is a safety check, and attempting to package the gem without them will fail with an error.

Though, I'm noticing now that include/yarp/config.h is being packaged up because it matches a glob; but we shouldn't be packaging it -- so I'd like to switch to listing every file out individually.

- add a gemspec - add a "rake build" task to package the gem - add a "rake check_manifest" task to validate the gem packaging - extconf.rb knows how to make librubyparser - add CI jobs to test gem packaging and installation Note: the 'compile' rake task is no longer a dependency of the 'test' task, to allow us to run the test suite against the installed gem in CI. Note: the C extension relies on the "configure" script and the generated files, so they have been added as explicit dependencies of the extension compilation task 'compile:yarp'. Co-authored-by: Kevin Newton <[email protected]>

The tests check that the static archive librubyparser.a hides all of the YARP functions, to ensure the symbols do not end up externally visible in a dynamic symbol table and therefore not susceptible to being replaced by the dynamic linker. We expect something like: ``` $ objdump --syms build/librubyparser.a | grep " yp_parse$" 000000000001dbf0 g F .text 0000000000000009 .hidden yp_parse ``` The `g` means the symbol is global, but `.hidden` means that the symbol should not be made external if linked into a shared object (like `yarp.so`). The tests also check that the shared object librubyparser.so contains global versions of the public methods like `yp_parse`: ``` $ nm build/librubyparser.so | grep " yp_parse$" 0000000000021a50 T yp_parse ``` Finally, the tests verify that there is exactly one dynamic symbol in `yarp.so`, which is the initializing function `Init_yarp`. NOTE that we add `-fvisibility=hidden` to the C extension compiler flags to ensure this test passes.

This reverses the convention introduced in ruby#1069, and requires `YP_EXPORT_SYMBOLS` to be defined at compile-time when building a shared library. See related ruby#1111 which describes a symbol leakage problem in Ruby 3.3 (ruby head) that this commit, once merged upstream into Ruby, should address. Note that the tests introduced in a previous commit ensure that the static archive and shared objects are all being created correctly after this change. Co-authored-by: Aaron Patterson <[email protected]>

flavorjones · 2023-07-11T01:32:41Z

I've force-pushed an update that lists every file in the gemspec individually, which also now correctly does not package include/yarp/config.h.

jemmaissroff

Thanks for your work here, Mike!

flavorjones force-pushed the bundle-as-gem branch 10 times, most recently from a649abe to 9ff18cd Compare June 30, 2023 21:29

tenderlove reviewed Jun 30, 2023

View reviewed changes

ext/yarp/extconf.rb Outdated Show resolved Hide resolved

tenderlove mentioned this pull request Jun 30, 2023

Define YP_EXPORTED_FUNCTION as empty #1111

Closed

flavorjones force-pushed the bundle-as-gem branch 6 times, most recently from 68d8d31 to a460b2e Compare July 1, 2023 13:11

flavorjones force-pushed the bundle-as-gem branch from a460b2e to b8a50b9 Compare July 3, 2023 14:04

flavorjones force-pushed the bundle-as-gem branch from b8a50b9 to 65cc4fa Compare July 3, 2023 15:22

eregon approved these changes Jul 3, 2023

View reviewed changes

tasks/check_manifest.rake Outdated Show resolved Hide resolved

yarp.gemspec Outdated Show resolved Hide resolved

flavorjones force-pushed the bundle-as-gem branch 2 times, most recently from a7681bd to 7b2d044 Compare July 3, 2023 16:14

flavorjones force-pushed the bundle-as-gem branch from 7b2d044 to 0030cc9 Compare July 9, 2023 17:24

flavorjones force-pushed the bundle-as-gem branch from 0030cc9 to 8de3697 Compare July 9, 2023 17:33

jemmaissroff reviewed Jul 10, 2023

View reviewed changes

yarp.gemspec Outdated Show resolved Hide resolved

tenderlove approved these changes Jul 10, 2023

View reviewed changes

flavorjones and others added 3 commits July 10, 2023 21:31

flavorjones force-pushed the bundle-as-gem branch from 8de3697 to 9cd27a9 Compare July 11, 2023 01:31

jemmaissroff approved these changes Jul 11, 2023

View reviewed changes

jemmaissroff merged commit 93f46c6 into ruby:main Jul 11, 2023
17 of 20 checks passed

flavorjones deleted the bundle-as-gem branch July 11, 2023 18:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Package up YARP as a gem #1110

Package up YARP as a gem #1110

flavorjones commented Jun 30, 2023 •

edited

Loading

eregon commented Jul 1, 2023

flavorjones commented Jul 1, 2023

eregon commented Jul 3, 2023

eregon commented Jul 3, 2023

flavorjones commented Jul 3, 2023

eregon left a comment

k0kubun commented Jul 3, 2023 •

edited

Loading

flavorjones commented Jul 9, 2023 •

edited

Loading

tenderlove left a comment

flavorjones commented Jul 10, 2023

flavorjones commented Jul 11, 2023

jemmaissroff left a comment

Package up YARP as a gem #1110

Package up YARP as a gem #1110

Conversation

flavorjones commented Jun 30, 2023 • edited Loading

eregon commented Jul 1, 2023

flavorjones commented Jul 1, 2023

eregon commented Jul 3, 2023

eregon commented Jul 3, 2023

flavorjones commented Jul 3, 2023

eregon left a comment

Choose a reason for hiding this comment

k0kubun commented Jul 3, 2023 • edited Loading

flavorjones commented Jul 9, 2023 • edited Loading

tenderlove left a comment

Choose a reason for hiding this comment

flavorjones commented Jul 10, 2023

flavorjones commented Jul 11, 2023

jemmaissroff left a comment

Choose a reason for hiding this comment

flavorjones commented Jun 30, 2023 •

edited

Loading

k0kubun commented Jul 3, 2023 •

edited

Loading

flavorjones commented Jul 9, 2023 •

edited

Loading