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

Happy. Halloween 🎃👻 #42

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

Happy. Halloween 🎃👻 #42

wants to merge 102 commits into from

Conversation

zeehio
Copy link
Contributor

@zeehio zeehio commented Oct 31, 2020

I don't know if you celebrate it or not. I don't expect you to review this pull request (it is just meant to scare you if you look at the large diff).

However today I managed to have this:

continuous integration success

Continuous integration of speech tools being built on Ubuntu, OSX, windows (using mingw), windows (using Microsoft Visual C/C++ compiler & vs2019 project on 32 and 64 bit builds)!

There are a lot of commits to be squashed and reorganized, and there are some visual studio warnings to be fixed, but I thought you might be happy to see the results of the fast iteration building process from the meson build system 😃

I need to clean things up and bring festival to the same level of compatibility but still. Enjoy the weekend!

@lenzo-ka
Copy link
Collaborator

I am very glad to see this and I apologize for being slow on the uptake... I’ve been distracted.

Meson is a nice addition and it is important to make the base build without it as well. This seems to do a good job of making meson optional. Going forward, it makes things a little harder to validate against both paths, so I hope you will be maintaining the meson build lest it fall away like previous python bindings.

That said, I am really looking forward to python3 bindings for all the components. That would be a major contribution.

@lenzo-ka
Copy link
Collaborator

I love seeing the CI!

@lenzo-ka
Copy link
Collaborator

lenzo-ka commented Jan 7, 2021

If you don't really want to merge this we should go ahead and close?

@zeehio
Copy link
Contributor Author

zeehio commented Jan 8, 2021

The approach we have been using so far has been to split this huge pull request into smaller pull requests that should be easier to review.

This has led to the successful review and merge of #39 and #40. We have #41 as well to be reviewed. During those reviews we have made further changes/rebases fixing additional issues, and branches have diverged a bit.

Once #41 is merged I can either rebase this whole branch so it becomes a bit smaller (although still too large to review at once in my opinion, but it's you the one reviewing it) or cherry pick a bunch of related commits from here, maybe squash some of them and present them to you in a pull request easy for you to review.

I don't mind (a) closing this, (b) rebasing this and asking you to review it at once or (c) marking this as a draft. If you choose (a) or (c), I am interested in continue working in preparing, discussing and merging smaller pull requests as we have been doing... whatever you prefer.

@lenzo-ka
Copy link
Collaborator

lenzo-ka commented Jan 8, 2021

(B) rebase and let’s review. I trust your judgment, and if some things go by in the near term review they can be corrected. I’ll look it over, while assuming the best. Your contributions have been excellent.

@nikitalita
Copy link

Any movement on this? this branch is the only way I can get it to compile on windows with msvc

@zeehio
Copy link
Contributor Author

zeehio commented Mar 7, 2021

For the last months I've been saturated at work and at home, without too much time to work on this. I do plan to keep working on this, as soon as I have the time again.

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.

3 participants