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

onig deprecation warnings #145

Open
vadi2 opened this issue Jul 16, 2024 · 22 comments
Open

onig deprecation warnings #145

vadi2 opened this issue Jul 16, 2024 · 22 comments

Comments

@vadi2
Copy link
Contributor

vadi2 commented Jul 16, 2024

The onig library is throwing up a lot of errors about deprecated functionality, e.g.

/Users/vadim.peretokin/Programs/Mudlet/3rdparty/edbee-lib/vendor/onig/st.c:856: warning: passing arguments to a function without a prototype is deprecated in all versions of C and is not supported in C2x [-Wdeprecated-non-prototype]
/Users/vadim.peretokin/Programs/Mudlet/3rdparty/edbee-lib/vendor/onig/st.c:856:6: warning: passing arguments to a function without a prototype is deprecated in all versions of C and is not supported in C2x [-Wdeprecated-non-prototype]
if (PTR_EQUAL(tab, &entries[i], hash_value, key))
^
/Users/vadim.peretokin/Programs/Mudlet/3rdparty/edbee-lib/vendor/onig/st.c:177:35: note: expanded from macro 'PTR_EQUAL'
((ptr)->hash == (hash_val) && EQUAL((tab), (key_), (ptr)->key))
^
/Users/vadim.peretokin/Programs/Mudlet/3rdparty/edbee-lib/vendor/onig/st.c:175:62: note: expanded from macro 'EQUAL'
#define EQUAL(tab,x,y) ((x) == (y) || (*(tab)->type->compare)((x),(y)) == 0)
^
image

Is there anything we can do about it?

gamecreature added a commit that referenced this issue Jul 16, 2024
@gamecreature
Copy link
Member

I've just tried to update Onigmo to the latest version 6.2.0, but it still throws these errors.
For now I've silenced these warnings for clang and gcc. (don't know if these warnings happen in vstudio)

Edbee uses the https://github.com/k-takata/Onigmo?tab=readme-ov-file Oniguruma mod, but this project seems to stand still. For the future we should check if the original https://github.com/kkos/oniguruma can be used.

@gamecreature
Copy link
Member

@vadi2 I've made a branch with a newer Onigmo library and silencing the specific warning.
It seems to work on OS X, I don't I you have to opportunity to check if it compiles on other OS-es

Like I mentioned for the future I think a better option is to upgrade to Oniguruma, but I think that could have some issues with some unsupported regexp options. (Got to checkout what the latest Ruby core uses)

@vadi2
Copy link
Contributor Author

vadi2 commented Jul 16, 2024

Compile is happier now using that branch!

I agree it would be nice to change over to Oniguruma. I think it would help in compiling the wasm version as well - last time I tried that, I got blocked by something on Onigmo.

@SlySven
Copy link
Contributor

SlySven commented Aug 19, 2024

Please see: https://github.com/edbee/edbee-lib#r145499264 ...

@gamecreature
Copy link
Member

gamecreature commented Aug 19, 2024

@vadi2, @SlySven do you if there's a way to solve this issue? (so both warnings are silenced). Perhaps via GCC version conditional?

@vadi2
Copy link
Contributor Author

vadi2 commented Aug 20, 2024

@SlySven have you got ideas?

@SlySven
Copy link
Contributor

SlySven commented Oct 21, 2024

One other detail that might encourage a switch back to Oniguruma from the Onigmo fork is that that latter has had no updates (other than for typos - some of them imported from the original library) since 2019 and even those haven't made it into the 6.2.0 release of that date - the last Unicode version it refers to is 12.1 though it is not clear to me if that is important. In comparison the original released it's own 6.9.9 release only last month, including an update for Unicode 16.0 - so if you can reformulate the appropriate parts of your code to use the original it would probably be for the best...

@gamecreature
Copy link
Member

gamecreature commented Oct 31, 2024

Hi @SlySven, I totally agree with this. When I started edbee I choose Onigmo because it had extension which were used by Textmate grammars. Using Oniguruma will cause some Textmate not to function anymore. (If that's still the case I don't know)

@gamecreature gamecreature pinned this issue Dec 22, 2024
gamecreature added a commit that referenced this issue Dec 28, 2024
- remove onigmo (vendor/onig)
- add oniguruma subtree oniguruma with some build patches

    Squashed 'vendor/oniguruma/oniguruma/' content from commit 5eaee9f5

    git-subtree-dir: vendor/oniguruma/oniguruma
    git-subtree-split: b68e7f7e036eede1e929b7c9ae5995af76339593
