search

jabber-tools / cognitive-services-speech-sdk-rs

Apache License 2.0
24 stars 15 forks source link

Upgrade sdk and add Windows support #11

Closed wangfu91 closed 5 months ago

wangfu91 commented 6 months ago

Changes in this PR:

adambezecny commented 6 months ago

hi,

first of all, thank you for your contribution and sorry for delayed response, I am quite busy with other things these days. I believe changes you did are really great and hope to merge them into library. The problem with this PR is that it is too big and thus hard to review & test reliably. I would suggest to split it into separate PRs where you will be always addressing just one aspect/feature. Something like:

PR1 - windows build PR2 - SDK upgrade PR3 - minor fixes

The more granular it will be the easier for me to review, test & merge. Keep in mind I did not touch this code base for some time, I need to refresh my rusty memory :)

thanks for understanding, looking forward additional PRs. For now keeping this one just for reference, once we address all features in separate PRs I will close this one.

regards,

Adam

wangfu91 commented 6 months ago

Thank you for your valuable feedback and kind words! I really appreciate it.

I completely understand your concerns about the size of the PR and the challenges it presents for review and testing. I was initially unsure about whether to submit a large PR or break it down into smaller ones, but I'm now convinced that smaller PRs would be more manageable and efficient.

I'll start working on breaking it down into smaller, more focused PRs shortly.

Best Regards.

adambezecny commented 5 months ago

hi,

I have tried to publish merged changes into crates.io and got this error:

error: failed to verify package tarball

Caused by:
  Source directory was modified by build.rs during cargo publish. Build scripts should not modify anything outside of OUT_DIR.
  Added: /Users/adambezecny/rustprojects/cognitive-services-speech-sdk-rs/target/package/cognitive-services-speech-sdk-rs-0.3.0/SpeechSDK/macOS
        /Users/adambezecny/rustprojects/cognitive-services-speech-sdk-rs/target/package/cognitive-services-speech-sdk-rs-0.3.0/SpeechSDK/macOS/MicrosoftCognitiveServicesSpeech-MacOSXCFramework-1.37.0.zip
...

So now I remember why SDK had to be downloaded into target folder :) So next PR should be about fixing this, otherwise we cannot publish your upgrade to 1.37.0

wangfu91 commented 5 months ago

Caused by: Source directory was modified by build.rs during cargo publish. Build scripts should not modify anything outside of OUT_DIR.

Oh, I wasn't aware of this limitation.

So now I remember why SDK had to be downloaded into target folder :) So next PR should be about fixing this, otherwise we cannot publish your upgrade to 1.37.0

Yes, seems the SDK must be downloaded into the OUT_DIR now, I was kinda hoping to avoid that , but, won't the generated src/ffi/bindings.rs also cause this cargo publish error?

adambezecny commented 5 months ago

we can give it a try but it was working before so I guess for some reason bindings are OK. There is a question whether bindings should be actually part of source code or rather generated into target folder as well. Most libraries do not include any generated bindings into source code, this is kind of special for this library since I wanted to see them just for better overview. If bindings will be causing we will put them into target as well.

adambezecny commented 5 months ago

ok, version 0.3.1 adding windows build support is out. thank you very much for this change, I am really excited for windows being added!

do you plan any further changes? Let me know please. If there is nothing else I will close this PR as well.

Once again great thanks for all your contributions.

regards,

Adam

wangfu91 commented 5 months ago

Thank you for reviewing and merging my PRs :), no further changes are planned at the moment, I think this PR can be closed.