eserte / perl-tk

the perl module Tk
https://metacpan.org/release/Tk
Other
44 stars 30 forks source link

Tk still broken on SrawberryPerl-5.38.2 #104

Open sisyphus opened 4 months ago

sisyphus commented 4 months ago

There's absolutely nothing new about this issue - but it still hasn't been addressed. The attached patch allows Tk to build on StrawberryPerl-5.38.2.

It's the same patch to X.h as provided at https://github.com/eserte/perl-tk/compare/master...gh-87. However, in addition to that, it also replaces (again in X.h) the following piece of non-portable code:

#if defined(_WIN64) && defined(_MSC_VER)
typedef __int64 XID;
#else
typedef unsigned long XID;
#endif

with a a rendition that ports correctly:

#if defined(_WIN64)
#   if defined(_MSC_VER)
    typedef __int64 XID;
#   else
    typedef unsigned long long XID;
#   endif
#else
typedef unsigned long XID;
#endif

I think that both of those fixes emanate from @chrstphrchvz. (The solution to the non-portability issue was identified in https://github.com/eserte/perl-tk/issues/92.) They're not something for which I can claim any credit, and I'm not trying to stomp all over the excellent work of anyone else. I just get very annoyed that this hasn't yet been fixed in https://github.com/eserte/perl-tk - and I am prepared to re-formulate this bug report as a PR, if that's what it's going to take.

Better still, of course, if we can also have a new cpan release that includes this fix, before perl-5.40 is released. (This would greatly enhance the chances of having Tk included in the Strawberry Perl 5.40 release.)

win32_patch.txt

Lamprecht commented 4 months ago

I confirm, that Tk builds on StrawberryPerl-5.38.2 with the patch attached above. All tests are passing.

shawnlaffan commented 3 months ago

I've created a patched distribution from the latest Tk commit. For some reason git did not recognise the format so I manually applied them.

I was also unable to update the version to append _001 as it needs to be in several places. If someone can provide a patch for that then that would be helpful.

https://github.com/StrawberryPerl/Perl-Dist-Strawberry/releases/tag/patched_cpan_modules

Whether we include it in the full SP release needs discussion. The compressed blib dir is another 2MB so would increase the downloads commensurately. This is not really very much in the scheme of things these days but it is still non-zero, and I would suspect the majority of users do no need Tk, even though there are many who do.

mohawk2 commented 3 months ago

@eserte Are you open to sharing commit bits on this repo, and PAUSE co-maint, to do these various maintenance tasks?

Lamprecht commented 3 months ago

I agree that it's desirable to get a version to cpan that builds on strawberry. However (correct me if i am wrong) we should also try to be in sync with tcl-tk, which can be found here: tcl-tk X.h.

Unfortunately the tcl-tk X.h also fails to compile with SP 5.38.2:

../pTk/mTk/xlib/X11/X.h:67:26: error: expected '=', ',', ';', 'asm' or '__attribute__' before 'XID'
   67 | typedef unsigned __int64 XID;
      |                          ^~~
sisyphus commented 3 months ago

Unfortunately the tcl-tk X.h also fails to compile with SP 5.38.2

I think all it needs is for the offending typedeffing to be made portable. Mingw-w64 ports of gcc understand the __int64 type, but only if _mingw.h has been included. Apparently _mingw.h is not included for this particular compilation.

I grabbed https://github.com/tcltk/tk/blob/74b174eed61acafb5e8bc26bfe8d8d33fcadd7aa/xlib/X11/X.h, and applied this patch to it:

--- X.h_tcl_tk  2024-05-06 11:07:06.086219000 +1000
+++ X.h 2024-05-06 11:43:06.491410400 +1000
@@ -65,7 +65,11 @@
 #  ifndef _XTYPEDEF_XID
 #    define _XTYPEDEF_XID
 #    ifdef _WIN64
+#      if defined(_MSC_VER)
 typedef unsigned __int64 XID;
+#      else
+typedef unsigned long long XID;
+#      endif
 #    else
 typedef unsigned long XID;
 #    endif

I then replaced the existing X.h in the Tk github repo (https://github.com/eserte/perl-tk) with this patched version of X.h. That Tk source then built fine for me on both SP-5.38.2-PDL and SP-5.39.10.

I have a strong aversion to making PRs, but I'll try to find the energy to make one to https://github.com/tcltk/tk/ later today.

UPDATE: I submitted https://github.com/tcltk/tk/pull/37, but apparently they want me to instead provide a report at https://core.tcl-lang.org/tk/tktnew - so now there's also https://core.tcl-lang.org/tk/tktview/ff5417505be90f5abbbf6ea4c7a3ac3bae6bc8b1

mohawk2 commented 2 weeks ago

Hi @eserte, I've been encouraged to bump this issue by @oalders. Are you open to sharing co-maint and/or collaboration status on this?

mohawk2 commented 2 weeks ago

The core Tcl/Tk repo has apparently now implemented this fix, so all that would be needed here is to copy the Tcl/Tk one over, and that would endure over future core Tk releases.