gamecreature added a commit that referenced this issue Dec 28, 2024
- remove onigmo (vendor/onig)
- add oniguruma subtree oniguruma with some build patches

    Squashed 'vendor/oniguruma/oniguruma/' content from commit 5eaee9f5

    git-subtree-dir: vendor/oniguruma/oniguruma
    git-subtree-split: b68e7f7e036eede1e929b7c9ae5995af76339593
gamecreature added a commit that referenced this issue Jan 4, 2025
- remove onigmo (vendor/onig)
- add oniguruma subtree oniguruma with some build patches

    Squashed 'vendor/oniguruma/oniguruma/' content from commit 5eaee9f5

    git-subtree-dir: vendor/oniguruma/oniguruma
    git-subtree-split: b68e7f7e036eede1e929b7c9ae5995af76339593
@gamecreature
Copy link
Member

gamecreature commented Jan 10, 2025

Hi @vadi2 and @SlySven, I just finished a feature/oniguruma branch.
This branch embeds the latest Oniguruma version.

I tried to build it for cmake / qmake on Windows / Mac OS X (Linux should be fine but I'm not sure).

It uses an unpatched copy of Oniguruma.
The build script adds some patches to it (mainly the config.h) to include some edbee-defaults to prevent build warnings. (Which are still there).

@vadi2
Copy link
Contributor Author

vadi2 commented Jan 10, 2025

Nice, I will try it!

Semi-related, last time I tried to compile Mudlet for WASM, I was blocked by issues in this area... would be nice if those are fixed too.

@vadi2
Copy link
Contributor Author

vadi2 commented Jan 14, 2025

Builds failed on Ubuntu, macOS using CMake for the same reason: config.h could not be found: https://github.com/Mudlet/Mudlet/actions/runs/12767874867/job/35587100250?pr=7661#step:21:379

Windows builds with qmake failed with... a strange error: https://github.com/Mudlet/Mudlet/actions/runs/12767874863/job/35587100183?pr=7661#step:7:85

@gamecreature
Copy link
Member

gamecreature commented Jan 14, 2025

Hi @vadi2, that's strange.

Config.h should have be generated via configure on a *nix/os x environment.
The win32 environments should copy the config.h.windows.in to config.h.

vendor/oniguruma/CMakeLists.txt contains the following code:

For cmake:

SET(ONIG_DIR "${CMAKE_CURRENT_SOURCE_DIR}/oniguruma")

IF(NOT WIN32)
  message(STATUS "ONIGURUMA Unix Like - patch and configure")
  EXECUTE_PROCESS(
    COMMAND "cp -Rp ${CMAKE_CURRENT_SOURCE_DIR}/patch/* ${ONIG_DIR}"
    WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}
  )

  EXECUTE_PROCESS(
    COMMAND ${ONIG_DIR}/configure
    WORKING_DIRECTORY ${ONIG_DIR}
  )
ENDIF(NOT WIN32)

IF(WIN32) 
  message(STATUS "ONIGURUMA Win32 - patch and copy config.h")
  file(COPY ${CMAKE_CURRENT_SOURCE_DIR}/patch/ DESTINATION ${CMAKE_CURRENT_SOURCE_DIR}/oniguruma/)
  file(COPY_FILE ${CMAKE_CURRENT_SOURCE_DIR}/oniguruma/src/config.h.windows.in ${CMAKE_CURRENT_SOURCE_DIR}/oniguruma/src/config.h)
ENDIF(WIN32)

For Qmake: (.pri)

# !win32:system($$PWD/oniguruma/configure)
!win32:system("cp -Rp $$PWD/patch/* $$PWD/oniguruma/")
## The line below doesn't work because autoreconf is not in the default path
# !win32:system("cd $$PWD/oniguruma; autoreconf -vfi; ./configure; cd -")
!win32:system("cd $$PWD/oniguruma; ./configure; cd -")

