Skip to content
This repository has been archived by the owner on Jun 9, 2022. It is now read-only.

Initial commit of experimental CMake build system #30

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

Conversation

syb0rg
Copy link

@syb0rg syb0rg commented Jul 29, 2016

Built and tested working static library.

@mbait
Copy link
Contributor

mbait commented Aug 2, 2016

Just to save your time - I'm not sure your PR is going to be accepted. First, it's not atomic, second, your changes are questionable (like, you changed int64_t to int), last (but not least), you've been already told that CMake isn't welcome in this project. There are no problems with using an autotools-configured project in a CMake-configured project.

@syb0rg
Copy link
Author

syb0rg commented Aug 2, 2016

it's not atomic

Can you explain what you would like atomic better?

your changes are questionable (like, you changed int64_t to int)

It was necessary to get the code to compile on my build systems, I'm not sure how it would compile under normal circumstances otherwise.

you've been already told that CMake isn't welcome in this project

I was told that it wasn't "needed", not that it wasn't welcome. Is there a reason that CMake isn't welcome in this project? I can't foresee any downsides to adding it.

@mbait
Copy link
Contributor

mbait commented Aug 2, 2016

Can you explain what you would like atomic better?

Atomic changes are easier to review because there's no need to switch contexts.

It was necessary to get the code to compile on my build systems, I'm not sure how it would compile under normal circumstances otherwise.

Obviously you should fix your build system, not the project. In this particular case the change is potentially dangerous because the C standard guarantees almost nothing regarding the size of int, while the size of int64_t is always fixed.

Is there a reason that CMake isn't welcome in this project? I can't foresee any downsides to adding it.

There's already a build system, why would we need another one? If it's added, somebody will have to maintain it.

@syb0rg
Copy link
Author

syb0rg commented Aug 2, 2016

Atomic changes are easier to review because there's no need to switch contexts.

Contexts? This isn't a C concept as far as I know, can you be more explicit?


Obviously you should fix your build system, not the project.

It was failing on all of my build systems across all platforms: Windows, Mac, Linux. That indicates either something wasn't setup in the build properly (didn't seem like the case here), or the project was at fault.

C standard guarantees almost nothing regarding the size of int

The standards specify this size range:

C99 & C11 §5.2.4.2

— minimum value for an object of type int
INT_MIN -32767 // −(215− 1)
— maximum value for an object of type int
INT_MAX +32767 // 215− 1

I agree that it may not be the safest way to handle those situations, but for now my code works on all of my build systems; yours doesn't.


There's already a build system, why would we need another one?

Actual portability. For example: building sphinxbase and pocketsphinx on Windows is a PITA right now. This is a step into making it more streamlined.

If it's added, somebody will have to maintain it.

Maintenance is minimal on build systems (that's the whole point of them). Right now all you have to do is add new files that you create so that they will be compiled... and even that could be fixed so you don't have to do that.

@mbait
Copy link
Contributor

mbait commented Aug 2, 2016

Anyway, I'm not the one who makes the final decision, so this was just my advice. Although I'd still wait @nshmyrev to accept your ideas before starting to write the code. FYI, building cross-platform projects on Windows has always been a pain, mainly because Visual Studio doesn't have a conforming C99 compiler and because Windows is not POSIX-compliant. I'd suggest MinGW build on UNIX-like OS instead.

@syb0rg
Copy link
Author

syb0rg commented Aug 2, 2016

Although I'd still wait nshmyrev to accept your ideas before starting to write the code

The build system is already been written, and my code that uses that build system has already been written. My fork is just serving as a "glue" between this code and mine, hopefully just temporarily.

Visual Studio doesn't have a conforming C99 compiler

Yes and no. It does support many features, but is not fully compliant.

@syb0rg syb0rg force-pushed the master branch 2 times, most recently from 0b5f032 to 5713af4 Compare August 5, 2016 00:49
@syb0rg syb0rg force-pushed the master branch 4 times, most recently from dffd95e to 89a2b8f Compare August 7, 2016 16:57
@syb0rg syb0rg force-pushed the master branch 3 times, most recently from d3fae91 to b21d652 Compare August 7, 2016 19:07
@dhdaines
Copy link
Contributor

dhdaines commented Jun 8, 2022

Hi, I'm not sure who said CMake wasn't welcome, but it wasn't me. It's awful, but it's less awful than autotools, and the configuration files are quite a lot simpler.

That said I imagine lots of things have changed since 6 years ago. Please take a look at this fork which switches everything to CMake, and will change other things as well: https://github.com/dhdaines/pocketsphinx

@nshmyrev
Copy link
Contributor

nshmyrev commented Jun 8, 2022

@dhdaines nice it is going David! I suppose you can push it all to the master instead of a fork.

@dhdaines
Copy link
Contributor

dhdaines commented Jun 8, 2022

Hi @nshmyrev ! I was hoping to discuss some of the other changes with you which is why it's on a fork :)

@nshmyrev
Copy link
Contributor

nshmyrev commented Jun 8, 2022

No need to ask me, please change as you like. I'm always here if needed.

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

Successfully merging this pull request may close these issues.

4 participants