Project

General

Profile

Actions

Feature #15425

closed

Store MJIT header into Ruby versioned directory.

Added by vo.x (Vit Ondruch) over 5 years ago. Updated 6 months ago.

Status:
Closed
Target version:
-
[ruby-core:90585]
Tags:

Description

This is a followup of #15391 which fixes JIT to respect the configuration options. However, I still wonder, why the file is versioned and why it is not stored in the versioned directory alongside all other internal Ruby headers. I believe, that while it now respects the header configuration flags, it still does not respect options such as "--with-ruby-version"


Files


Related issues 1 (0 open1 closed)

Related to Ruby master - Bug #16416: Suspicious include directories.ClosedActions

Updated by nobu (Nobuyoshi Nakada) over 5 years ago

  • Status changed from Open to Feedback

It should be placed under the directory which has the version and architecture name.
How did you configure it?

Updated by vo.x (Vit Ondruch) over 5 years ago

I am using --with-rubyhdrdir=/usr/include --with-rubyarchhdrdir=/usr/include --enable-multiarch configuration options. This is the output:

$ tree
.
`-- ruby-2.6.0
    |-- ruby
    |   |-- backward
    |   |   |-- classext.h
    |   |   |-- rubyio.h
    |   |   |-- rubysig.h
    |   |   |-- st.h
    |   |   `-- util.h
    |   |-- backward.h
    |   |-- debug.h
    |   |-- defines.h
    |   |-- digest.h
    |   |-- encoding.h
    |   |-- intern.h
    |   |-- io.h
    |   |-- missing.h
    |   |-- onigmo.h
    |   |-- oniguruma.h
    |   |-- re.h
    |   |-- regex.h
    |   |-- ruby.h
    |   |-- st.h
    |   |-- subst.h
    |   |-- thread.h
    |   |-- thread_native.h
    |   |-- util.h
    |   |-- version.h
    |   `-- vm.h
    |-- ruby-2.6.0
    |   `-- x86_64-linux
    |-- ruby.h
    `-- x86_64-linux
        |-- rb_mjit_min_header-2.6.0.h
        `-- ruby
            `-- config.h

7 directories, 28 files

Different headers respect the configuration differently. The header should be probably placed on the same location as "config.h" for consistency. The ruby-2.6.0/ruby-2.6.0/x86_64-linux should not be created at all.

Actions #3

Updated by vo.x (Vit Ondruch) about 5 years ago

  • Status changed from Feedback to Open

Updated by mame (Yusuke Endoh) over 4 years ago

  • Assignee set to k0kubun (Takashi Kokubun)

Updated by nobu (Nobuyoshi Nakada) over 4 years ago

It seems stored in the rubyarchhdrdir already.

$ LC_ALL=C tree usr/include/
usr/include/
|-- rb_mjit_min_header-2.7.0.h
|-- ruby
|   |-- assert.h
|   |-- backward
|   |   |-- classext.h
|   |   |-- cxxanyargs.hpp
|   |   |-- rubyio.h
|   |   |-- rubysig.h
|   |   |-- st.h
|   |   `-- util.h
|   |-- backward.h
|   |-- config.h
|   |-- debug.h
|   |-- defines.h
|   |-- digest.h
|   |-- encoding.h
|   |-- intern.h
|   |-- io.h
|   |-- missing.h
|   |-- onigmo.h
|   |-- oniguruma.h
|   |-- re.h
|   |-- regex.h
|   |-- ruby.h
|   |-- st.h
|   |-- subst.h
|   |-- thread.h
|   |-- thread_native.h
|   |-- util.h
|   |-- version.h
|   `-- vm.h
`-- ruby.h

2 directories, 30 files

Updated by k0kubun (Takashi Kokubun) over 4 years ago

why the file is versioned

Basically rubyarchhdrdir directory should protect the header from different Ruby versions, but you almost proved that it's necessary when you passed --with-rubyhdrdir=/usr/include --with-rubyarchhdrdir=/usr/include. At least 2.6's header can't be used from 2.7.

why it is not stored in the versioned directory alongside all other internal Ruby headers

IIRC, nobu chose the directory to support multi-arch (or universal?) build, and it seems to follow his intention as he quoted.

The header should be probably placed on the same location as "config.h" for consistency. The ruby-2.6.0/ruby-2.6.0/x86_64-linux should not be created at all.

I have no idea why ruby-2.6.0/ruby-2.6.0/x86_64-linux is created, but rb_mjit_min_header-2.6.0.h is not placed in that directory and so I believe it should be a separated ticket.

it still does not respect options such as "--with-ruby-version"

So maybe respecting --with-ruby-version in the header file name (or removing the version fragment) is the only thing we need to discuss/implement here?

Updated by vo.x (Vit Ondruch) over 4 years ago

So maybe respecting --with-ruby-version in the header file name (or removing the version fragment) is the only thing we need to discuss/implement here?