win32 {
    edbee_xcopy_command.target = edbee_xcopy_files
    edbee_xcopy_command.commands = xcopy /s /e /Y /I $$shell_quote($$shell_path($$PWD/patch/*)) $$shell_quote($$shell_path($$PWD/oniguruma/)) 
    edbee_xcopy_command.commands += && copy $$shell_quote($$shell_path($$PWD/oniguruma/src/config.h.windows.in)) $$shell_quote($$shell_path($$PWD/oniguruma/src/config.h))
    PRE_TARGETDEPS += edbee_xcopy_files
    QMAKE_EXTRA_TARGETS += edbee_xcopy_command
}

This should have been executed with qmake or cmake
Maybe this configuration is too complex and we just need to dump a general config.h. With some smart defines to include the correct configuration for the environment.

I don't really sure what's the best choice with this.

@vadi2
Copy link
Contributor Author

vadi2 commented Jan 14, 2025

A general config.h would be nice I reckon, because otherwise generating it takes time.

@SlySven
Copy link
Contributor

SlySven commented Jan 15, 2025

For the Windows build case it seems to be using Windows native xcopy.exe:

xcopy /s /e /Y /I '/D/a/Mudlet/Mudlet/3rdparty/edbee-lib/vendor/oniguruma/patch/*' /D/a/Mudlet/Mudlet/3rdparty/edbee-lib/vendor/oniguruma/oniguruma/ && copy /D/a/Mudlet/Mudlet/3rdparty/edbee-lib/vendor/oniguruma/oniguruma/src/config.h.windows.in /D/a/Mudlet/Mudlet/3rdparty/edbee-lib/vendor/oniguruma/oniguruma/src/config.h

the thing is - for Mudlet Windows builds - this is being run in a bash shell in a pseudo-POSIX environment - so the Unix equivalent commands should work - it seems though that what looks like an autoconf setup in the 3rd party dependency is not, I am guessing, configured (a bit of humour there! 😀) for mingw/mingw-w64/(maybe even `cygwin) type systems...

I've got to boot up Windows for something else this afternoon so I'll play with this though I can't promise anything at this point.

@gamecreature
Copy link
Member

gamecreature commented Jan 15, 2025

Hi @SlySven I'm working on a simple general config.h (no external commands, like autoconf or other (x)copy operation needed).
The last commit on the feature/onigurma branch works this way. (Tested with OS X with cmake and qmake).
I still need to test the windows version, but that should because this simply includes the default windows config header.
(I think this is much easier for build process)

@SlySven
Copy link
Contributor

SlySven commented Jan 15, 2025

For the moment any Windows builds for our project does need to support both 32 and 64 bit Windows.

As an aside I do recall that in the past (haven't checked recently) that config.h file was constantly being tweaked to be different to the shipped one to have one of the HAVE_XXXX being commented out (disabled)/uncommented (enabled) {I don't recall which} on FreeBSD. But that is a whole different issue than this one and I'll defer that for now...

@gamecreature
Copy link
Member

Building the latest commit via qmake and cmake on windows 11 (64bits) seems to work correctly. The general supplied windows config file of oniguruma supports both 32/64 bits (with defines).
I don't know if it compiles on WSL (It's very frustrating there so many platform it can be run on).

@SlySven
Copy link
Contributor

SlySven commented Jan 15, 2025

I don't know if it compiles on WSL (It's very frustrating there so many platform it can be run on).

Well that (getting things to make a workable Makefile on anything) was the reason for the whole autoconf toolset - but not everyone groks it well enough to make it work for them...

😀

@vadi2
Copy link
Contributor Author

vadi2 commented Jan 16, 2025

Hmm, so I have most definitely updated the edbee-lib submodule to yesterday's commit correctly, but the config.h build issues persist. What is good now is that it is the same error across cmake in macos and linux and qmake in windows:

windows: https://github.com/Mudlet/Mudlet/actions/runs/12802647668/job/35694079755?pr=7661#step:20:487
macos: https://github.com/Mudlet/Mudlet/actions/runs/12802647668/job/35694080225?pr=7661#step:20:328
linux: https://github.com/Mudlet/Mudlet/actions/runs/12802647668/job/35694080058?pr=7661#step:20:627

Everything does look correct on edbee's part, so I am not immediately sure where the issue is, could be something I did. I'll look at it more later on.

@gamecreature
Copy link
Member

gamecreature commented Jan 16, 2025

You're right, when I'm looking at GitHub I see the config.h is missing. (Sorry about that)
Despite the .gitignore, I added it with git add -f.
I think it's better I also patch the .gitignore to make sure it's committed.

@gamecreature
Copy link
Member

Just pushed a new commit. config.h is now definitely in it (https://github.com/edbee/edbee-lib/blob/feature/oniguruma/vendor/oniguruma/oniguruma/src/config.h)

@vadi2
Copy link
Contributor Author

vadi2 commented Jan 16, 2025

Nice, that builds and runs okay! I did some light testing and it checks out.

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

When branches are created from issues, their pull requests are automatically linked.

3 participants