Skip to content
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

Update to Ruby 3 #10

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Update to Ruby 3 #10

wants to merge 4 commits into from

Conversation

jhernand
Copy link
Contributor

@jhernand jhernand commented Feb 1, 2024

Currently the SDK doesn't build or work with Ruby 3:

  • The code generator needs at least Weld 3 in order to work with Java 11.

  • The versions of rake, rubocop and webrick need to be udpated to work with Ruby 3.

  • The C code needs to be udpated to use the rb_thread_call_with_gvl only when the code doesn't already hold the GVL. This is probably due to changes in libcurl.

This patch contains fixes for all those issues.

@jhernand
Copy link
Contributor Author

jhernand commented Feb 1, 2024

Not that this will also need a new version of the API metamodel containing this change: oVirt/ovirt-engine-api-metamodel#34

@kbrock
Copy link
Contributor

kbrock commented Sep 11, 2024

Would it be easier to merge if we split this PR apart?
e.g.: the rubocop changes seem like a no brainer

Or is the issue that there just isn't much time and multiple PRs are actually more difficult?

@kbrock
Copy link
Contributor

kbrock commented Sep 12, 2024

I pulled the c code fix and put into #17 (kept attribution)

@kbrock
Copy link
Contributor

kbrock commented Sep 12, 2024

@sandrobonazzola FYI - your commit does not have signoff

@sandrobonazzola
Copy link
Member

@sandrobonazzola FYI - your commit does not have signoff

I just hit rebase :-)
@jhernand / @eremeyev can you fix commit message with missing sign off?

jhernand and others added 2 commits September 13, 2024 09:07
Currently the SDK doesn't build or work with Ruby 3:

- The code generator needs at least Weld 3 in order to work with Java
  11.

- The versions of `rake`, `rubocop` and `webrick` need to be udpated to
  work with Ruby 3.

- The C code needs to be udpated to use the `rb_thread_call_with_gvl`
  only when the code doesn't already hold the GVL. This is probably
  due to changes in `libcurl`.

This patch contains fixes for all those issues.

Signed-off-by: Juan Hernandez <[email protected]>
@sandrobonazzola sandrobonazzola requested review from mwperina and a team and removed request for eremeyev September 13, 2024 07:11
@mwperina
Copy link
Member

The problem with this change is, that weld 3 super hard to package as RPM due to missing dependencies, that's why we are still using weld 2.y. If we merge this PR before weld 3.y is available as RPM on cbs.centos.org, build process will be completely broken:

https://cbs.centos.org/koji/packageinfo?packageID=8971

@kbrock
Copy link
Contributor

kbrock commented Sep 13, 2024

@mwperina thank you

Can we take the weld changes out of here? (and put into another PR)
So this would be mergable?

UPDATE: I created #19 which contains the ruby changes but not the Weld changes
Any little bit of progress is still progress

@dupondje
Copy link
Member

@kbrock or @jhernand : Can you check the build failure? Guess some additional stuff needs to be fixed.

Copy link
Member

@dupondje dupondje left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The build still fails. Needs to be checked.
Is it also possible to make a proper rebase? :)

@@ -66,8 +66,8 @@ limitations under the License.
<!-- The generator runs in a CDI environment, implemented by Weld: -->
<dependency>
<groupId>org.jboss.weld.se</groupId>
<artifactId>weld-se</artifactId>
<version>2.3.0.Final</version>
<artifactId>weld-se-core</artifactId>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we change to weld-se-shaded?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants