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

Replace Getopt with GLib option parsing #378

Open
danyeaw opened this issue May 4, 2024 · 11 comments
Open

Replace Getopt with GLib option parsing #378

danyeaw opened this issue May 4, 2024 · 11 comments
Milestone

Comments

@danyeaw
Copy link

danyeaw commented May 4, 2024

Gvsbuild has a very old version of Enchant from an old XChat fork. Since Getopt isn't very cross-platform, would it be possible to replace it with GLib's command parsing functionality?

@rrthomas
Copy link
Owner

rrthomas commented May 5, 2024

Did you have an example in mind? I've never had a report of a system on which Enchant won't build owing to getopt. GitHub CI builds it on macOS, Windows and Ubuntu. getopt(3) is a POSIX standard since at least 1996. I can always use the gnulib polyfill if necessary.

Having said that, I would happily accept a patch to the vala-commands branch to use GLib (i.e. written in Vala). Since I am in the process of rewriting the commands in Vala, I'll only be making essential bug fixes to the current C versions from now on.

@danyeaw
Copy link
Author

danyeaw commented May 5, 2024

Hi @rrthomas, yup, Windows with MSVC doesn't have POSIX GNU tools and libraries, here is the traceback:

(tar) Exporting enchant
Building project enchant (2.7.3)
        mkdir .\..\bin
        mkdir .\..\bin\release
        mkdir .\..\bin\release
A subdirectory or file .\..\bin\release already exists.
        mkdir .\..\bin\release\obj
        mkdir .\..\bin\release\pdb
        cl -I.\..\src  -I.\..  -Ic:\usr\include  -IC:\gtk-build\gtk\x64\release\include\glib-2.0  -IC:\gtk-build\gtk\x64\release\include\glib-2.0\glib  -IC:\g
tk-build\gtk\x64\release\include\glib-2.0\gmodule  -IC:\gtk-build\gtk\x64\release\include\glib-2.0\..\..\lib\glib-2.0\include -FI ".\..\src\config.h"  -MD  -W
1  -EHsc  -DNDEBUG -D_NDEBUG -Ox -MP2  -DWINDOWS  -D_WINDOWS  -DWIN32  -D_WIN32  -DUNICODE  -D_UNICODE  -Fo".\..\bin\release\obj\\" /favor:AMD64 /D_WIN64 -c .\..\src\enchant.c -D_ENCHANT_BUILD=1  -DWINDLL
Microsoft (R) C/C++ Optimizing Compiler Version 19.39.33523 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.

enchant.c
.\..\src\enchant.c(38): fatal error C1083: Cannot open include file: 'getopt.h': No such file or directory
NMAKE : fatal error U1077: 'cl -I.\..\src  -I.\..  -Ic:\usr\include  -IC:\gtk-build\gtk\x64\release\include\glib-2.0  -IC:\gtk-build\gtk\x64\release\include\g
lib-2.0\glib  -IC:\gtk-build\gtk\x64\release\include\glib-2.0\gmodule  -IC:\gtk-build\gtk\x64\release\include\glib-2.0\..\..\lib\glib-2.0\include -FI ".\..\sr
c\config.h"  -MD  -W1  -EHsc  -DNDEBUG -D_NDEBUG -Ox -MP2  -DWINDOWS  -D_WINDOWS  -DWIN32  -D_WIN32  -DUNICODE  -D_UNICODE  -Fo".\..\bin\release\obj\\" /favor:AMD64 /D_WIN64 -c .\..\src\enchant.c -D_ENCHANT_BUILD=1  -DWINDLL' : return code '0x2'
Stop.

Making this part of the updates as you move the commands to Vala sounds like a great plan.

@rrthomas
Copy link
Owner

rrthomas commented May 5, 2024

Windows with MSVC doesn't have POSIX GNU tools and libraries

I'd be happy to add support for proprietary toolchains as paid work.

Otherwise, please try rrthomas/master where I have added the gnulib getopt-posix module, which should fix this problem. Please let me know either way.

@rrthomas
Copy link
Owner

rrthomas commented May 5, 2024

I would also accept a patch to the GitHub Actions to build Enchant on Windows with MSVC, once it's confirmed working.

@danyeaw
Copy link
Author

danyeaw commented May 6, 2024

Hi @rrthomas,

Here is what I get:

(git) Cloning https://github.com/rrthomas/enchant.git to C:\gtk-build\src\git-exp\enchant
Cloning into 'C:\gtk-build\src\git-exp\enchant'...
remote: Enumerating objects: 8983, done.
remote: Counting objects: 100% (1490/1490), done.
remote: Compressing objects: 100% (434/434), done.
remote: Total 8983 (delta 1044), reused 1249 (delta 955), pack-reused 7493
Receiving objects: 100% (8983/8983), 9.65 MiB | 1.43 MiB/s, done.
Resolving deltas: 100% (6124/6124), done.
Already on 'master'
Your branch is up to date with 'origin/master'.
(git) Exporting directory C:\gtk-build\build\x64\release\enchant
Building project enchant (2.7.3)
        mkdir .\..\bin
        mkdir .\..\bin\release
        mkdir .\..\bin\release
A subdirectory or file .\..\bin\release already exists.
        mkdir .\..\bin\release\obj
        mkdir .\..\bin\release\pdb
        cl -I.\..\src  -I.\..  -Ic:\usr\include  -IC:\gtk-build\gtk\x64\release\include\glib-2.0  -IC:\gtk-build\gtk\x64\release\include\glib-2.0\glib  -IC:\g
tk-build\gtk\x64\release\include\glib-2.0\gmodule  -IC:\gtk-build\gtk\x64\release\include\glib-2.0\..\..\lib\glib-2.0\include -FI ".\..\src\config.h"  -MD  -W
1  -EHsc  -DNDEBUG -D_NDEBUG -Ox -MP2  -DWINDOWS  -D_WINDOWS  -DWIN32  -D_WIN32  -DUNICODE  -D_UNICODE  -Fo".\..\bin\release\obj\\" /favor:AMD64 /D_WIN64 -c .\..\src\enchant.c -D_ENCHANT_BUILD=1  -DWINDLL
Microsoft (R) C/C++ Optimizing Compiler Version 19.39.33523 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.

enchant.c
.\..\src\enchant.c(38): fatal error C1083: Cannot open include file: 'getopt.h': No such file or directory
NMAKE : fatal error U1077: 'cl -I.\..\src  -I.\..  -Ic:\usr\include  -IC:\gtk-build\gtk\x64\release\include\glib-2.0  -IC:\gtk-build\gtk\x64\release\include\g
lib-2.0\glib  -IC:\gtk-build\gtk\x64\release\include\glib-2.0\gmodule  -IC:\gtk-build\gtk\x64\release\include\glib-2.0\..\..\lib\glib-2.0\include -FI ".\..\sr
c\config.h"  -MD  -W1  -EHsc  -DNDEBUG -D_NDEBUG -Ox -MP2  -DWINDOWS  -D_WINDOWS  -DWIN32  -D_WIN32  -DUNICODE  -D_UNICODE  -Fo".\..\bin\release\obj\\" /favor:AMD64 /D_WIN64 -c .\..\src\enchant.c -D_ENCHANT_BUILD=1  -DWINDLL' : return code '0x2'
Stop.

Yup, I can contribute a GitHub Action.

@rrthomas
Copy link
Owner

rrthomas commented May 6, 2024

I don't understand how the build is run here, maybe you can help. In particular, Makefile.am contains:

AM_CPPFLAGS = -I$(top_srcdir) $(ISYSTEM)$(top_builddir)/libgnu $(ISYSTEM)$(top_srcdir)/libgnu -I$(top_srcdir)/lib $(GLIB_CFLAGS) $(WARN_CFLAGS) -DG_LOG_DOMAIN='"libenchant"'

Note the two occurrences of libgnu. This is the directory that contains getopt.h after bootstrap is run.

But I don't see libgnu mentioned anywhere in the invocation of the C compiler above.

@rrthomas
Copy link
Owner

rrthomas commented May 6, 2024

Yup, I can contribute a GitHub Action.

Great, a PR that adds a suitable stanza to .github/workflows/c-cpp.yml would be much appreciated.

@danyeaw
Copy link
Author

danyeaw commented May 8, 2024

I looked at this more closely on how the build is working on Windows. We are patching in a NMake makefile.mak which is posted here:
https://github.com/wingtk/gvsbuild/blob/main/gvsbuild/patches/enchant/src/makefile.mak

nmake /nologo -f makefile.mak DLL=1 X64=1 MFLAGS=-MD GLIBDIR=%(gtk_dir)s\include\glib-2.0

So your changes on your fork aren't getting pulled in because we aren't using the bootstrap script (no bash) or GNU Make. This might be a larger project than just replacing Getopt.

@rrthomas
Copy link
Owner

rrthomas commented May 8, 2024

OK, so this is not an Enchant issue. Thanks!

@rrthomas rrthomas closed this as completed May 8, 2024
@rrthomas
Copy link
Owner

Reopening, as I will use GLib's command-line parser in the Vala commands.

@rrthomas rrthomas reopened this May 26, 2024
@rrthomas
Copy link
Owner

Unfortunately, GOption doesn't support long options that start with a single dash, so I won't be able to use it for enchant-lsmod until version 3 (see #374), i.e. breaking backwards compatibility.

@rrthomas rrthomas added this to the v3 milestone Aug 12, 2024
rrthomas added a commit that referenced this issue Oct 6, 2024
Use GLib’s option parsing for enchant but not enchant-lsmod (see #378).

This commit also includes updates to the GLib VAPI.

Remove enchant_get_user_language from the public API, as it’s now trivial,
and was probably not applicable to all applications anyway.
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

No branches or pull requests

2 participants