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

Switch to CMake-based builds #1360

Closed
wants to merge 15 commits into from

Conversation

iterumllc
Copy link
Collaborator

@iterumllc iterumllc commented May 14, 2021

Description

Some of the likely further development of this repository would benefit from simplification of the C code build system. This PR removes the old files and replaces them with a CMake-driven build started off by the import of scikit-build into setup.py.

This method of building non-python code within python packages is now supposed to be widely supported on Linux, Mac OS X, and more recent versions of Visual Studio. I have only tested on Linux, however, so one active role of this PR is to see how the build process goes on the other platforms and make any needed adjustments.

Some source directories are moved to more standard locations. The documentation of manual builds in doc/FDK_Build_Notes.md has been revised. Various workflow files were slightly changed.

What this PR does not change:

  1. It has no new C or Python code.
  2. It attempts to retain the previous compilation defines.
  3. The release builds are still compiled without optimization and the executables are still not stripped.
  4. The executables are still supplied directly as "scripts", although we should consider putting them in platform-specific data directories and using entrypoint-based wrappers as in https://github.com/scikit-build/ninja-python-distributions/tree/master/ninja , as this would be an overall cleaner and more accepted solution.

Added: As of 4e7241b the default build type is now "RelWithDebInfo", which combines optimization and putting debugging symbols into the final binary. While the latter takes up some extra space it can be useful for users who want to step through the programs in a debugger.

Checklist:

  • I have followed the Contribution Guidelines
  • I have added test code and data to prove that my code functions correctly
  • I have verified that new and existing tests pass locally with my changes
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation

Closes #1124

@lgtm-com
Copy link

lgtm-com bot commented May 14, 2021

This pull request introduces 2 alerts when merging fa995f5 into 33acc7d - view on LGTM.com

new alerts:

  • 1 for Unused import
  • 1 for Declaration hides parameter

@miguelsousa
Copy link
Member

@iterumllc I've authorized the CI to run.

@lgtm-com
Copy link

lgtm-com bot commented May 14, 2021

This pull request introduces 1 alert when merging 53130bd into 33acc7d - view on LGTM.com

new alerts:

  • 1 for Declaration hides parameter

@lgtm-com
Copy link

lgtm-com bot commented May 14, 2021

This pull request introduces 1 alert when merging eb5c3d8 into 33acc7d - view on LGTM.com

new alerts:

  • 1 for Declaration hides parameter

@lgtm-com
Copy link

lgtm-com bot commented May 14, 2021

This pull request introduces 1 alert when merging 4e842be into 33acc7d - view on LGTM.com

new alerts:

  • 1 for Declaration hides parameter

@lgtm-com
Copy link

lgtm-com bot commented May 14, 2021

This pull request introduces 1 alert when merging cd1c965 into 33acc7d - view on LGTM.com

new alerts:

  • 1 for Declaration hides parameter

@skef
Copy link
Collaborator

skef commented May 14, 2021

OK, most of that churn was various flags I hadn't dug out of the old config yet.

It could still benefit from some tuning -- it appears that some platforms enable optimization on their own and some don't, and a number of the build flags are questionable in 2021. Still, it's building and passing tests on every platform in the workflow.

@skef
Copy link
Collaborator

skef commented May 14, 2021

Oh, I guess I did make one small functional change. The code at the top-level of makeotfexe now only imports the mac.c implementations of the curdir() and sep() file operations. curdir() was the same in both and using a POSIX forward-slash on Windows is supposed to be fine since XP (or before?).

It would be easy enough to put back if folks want to keep the distinction.

@lgtm-com
Copy link

lgtm-com bot commented May 14, 2021

This pull request introduces 1 alert when merging 4b1ca04 into 33acc7d - view on LGTM.com

new alerts:

  • 1 for Declaration hides parameter

@lgtm-com
Copy link

lgtm-com bot commented May 23, 2021

This pull request introduces 1 alert when merging 4e7241b into ea0cfec - view on LGTM.com

new alerts:

  • 1 for Declaration hides parameter

@iterumllc
Copy link
Collaborator Author

This is now superseded by #1367

@iterumllc iterumllc closed this May 31, 2021
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.

Better build system
3 participants