Conversation
|
I left the app version as yyyymmdd but used CalVers for the python version. it would be a bit more streamlined if we switched everything to CalVers. |
iorsh
left a comment
There was a problem hiding this comment.
This is by no means a complete review, it will take me time to go through all the changes. Just a few initial questions, I promise to persevere.
There was a problem hiding this comment.
What is the advantage of having "in-house" CMake definitions? I was actually considering removing the existing ones, since they tend to break arbitrarily once in a while, and the cmake / pkgconfig installation just does its job.
There was a problem hiding this comment.
I have no opinion on this, really. We can try the alternative and if it works I'm happy.
There was a problem hiding this comment.
Just because there is a lot of change with this anyway, I went ahead and made a draft with all of that removed. I can back it out to the earlier files if you have concerns.
|
I don't know if it's feasible, but if this PR could be split into several, that would perhaps simplify future debugging, when we discover issues linked to this change. It would be also easier to review; for example I can blindly approve a sub-PR which only does in an infinite number of files. Don't waste a time on it though if it's not straightforward. |
|
Yeah, I can probably do that fairly easily. I'll think about an appropriate decomposition. |
|
I take it back. It would be very time-consuming to separate the strictly c++ changes from the GLib-replacement changes. I don't think it's worth the cost to improve the review-side. |
|
(In any case, there's not really much hope of a PR with "just" easy changes like all the extern "C" lines, because those would have to be mixed in with the malloc casts and renaming of reserved keyword variables.) |
CMake cleanup: - Remove unnecessary Find modules (FindCairo, FindPango, FindGDK3, FindReadline, FindWOFF2, FindLibunibreak) - these are now handled via pkg-config directly - Add pkg_check_auto macro to PackageUtils.cmake for simpler pkg-config usage - Cairo and Pango come in as transitive dependencies of GDK3/GTK3 ffglib_compat improvements: - Change ff_array_free to take FFArray** and null pointer after free, preventing use-after-free bugs - Add ff_regex_match wrapper for std::regex_match, simplifying fea_isInUFO from 45 lines to 1 line - Add FFStringSet wrapper for std::set<std::string> with case-insensitive comparison, replacing 48-line local implementation in featurefile.c Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
iorsh
left a comment
There was a problem hiding this comment.
Ok, reviewed 102 / 236 files
- Remove redundant #ifdef __cplusplus pair in ffpython.h - Remove deprecated compressors global; use ff_compression_* API in fontview.c - Consolidate platform-specific seed generation in noprefs.c DefaultXUID() - Remove MakeTemporaryFile(), use GFileTmpfile() directly Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- ff_strtod: Replace ff_ascii_strtod with ff_strtod using std::from_chars where available, with manual fallback for macOS/libc++. Simplified to only handle PostScript data (no whitespace/infinity/NaN handling needed). - Decompress: Remove unused compression parameter since extension is now detected internally by ff_decompress_to_temp. - Random number generation: Migrate all rand()/srand() calls to use ff_random_*() functions (std::mt19937). This provides consistent, higher-quality randomness across all platforms with a single engine seeded once at initialization. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add extern "C" guards to inc/basics.h and inc/ustring.h - Remove deprecated 'register' keyword from utf8_strncpy declaration - Add explicit (char*) cast to malloc in memory.c (C++ requires this) - Set LANGUAGE CXX for memory.c and ustring.c in Unicode/CMakeLists.txt This is the first step in porting GLib-using code to C++ so we can replace GLib functions with C++ std:: equivalents for MSVC compatibility. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Remove all 'register' keywords (deprecated in C++17) - Add explicit cast for malloc in u_copynallocm - Add explicit cast for realloc in realloc_tail - Fix const correctness in endswith (const char* for strstr result) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add explicit casts for malloc/realloc returns - Add explicit casts for do_iconv() return values (void* to char*/unichar_t*) - Add ucharmap.c to LANGUAGE CXX in CMakeLists.txt Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add ArabicForms.c, char.c, unialt.c, uninames.c, utype.c to LANGUAGE CXX - All Unicode library files now compile as C++ Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add explicit cast for calloc return value - Add dlist.c to LANGUAGE CXX in CMakeLists.txt Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add 17 more files to LANGUAGE CXX in CMakeLists.txt - Fix const correctness in gwwintl.c (strrchr returns const char*) - Fix string literal assignments in gimagewritexpm.c (const char*) Files now compiling as C++: - dlist.c, gcol.c, gimage.c - gimagereadbmp.c, gimageread.c, gimagereadgif.c, gimagereadpng.c - gimagereadras.c, gimagereadrgb.c, gimagereadtiff.c - gimagereadxbm.c, gimagereadxpm.c - gimagewritebmp.c, gimagewritegimage.c, gimagewritexbm.c, gimagewritexpm.c - gwwintl.c, prefs.c Remaining gutils files (use GLib or have POSIX deps): - fsys.c, gimagereadjpeg.c, gimagewritejpeg.c, gimagewritepng.c, gutils.c Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Remove 'register' keywords (deprecated in C++17) - Add files to LANGUAGE CXX in CMakeLists.txt 20 of 23 gutils files now compile as C++. Remaining files (use GLib): - fsys.c (also has POSIX deps) - gimagewritepng.c - gutils.c Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Headers updated: - inc/utype.h - Unicode type functions - inc/gimage.h - GImage functions - inc/gfile.h - File system functions - inc/dlist.h - Double-linked list functions - inc/intl.h - Internationalization functions This ensures C linkage for functions defined in files compiled as C++, maintaining ABI compatibility with C code that calls these functions. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Files ported (no malloc casts needed): - activeinui.c, autosave.c, bezctx_ff.c, clipnoui.c, crctab.c - dumpbdf.c, flaglist.c, fvmetrics.c, mem.c, nouiutil.c - ofl.c, pua.c, splinefit.c, splineorder2.c, splinerefigure.c - utanvec.c, zapfnomen.c Fixed: Remove 'register' keywords in crctab.c Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Replace all GLib function calls in the fontforge library with portable alternatives to enable MSVC builds for Python wheel distribution. Key changes: - Add ffglib_compat.h with portable replacements for GLib functions: - ff_ascii_strtod/ff_ascii_formatd for locale-independent float I/O - ff_utf8_strlen, ff_strstrip, ff_ascii_strcasecmp - ff_uri_unescape_string, ff_random_*, ff_get_charset - FFArray (replaces GArray), FFList (replaces GList) - Add optional libunibreak support for Unicode line breaking (UAX #14) with simple Latin-text fallback when not available - Add FindLibunibreak.cmake and ENABLE_LIBUNIBREAK build option - Update plugin.c/h to use FFList instead of GList_Glib - Replace GDateTime with standard C time functions in print.c, cvexport.c - Replace g_file_open_tmp/g_mkstemp with portable alternatives - Add ffprocess.c/h for process-related utility functions The fontforge library no longer makes direct GLib function calls. views.h and fontforgeui.h still include ffglib.h for fontforgeexe compatibility (the GUI will continue to use GLib). Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Split ffglib_compat into header (C-compatible declarations) and implementation file (C++ std:: library usage): - ffglib_compat.h: Pure C header with extern "C" function declarations - ffglib_compat.cpp: C++ implementation using standard library Key improvements: - Random numbers use std::mt19937 and std::uniform_*_distribution instead of rand()/srand() for better quality and portability - Use std:: functions (isspace, isdigit, nan, pow, etc.) - FFArray struct is now opaque; access via ff_array_data/ff_array_len - Type-safe C++ template for ff_steal_pointer This architecture keeps platform-specific code minimal and uses portable C++ standard library for the implementation, making it easier to build with any C++ compiler (GCC, Clang, MSVC). Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add strtok_r -> strtok_s mapping for MSVC - Add strcasecmp -> _stricmp mapping for MSVC - Add strncasecmp -> _strnicmp mapping for MSVC - Fix namehash.h to use standard 'inline' instead of '__inline__' - Include ffglib_compat.h in ufo.c for strtok_r compatibility Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add extern "C" wrappers for POSIX filesystem functions: - ff_access() - file accessibility check - ff_unlink() - delete file - ff_rmdir() - remove empty directory - ff_mkdir() - create directory - ff_chdir() - change working directory - ff_getcwd() - get current working directory These use C++17 std::filesystem internally, providing portable implementations that work on Windows, Linux, and macOS without requiring platform-specific ifdefs in calling code. Also define F_OK, X_OK, W_OK, R_OK constants for compatibility. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Replace direct POSIX calls (unlink, access, chdir, rmdir) with portable ff_* wrapper functions that use C++ std::filesystem internally for MSVC/Windows compatibility. Files updated: - fontviewbase.c, groups.c, fvimportbdf.c, autosave.c, autotrace.c, encoding.c, scripting.c, splinefont.c, plugin.c, python.c, sfd.c Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Use simplified workflow focused on Python module builds: - Linux, macOS, and Windows MSVC - No GUI, Python scripting enabled - vcpkg for Windows dependencies Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add mINI header-only INI parser library for plugin configuration - Add vcpkg.json manifest for Windows MSVC dependencies Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Make GLIB dependency conditional: required except for MSVC non-GUI builds - Remove unused ffglib.h includes from fontforge library files - Make GLIB linking conditional in fontforge, inc, and tests CMakeLists - Remove glib from vcpkg.json (not needed for MSVC builds) This allows the Python wheel to be built with MSVC on Windows without requiring GLib, which has build issues on Windows with MSVC. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Remove unused PyFFCvt_Slice() and PyFFCvt_SliceAssign() functions (dead code from deprecated Python 2 slice protocol) - Replace FFByteArray with std::vector<unsigned char> in gimagewritepng.cpp and remove FFByteArray from ffglib_compat entirely - Remove duplicate MIN/MAX macro definitions from ggadget.h and cvpointer.c (already provided by basics.h) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Replace hand-rolled filesystem utilities with std::filesystem - Remove ~190 lines of platform-specific code for path handling, directory creation, and recursive directory removal - Update C++ standard from C++11 to C++17 in CMakeLists.txt - Remove unused Windows DLL path handling from pyhook_smoketest.py Addresses code review feedback from iorsh on PR #5750. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Please add all the new source files to (that's including |
…docs Code changes: - Remove GLib dependency from fontforge core library, move to fontforgeexe - Move MSVC stat compatibility from fontforge-config.h.in to gfile.h - Remove unused pid_webfontserver field from FontView - Move GLib-dependent declarations (CVGetSelectedPoints, visitSelectedControlPoints, visitAllControlPoints) from views.h to charview_private.h Documentation: - Consolidate MSVC_WHEEL_PLAN.md and PYTHON_MODULE_PLAN.md into sphinx docs - Add doc/sphinx/scripting/python-extension.rst for technical details - Expand python.rst with pip installation info - Add temporary WHEEL_BUILD_STATUS.md for implementation tracking Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
777d8f6 has the reformatting and some trivial fixes to remove a conflict with |
|
What is you |
|
It's apparently 21.1.8? |
|
I shifted this to a normal PR. The sooner we merge the sooner we avoid further synch-ing headaches. However, we should also discuss the remaining steps in the WHEEL_BUILD_STATUS.md document. So basically:
If you think this is ready to merge, and you're OK with the CI changes, go ahead and merge it and I'll open an issue for the PyPI considerations so we can discuss that there. |
|
The clang-format problem seems to be that the current file doesn't specify detailed formatting for .c files because the language is Cpp, so C files fall back on Google formatting, which has 2 indents. Do you want me to update the file to treat the two languages similarly? |
|
Yes, please update the I think we can do the release both for wheel and for app in a short time, with the content we already have (this PR and the two pending UI PRs). I'm ok with being involved in a PyPI publishing, but don't quite understand the procedure yet. Could you please check and update the procedure in https://github.com/fontforge/fontforge/wiki/Making-a-release? Making concurrent releases sounds like a good idea in general. |
Add new source files to .clang-files and run ninja format. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
OK, that push took care of the formatting but I'm tinkering with the wheel workflow |
|
Ok, let me know when when you are ready to merge |
Update the wheels.yml workflow to trigger on YYYYMMDD tags (e.g., 20260220) instead of v* tags. The version_provider.py converts this to CalVer format (e.g., 2026.2.20) for the wheel version. This aligns the wheel workflow with the existing release tagging convention used by the application, so a single tag triggers both the app release and the PyPI wheel build. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
That Making-a-release page seems very out of date. It doesn't mention PROJECT_VERSION, which seems to be integral to the current system. How did you do the release last fall? |
|
I think I sort of followed it, but probably just did some adjustments on-fly. |
|
OK, so I changed the wheels workflow to build wheels when a YYYYMMDD tag is added. Eventually we'll set up publishing so that that workflow will have access to a PyPI token and basically everything happens automatically. For now just the wheels will be built. I think this is ready to be merged. We can always tinker with the workflow stuff. The wheels workflow won't run unless there's a version tag or it's run manually. |
It might make sense to add a check that PROJECT_VERSION matches either the tag on the current commit or the tag on the most recent commit, or something like that. |
See the two added temporary documents, one focusing on the MSVC port and one on the module strategy.
This will need some changes but I think it's relatively close to something that can be merged. Still, I'm leaving it as a draft for now.