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

Win32 build of 2.x #84

Merged
merged 15 commits into from
Dec 29, 2017
Merged

Win32 build of 2.x #84

merged 15 commits into from
Dec 29, 2017

Conversation

phlash
Copy link
Contributor

@phlash phlash commented Dec 5, 2017

Hi Alex, win32 build based on Gnome-provided dependant packages (goocanvas2, gtk+-3.10), no curl support yet, hence the conditional compilation of curl bits in src tree. Some instructions provided :)

@csete
Copy link
Owner

csete commented Dec 5, 2017

Thanks Phil, I will take a look at it after 2.0 is released. In general I would prefer that we don't use a version number that has not been released yet, as it can cause a lot of confusion. Like the latest windows package was called 1.4, but there has never been any 1.4 release :)

The command "git describe" or the summary at the end of configure gives a current version number with respect to the latest version tag, currently 1.9.87 - the last number is incremented at each commit, so you can just use 1.9 until 2.0 is released.

@phlash
Copy link
Contributor Author

phlash commented Dec 6, 2017

Righto, I'll see if I can automated the versioning from git describe..

@phlash phlash changed the title First win32 build of 2.0 First win32 build of 2.x candidate Dec 6, 2017
if (gdk_window_get_state(widget->window) & GDK_WINDOW_STATE_MAXIMIZED)
{
return FALSE;
}
} */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The GtkWidget in Gtk3 does not have a windows member. So shouldn't it simply say:

if (gdk_window_get_state(gtk_widget_get_window(widget)) & GDK_WINDOW_STATE_MAXIMIZED)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very probably, if needed at all since this is a workaround for a Gtk 2 bug. I'll check the bug report and update as appropriate. Thanks for the review.

Copy link
Contributor

@gvanem gvanem Dec 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another idea I have; why not require libcurl and use that for the Hamlib connection too? Like curl_easy_setopt(curl, CURLOPT_CONNECT_ONLY, 1). etc.

I don't know the details of Hamlib. Maybe it's close to a ping-pong protocol. Then libcurl could be used for all Hamlib comms.

src/main.c Outdated
if (gdk_window_get_state(widget->window) & GDK_WINDOW_STATE_MAXIMIZED)
{
return FALSE;
}
} */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

@csete
Copy link
Owner

csete commented Dec 9, 2017

I have pushed the GtkWindow changes.

I am wondering why curl had to be disabled to make the build work? For now, curl is required even on windows, otherwise user will not be able to update TLE data and the application becomes useless after a few weeks.

@csete
Copy link
Owner

csete commented Dec 9, 2017

@gvanem:

Another idea I have; why not require libcurl and use that for the Hamlib connection too? Like curl_easy_setopt(curl, CURLOPT_CONNECT_ONLY, 1). etc.

I don't know the details of Hamlib. Maybe it's close to a ping-pong protocol. Then libcurl could be used for all Hamlib comms.

The hamlib protocol is a simple text-based TCP protocol. It can even be used over telnet, but it requires frequent exchange of data. For radio control read/write frequency can be even 10 Hz or more. I don't know if there would be an advantage of using libcurl over raw TCP sockets here. Anyway, this is another topic.

@phlash
Copy link
Contributor Author

phlash commented Dec 10, 2017

I left libcurl out of the first build simply to reduce the pain, and I wasn't sure which of these to use: https://curl.haxx.se/download.html#Win32, since there was no documentation available from earlier builds on cross-compile dependencies.

@phlash
Copy link
Contributor Author

phlash commented Dec 23, 2017

Since I was unable to find a suitable libcurl dev package (all officially listed Win32 packages on curl.haxx.se are not development capable), I have written a 20-line replacement using wininet. This successfully downloads both TLE and Transponder data on my Win7 VM.

if (NULL == hInt)
return 0x80000000 | (int)GetLastError();

HINTERNET hUrl = InternetOpenUrl(hInt, url, NULL, 0, INTERNET_FLAG_RELOAD, 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a neat replacement for libcurl. But could you adapt it to compile with a C-89 compiler?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May I ask why we need that level of compatibility?
I wish to enable more gcc checks and standard compliance once I'm done with Gtk+ 3 migration, but I was thinking about C11. So, now I am wondering why C-89 is better?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why we would aim for C-89 either, but happy to move my variable declarations to the top if it helps.

@csete
Copy link
Owner

csete commented Dec 24, 2017

Thanks for the update Phil. It is beginning to look good :)
I will give it a try as soon as I have some time to spare.

win32/README Outdated

Compile tested on Debian 9 (stretch) using standard mingw-w64 toolchain.

Sorry, no installer package as yet, you will need to copy out all the
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO we don't need any installer. A simple ZIP package like the ones we used to have seems sufficient and much easier to maintain.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, Makefile could probably do with a 'dist' target then to put that together. On it.

@@ -1,6 +1,6 @@
<?xml version='1.0' encoding='UTF-8' standalone='yes'?>
<assembly xmlns='urn:schemas-microsoft-com:asm.v1' manifestVersion='1.0'>
<assemblyIdentity name="GPredict" processorArchitecture="x86" version="1.3.99.0" type="win32"/>
<assemblyIdentity name="GPredict" processorArchitecture="x86" version="VERSION_STRL" type="win32"/>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we generating 32 bit binaries with mingw64 or does this need to be updated?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are generating 32-bit binaries (-m32 in CC options). In the interests of avoiding complexity for the user (ie: deciding on a download!) I'd like to stick with 32-bit until we have a proven need for 64-bit on Windows.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

