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 signature for hash_foreach value #17

Merged
merged 4 commits into from
Sep 13, 2024
Merged

Conversation

kbrock
Copy link
Contributor

@kbrock kbrock commented Sep 11, 2024

Resolves one issue in #14

Change

Updated c files to fix warnings (that prevent this from building on a mac and linux machine)

Before

gem install ovirt-engine-sdk -v4.6.0
# fail

gem install --verbose ovirt-engine-sdk -v4.6.0 -- --with-cflags="-Wno-error=incompatible-function-pointer-types -Wno-error=implicit-function-declaration"
# succeed

After

(hopefully. I can't do this since I can't push the changes)

gem install ovirt-engine-sdk -v4.6.0

@sunpoet This is your code change from freebsd. If this gets merged, you will be able to remove it from your custom patch. Is it ok to apply here? Thnx

If you want me to add attribution and signoff for you, or if you'd prefer creating your own PR. Please let me know.

UPDATE: contact with author and signed off

--

@jhernand I pulled out your c change from #10 - I feel the gemspec changes are probably also needed, but trying to keep this as minimal as possible.

I left your signoff for the partial commit, please let me know if you agree or feel this does not properly reflect your intent.

@sandrobonazzola I noticed your commit does not have a Signoff. Please advise

@@ -490,12 +493,20 @@ static int ov_http_client_debug_function(CURL* handle, curl_infotype type, char*
/* Get the pointer to the transfer: */
ov_http_transfer_ptr(transfer, transfer_ptr);

/* Execute the debug code with the global interpreter lock acquired, as it needs to call Ruby methods: */
/* The global interpreter lock may be acquired or not, so we need to check and either call the task directly
or else call it after acquiring the lock. Note that the `ruby_thread_has_gvl_p` function that we ue to
Copy link

Choose a reason for hiding this comment

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

Suggested change
or else call it after acquiring the lock. Note that the `ruby_thread_has_gvl_p` function that we ue to
or else call it after acquiring the lock. Note that the `ruby_thread_has_gvl_p` function that we use to

Copy link
Contributor Author

@kbrock kbrock Sep 12, 2024

Choose a reason for hiding this comment

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

ooh, I see this commit in the other PR, will bring it across

I will pull from #10 but I see that it is not signed off

@kbrock
Copy link
Contributor Author

kbrock commented Sep 12, 2024

I can pull just my own commits out into a separate PR (since it is signed off) if you would like.

@sunpoet
Copy link

sunpoet commented Sep 12, 2024

@sunpoet This is your code change from freebsd. If this gets merged, you will be able to remove it from your custom patch. Is it ok to apply here? Thnx

If you want me to add attribution and signoff for you, or if you'd prefer creating your own PR. Please let me know.

Thanks for handling this. Please feel free to use that patch. It'll be great if the fix is merged upstream.

@sandrobonazzola sandrobonazzola requested review from mwperina and a team September 13, 2024 07:07
jhernand and others added 4 commits September 13, 2024 09:07
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`.

Signed-off-by: Juan Hernandez <juan.hernandez@redhat.com>
Thanks

Co-authored-by: eremeyev <andrey.eremeyev@gmail.com>
Signed-off-by: Keenan Brock <keenan@thebrocks.net>
This can be found https://github.com/ruby/ruby/blob/5d358b660d41e64de301f428dc0300a52a6f9566/internal/thread.h#L77

All users of this method seem to just declare the method up front, as access to ruby's internal headers are discouraged

Signed-off-by: Keenan Brock <keenan@thebrocks.net>
Copy link
Member

@mwperina mwperina left a comment

Choose a reason for hiding this comment

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

LGTM

@mwperina mwperina merged commit ef16365 into oVirt:master Sep 13, 2024
1 of 2 checks passed
@kbrock kbrock deleted the fix_pointer branch September 13, 2024 14:45
@kbrock kbrock removed their assignment Sep 13, 2024
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