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

CとJavaのブロッキングAPIを実装 #705

Merged
merged 10 commits into from
Dec 10, 2023

Conversation

qryxip
Copy link
Member

@qryxip qryxip commented Dec 4, 2023

内容

#702 で実装したブロッキングAPIをC APIとJava APIで使い、両者からtokioの依存を外します。

  • C
  • Java

ロガーの初期化をどうするかですが、次の方針に従います。

        // FIXME: `into_result_code_with_error`を`run`とかに改名し、`init_logger`をその中に移動

追記: #705 (comment)

関連 Issue

#662
#702

その他

@qryxip
Copy link
Member Author

qryxip commented Dec 4, 2023

ロガーの初期化をどこに置くのかについては選択肢がいくつかあると思います。例えば次のように毎回init_logger()を呼ぶというのもありなはずで、draftの間に意見を伺えればと思っています。

#[no_mangle]
pub unsafe extern "C" fn f() -> VoicevoxResultCode {
    let _ = init_logger();
    into_result_code_with_error((|| {})())
}

#[no_mangle]
pub unsafe extern "C" fn g() -> VoicevoxResultCode {
    let _ = init_logger();
    into_result_code_with_error((|| {})())
}

@qryxip qryxip marked this pull request as ready for review December 10, 2023 08:03
@qryxip
Copy link
Member Author

qryxip commented Dec 10, 2023

ロガーの初期化ですが、C APIについては、全ての関数にロガー初期化を明示的にはさむようにしました。

 #[no_mangle]
 pub unsafe extern "C" fn voicevox_synthesizer_new(
     open_jtalk: &OpenJtalkRc,
     options: VoicevoxInitializeOptions,
     out_synthesizer: NonNull<Box<VoicevoxSynthesizer>>,
 ) -> VoicevoxResultCode {
+    init_logger_once();
     into_result_code_with_error((|| {
         let options = options.into();

         let synthesizer = VoicevoxSynthesizer::new(open_jtalk, &options)?.into();
         out_synthesizer.as_ptr().write_unaligned(synthesizer);
         Ok(())
     })())
 }

Javaではclass Dllの末尾で直接呼ぶようにしました。

@@ -64,5 +64,11 @@ abstract class Dll {
         throw new RuntimeException("Failed to load Voicevox Core DLL for " + target, e);
       }
     }
+
+    new LoggerInitializer().initLogger();
+  }
+
+  static class LoggerInitializer {
+    native void initLogger();
   }
 }

@qryxip qryxip requested a review from Hiroshiba December 10, 2023 08:10
@Hiroshiba
Copy link
Member

あすみません、返信できてませんでした 🙇

ロガーの初期化ですが、ちょっと考えた感じCもJavaも実装された方法が良いのかなと思いました。
明示的に呼んでもらう方法もありますが、うーん・・・・・・・。
主要な機能だけに入れるのはありかもですが、なんかこっちが予期しない流れもあるだろうと思うので、であればまあ毎回呼ぶのがやっぱり無難なのかなと!

Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

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

LGTM!!!!!

お疲れ様でした!!!

あ、C APIは意外とinit_loggerを明示的に呼んでもらう形でも良いかなとちょっと思いました。
C APIを使うのはプログラミングに割と慣れてる人だと思うので、ドキュメント読んでくれるかなと。
(あとexampleに書いておけば十分そう)

@Hiroshiba Hiroshiba merged commit 0788c2e into VOICEVOX:main Dec 10, 2023
45 checks passed
@qryxip qryxip linked an issue Mar 17, 2024 that may be closed by this pull request
@qryxip qryxip mentioned this pull request Mar 17, 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.

同期版APIを作る?
2 participants