32 bit is fine, I was just wondering whether mingw64 implied 64 bit binaries.

@phlash
Copy link
Contributor Author

phlash commented Dec 24, 2017

@gvanem C-89 compilable code in win32-fetch.c (aka, variables at the top :))
@csete ZIP archive build target 'dist', works for me when unpacked into vanilla Win7 VM.

src/tle-update.c Outdated
@@ -34,6 +34,9 @@
#ifdef HAVE_CURL
#include <curl/curl.h>
#endif
#ifdef USE_WIN32_FETCH
#include "win32-fetch.h"
#endif
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for being a pain in the butt, but another comment.
The use of lines such as:

#ifdef HAVE_CURL
..
#endif
#ifdef USE_WIN32_FETCH
....
#endif

could indicate that both combination would be allowed. IMHO, it would better be:

#if defined(_WIN32)
...
#elif defined(HAVE_CURL)
...
#endif

throughout the code. I mean, what would be the benefit of not using WinInet functions on Windows?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I added HAVE_CURL in the first place so I could omit curl calls while sorting other dependencies out. That temporary fix can now be removed so we end up with:

#ifdef WIN32
    <win32 fetch code>
#else
    <curl code>
#endif

@dl9sec
Copy link

dl9sec commented Dec 26, 2017

Hi,
any hints how to build the win32 port from the sources or maybe where to get a binary?
Tnx.
73, Thorsten

@gvanem
Copy link
Contributor

gvanem commented Dec 26, 2017

any hints how to build the win32 port from the sources

I built it with this home-brewed makefile. You're welcome to try it.

@phlash
Copy link
Contributor Author

phlash commented Dec 26, 2017

@dl9sec assuming you would be starting with my fork of the source, there are instructions on obtaining and unpacking the dependencies (goocanvas2, gtk+3) in the win32/README & config.mk files. I have currently adapted the existing GNU Makefile for cross-compiling on Linux, if you are building on Windows, then you may be better off starting with @gvanem home-brew makefile as above. I've popped an unsupported current binary ZIP on my website here: https://ashbysoft.com/download/gpredict-2.1.93.zip, unzip and run...

@phlash phlash changed the title First win32 build of 2.x candidate Win32 build of 2.x Dec 26, 2017
@csete
Copy link
Owner

csete commented Dec 27, 2017

I'm glad to see that this has evolved into actual binaries :-)
I hope to finish Gtk+ 3 migration in a few days and then I can give this the priority it deserves.

@dl9sec
Copy link

dl9sec commented Dec 27, 2017

Guys, thanks a million for your efforts. Great stuff!
The zip ist "Deluxe"...

@phlash
Copy link
Contributor Author

phlash commented Dec 28, 2017

Updated Win32 binary ZIP: https://ashbysoft.com/download/gpredict-2.1.138.zip

@csete
Copy link
Owner

csete commented Dec 28, 2017

Thanks @phlash, I gave your binary a try and noted a few things:

  • The DLLs in thew package seem to be from 2013. Are they the latest available ones?
  • As @dl9sec also pointed out to me, the application crashes if one tries to resize the window.
  • Update TLE from files also leads to crash because some GIO-something is missing... In fact, there seem to be no Gtk/Glib related data files in the ZIP
  • I'm not sure the default theme is all fine. The fonts do not look well on the laptop I tried.

@csete
Copy link
Owner

csete commented Dec 28, 2017

Anyway, I will look into merging soon. The above issues can be sorted out along the way.

@phlash
Copy link
Contributor Author

phlash commented Dec 28, 2017

Thanks for the feedback @csete, here's what I've been working through so far:

  • DLLs are latest GNOME pre-built ones I could find, didn't want to force everyone to build from source for all of GTK etc. or depend on non-GNOME supply chain. This is always a pain with Win32 targets as there is no sane build-dep mechanism...
  • Resize crash I've now reproduced, may be related to specific GTK version? Difficult to debug (see below)
  • Crash when updating TLE from files: missing dialog schema files (fixed, see below).

The GTK binary package I used contains a README with details of how to install, that I have not applied to the ZIP (I think it writes target-specific paths to config files). I have found a reasonable description of bundling GTK+3 with an application: http://www.tarnyko.net/repo/gtk3_build_system/tutorial/gtk3_tutorial.htm

Following information from packaging tutorial seems to fix TLE load from file / other dialogs, and enables display of SVG logo.

Attempting to debug the crash on resize by setting GTK_DEBUG=all, slows gpredict to a crawl and prevents the crash. This indicates a Heisenbug / timing issue, likely to need a proper Win32 debugger.

@csete csete merged commit 4b8750f into csete:master Dec 29, 2017
@csete
Copy link
Owner

csete commented Dec 29, 2017

Hi Phil,

I have now merged the changes.

The fact that only old binaries are available from the project is worrying. It indicates that they either don't care about windows or that the official distribution is now msys2. I will take a look at building on windows using msys2/mingw,

In any case, we will need newer libraries.

@csete csete added this to the v2.2 milestone Dec 29, 2017
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.

None yet

4 participants