That would be good start. The version there is probably useless.

This is the configuration I used:

$ ./configure --with-ruby-version='foo'

And this is the result:

$ cd /usr/local/include/
$ tree
.
└── ruby-foo
    ├── ruby
    │   ├── assert.h
    │   ├── backward
    │   │   ├── classext.h
    │   │   ├── cxxanyargs.hpp
    │   │   ├── rubyio.h
    │   │   ├── rubysig.h
    │   │   ├── st.h
    │   │   └── util.h
    │   ├── backward.h
    │   ├── debug.h
    │   ├── defines.h
    │   ├── digest.h
    │   ├── encoding.h
    │   ├── intern.h
    │   ├── io.h
    │   ├── missing.h
    │   ├── onigmo.h
    │   ├── oniguruma.h
    │   ├── regex.h
    │   ├── re.h
    │   ├── ruby.h
    │   ├── st.h
    │   ├── subst.h
    │   ├── thread.h
    │   ├── thread_native.h
    │   ├── util.h
    │   ├── version.h
    │   └── vm.h
    ├── ruby-
    │   └── x86_64-linux
    ├── ruby.h
    └── x86_64-linux
        ├── rb_mjit_min_header-2.7.0.h
        └── ruby
            └── config.h

7 directories, 30 files
Actions #8

Updated by vo.x (Vit Ondruch) over 4 years ago

  • Related to Bug #16415: MJIT is using/creating some suspicious include directories added
Actions #9

Updated by vo.x (Vit Ondruch) over 4 years ago

  • Related to deleted (Bug #16415: MJIT is using/creating some suspicious include directories)
Actions #10

Updated by vo.x (Vit Ondruch) over 4 years ago

  • Related to Bug #16416: Suspicious include directories. added

Updated by vo.x (Vit Ondruch) over 4 years ago

Isn't part of the problem, that the ruby_version variable is read much later than it is used for configuration of MJIT header names? These are the relevant lines:

https://github.com/ruby/ruby/blob/40026a408df5e3576380f6c1d8bf6c119fa2e32b/configure.ac#L3099
https://github.com/ruby/ruby/blob/40026a408df5e3576380f6c1d8bf6c119fa2e32b/configure.ac#L3719

Updated by vo.x (Vit Ondruch) over 4 years ago

The two attached patches might improve the issue. The 0001-Store-MJIT-header-into-Ruby-versioned-directory-1.patch includes the ruby_version in the header name, while the 0001-Store-MJIT-header-into-Ruby-versioned-directory-2.patch removes the version from the header altogether, because the header lives in versioned directory anyway.

However, they still don't move the file to the same place as the config.h :/

Updated by nobu (Nobuyoshi Nakada) over 4 years ago

Why is this needed?
It seems having no effects.

--- a/configure.ac
+++ b/configure.ac
@@ -3096,9 +3096,6 @@ AC_ARG_ENABLE(multiarch,
 	      [multiarch=], [unset multiarch])
 AS_IF([test ${multiarch+set}], [
    AC_DEFINE(ENABLE_MULTIARCH)
-   MJIT_HEADER_INSTALL_DIR=include/'${arch}/${RUBY_VERSION_NAME}'
-], [
-   MJIT_HEADER_INSTALL_DIR=include/'${RUBY_VERSION_NAME}/${arch}'
 ])
 
 archlibdir='${libdir}/${arch}'
@@ -3772,6 +3769,12 @@ AS_IF([test "${LOAD_RELATIVE+set}"], [
     RUBY_EXEC_PREFIX=''
 ])
 
+AS_IF([test ${multiarch+set}], [
+   MJIT_HEADER_INSTALL_DIR=include/'${arch}/${RUBY_VERSION_NAME}'
+], [
+   MJIT_HEADER_INSTALL_DIR=include/'${RUBY_VERSION_NAME}/${arch}'
+])
+
 AC_SUBST(RUBY_EXEC_PREFIX)
 
 AC_SUBST(libdirname, ${multiarch+arch}libdir)
Actions #14

Updated by k0kubun (Takashi Kokubun) almost 3 years ago

  • Status changed from Open to Feedback
Actions #15

Updated by jeremyevans0 (Jeremy Evans) over 2 years ago

  • Tracker changed from Bug to Feature
  • ruby -v deleted (ruby 2.6.0rc2 (2018-12-15 trunk 66408) [x86_64-linux])
  • Backport deleted (2.4: UNKNOWN, 2.5: UNKNOWN)

Updated by vo.x (Vit Ondruch) 7 months ago

This is probably no issue for Ruby 3.3. The header in question was dropped by git|31f4b2d86bfbc753cec9be376719acc4b120e944. So this can be closed (I used to have powers to close the tickets myself, how have I lost them? 🤔)

Actions #17

Updated by Eregon (Benoit Daloze) 6 months ago

  • Status changed from Feedback to Closed
Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0