-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Windows build #1327
Windows build #1327
Conversation
…nto windows-build Resolving conflicts: # src/CMakeLists.txt # src/common/collection.c # src/dtview/main.c # src/external/rawspeed/CMakeLists.txt # src/external/rawspeed/rawspeed-identify.cpp
…nto windows-build
It's hard to keep disscussion constructive when the questions are being ignored :)
Since you will ignore our words anyway, i'm gonna go forward right now and state the same as with, parta: you must specify really clearly that that is not darktable and is not related to the darktable in any way, and no one should report anything about it's brokenness to us. Let's try this again. Now, tell us: how exactly does this not go exactly against the points raised there, |
I have to agree with Roman. The Windows port is long story and we do not want only a port but a group of highly motivated people around to fix issues specific to this platform. This is the third, fourth, fifth... I don't count anymore... Windows port from different people. Let's try to create a group, come discuss on IRC, stay around... |
To be even clearer, we do not want a build today and nothing tomorrow. This will be very bad for darktable reputation. Do you see all those users left in the void with an old buggy version? |
Dear Roman and Pascal, this is my first contribution to the darktable community, so I don't have any history of ignoring words. I clearly see the risks you mentioned, and I trust your experience. I was thinking hard before submitting this PR (You saw the first sentence...): Nobody knows how many bugs will surface. Nobody knows how many people would contribute and maintain this part of the code. I don't have a team and I can only speak for myself. As you have seen in the last two months I have spent quite some time to understand the code and the software, and put a lot of energy to build a foundation and reach this state. I would happy to do that in the future as well, as it was fun and rewarding. It's a collaborative effort, and we need to start somewhere. I believe all software (I guess also darktable) started small with a lot of bugs, and with a few people to participate. But with the time bugs are getting less and more people are joining. I'm happy to continue discussing it on IRC or some other forum in a smaller group. Peter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, so i'm the one that is not against a win port, so I did actually look at the code
this is a very clean code that mainly updates the existing (limited) support that we already have and adds what's needed to actually get it to work.
it also adds a readme that would allow other people to work with that. So i'm all in favour of it
You have a few (mainly cosmetic) changes here and there that could be made into a separate patch
So overall, if there was not the elephant in the room of "do we want a windows port" i'd be all for including that patch
Now, I can't ignore that elephant, and i'm not sure what's the proper next step is... here are a couple of idea. Note that this is not anything official, just my personal thought on it.
- maintain this patch serie as a branch, keep it up to date, rebased on master on a regular basis
- DO NOT release any binary builds for windows. That might not release the torrent of bugs that the other devs fear, but it will certainly bring you a huge amount of bad karma.
- do not post your port on the ML before asking and getting a positive answer from the core devs. They'll probably say no for a couple of release and until you are better known and trusted, but that's a necessary step.
- Join IRC and fix bugs, offer new features, become a known devs.That was the first step in the mail stated above, and for a very good reason
- Just drop any idea of getting this merged right now. You'll anger everybody and won't get anything constructive. Maintaining your unofficial branch is safer
I can't really offer you any long term advice except "become known and trusted and then we'll see" There is no predefined path for you, and as the mail stated, there is a real long-time commitement we need to be sure of.
Again, i'd really like to see a windows port, but we need a consensus within the core devs that whatever port is proposed is a long term commitment, and we can only juge that... from a long term commitement
packaging/windows/BUILD.txt
Outdated
|
||
For gphoto2: | ||
$ pacman -S mingw-w64-x86_64-{libusb,popt,libgphoto2} | ||
Check that CAMLIBS and IOLIBS for LIBGPHOTO are available |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how do you do that ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can check it by "echo $CAMLIBS". I made a note here as normal MSYS2 packages do not like to add env variables, but libgphoto2 needs that. So for the current mingw-w64-x86_64-libgphoto2 I have included a script which adds this to the MSYS2 environment, but this comment is more of a reminder for later that the installer needs to take care of this aspect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then add an export
of those to the commands given later.
src/CMakeLists.txt
Outdated
SET(CMAKE_SHARED_LIBRARY_LINK_CXX_FLAGS "-Wl,--no-undefined -static-libgcc -Wl,-O1 -Wl,--as-needed -Wl,--sort-common -s") | ||
#removed the -s flag, as it has stripped the debug symbols even from debug builds | ||
SET(CMAKE_SHARED_LIBRARY_LINK_C_FLAGS "-Wl,--no-undefined -static-libgcc -Wl,-O1 -Wl,--as-needed -Wl,--sort-common") | ||
SET(CMAKE_SHARED_LIBRARY_LINK_CXX_FLAGS "-Wl,--no-undefined -static-libgcc -Wl,-O1 -Wl,--as-needed -Wl,--sort-common") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if this is a generic problem, you might want to make a separate PR for those
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I see these flags were used only when it was built for WIN32. The only change I made is to remove the -s flag. I guess nobody noticed that there are no debug symbols are available in the debug version, maybe there was no debug done on those early cross-compiling versions at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, stripping should be avoided, especially in these early stages where debugging crashes will be important.
src/common/darktable.c
Outdated
} while(c != '\n' && c != EOF); | ||
fputc(c, fout); | ||
if(c == '\n') {break;} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the problem with this code, win miss fseek ? is that something that is generic and is needed for other arch ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fseek is there, just the original code went to an infinite loop, as the condition
if(c != EOF)
was never true.
This particular code as I see is creating your default keyboardrc file if you don't have one from a template, but due to never reaching EOF condition, it has created 90MB+ size keyboardrc file until I killed the process :)
I'm not an expert, but in the fseek documentation I see that " A successful call to the fseek() function clears the end-of-file indicator for the stream" For that reason I have decided not to use here the fseek().
The updated code above was safely creating for me the keyboardrc file from the template, and I have tested that functionalty also on Ubuntu and worked well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clears the end-of-file indicator for the stream
Can't you cache it before calling fseek()
and check the cached value for EOF
after calling fseek()
?
Did not read the code though.
Hi @boucman , thanks for the reply. Actually it makes a lot of sense: I'm happy to keep it on a separate branch, which we can merge/rebase from time to time. Also you have a point with the binary release, anyhow it will be a longer shot to do it properly. |
Just want to point out that from our point of view the code changes to build on windows, the windows port and "darktable" for windows is the same thing. And we have already stated our view on the latter. It may be better to go help out some other free software project that already had the misfortune of working on that platform, from what i gather gimp is seriously lacking help and they are much more friendly and welcoming :) Otherwise, the 'last' part of the mail explained the way quite clearly. |
I didn't look at the changes in this PR (yet) but I would like to state some general things, all of which are my personal view, not representative of the darktable team and I might change my mind anytime I feel like it. So here we go:
So as my personal conclusion barring a look at the actual changes is that a joint effort of the two would make me more or less happy with the prerequisites for approving a Windows port. Even if it's just something handed to a few people we know at the beginning as a closed test phase to see what bugs surface and how our then-windows-maintainers handle them. |
Right, now that i have actually checked, in #1327 (comment) "It's hard to keep disscussion constructive when the questions are being ignored :)" was said because i just assumed you were the same person who started last thread about this https://www.mail-archive.com/darktable-dev@lists.darktable.org/msg01242.html But as others said, the mail more or less summed up the steps (at the end of the mail), not sure there is anything for me to add right now. |
data/CMakeLists.txt
Outdated
@@ -186,6 +186,12 @@ if(USE_XMLLINT) | |||
add_dependencies(darktablerc_file validate_darktableconfig_xml) | |||
endif(USE_XMLLINT) | |||
|
|||
if (WIN32 AND EXISTS ${CMAKE_CURRENT_BINARY_DIR}/darktablerc) | |||
#make sure line endings are UNIX like even on Windows | |||
configure_file(${CMAKE_CURRENT_BINARY_DIR}/darktablerc ${CMAKE_CURRENT_BINARY_DIR}/darktablerc.tmp NEWLINE_STYLE LF) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be surprized to learn that you can not pass that to normal configure_file()
call.
I.e. this should be done when configuring ${CMAKE_CURRENT_BINARY_DIR}/darktablerc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is that darktablerc is a result of an xsltproc custom command.
add_custom_command(
DEPENDS ${CMAKE_CURRENT_SOURCE_DIR}/../tools/generate_darktablerc.xsl ${CMAKE_CURRENT_BINARY_DIR}/darktableconfig.dtd ${CMAKE_CURRENT_BINARY_DIR}/darktableconfig.xml
OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/darktablerc
COMMAND ${Xsltproc_BIN} ${CMAKE_CURRENT_SOURCE_DIR}/../tools/generate_darktablerc.xsl ${CMAKE_CURRENT_BINARY_DIR}/darktableconfig.xml > ${CMAKE_CURRENT_BINARY_DIR}/darktablerc
)
I tried to change the xsl file to make sure darktablerc have LF only at the line ends but did not find a good solution, and ended always with CRLF line ends. Any suggestion would be helpful here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugh, ok.
But even then, it creates kinda-circular dependency chain for ${CMAKE_CURRENT_BINARY_DIR}/darktablerc
.
I really doubt having 2 different targets providing the same output file is correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a cmake guru, so I might be wrong, but the added section is not interfering with the darktablerc output or creating circular reference.
For the configure_file there is the parameter called COPYONLY, which according to the documentation, "Copy the file without replacing any variable references or other content". So I beleive thats the reason that we have only one target (the original), the added configure_file is not defining any additional target IMHO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer fixing the parsing of the file to ignore the line endings. People might edit it in an editor later and then all bets are off anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better fix parsing the rc file to accept both line endings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have followed your suggestions:
- reverted back the CMakeLists.txt to the original
- updated dakrtablerc habdling. Actually it was much simpler than I thought: opening with "r" instead of "rb" fif the trick. Also saving darktablerc has been updated from "wb" to "w". On linux there s no difference, but on Windows this was the trick.
packaging/windows/BUILD.txt
Outdated
@@ -0,0 +1,106 @@ | |||
How to make a darktable windows installer (64 bit Intel only): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Intel? So no amd?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually no intel specific there, should work both, will update.
src/common/opencl.c
Outdated
@@ -1631,7 +1645,15 @@ int dt_opencl_build_program(const int dev, const int prog, const char *binname, | |||
char dup[PATH_MAX] = { 0 }; | |||
g_strlcpy(dup, binname, sizeof(dup)); | |||
char *bname = basename(dup); | |||
#if defined(_WIN32) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spaces before #if
.
You really should follow https://github.com/darktable-org/darktable/blob/master/CONTRIBUTING.md, in particular https://github.com/darktable-org/darktable/blob/master/CONTRIBUTING.md#coding-style
Just set up clang-format hook and never worry about it again.
src/iop/ashift_lsd.c
Outdated
@@ -1914,7 +1914,7 @@ static int reduce_region_radius( struct point * reg, int * reg_size, | |||
image_char used, image_double angles, | |||
double density_th ) | |||
{ | |||
double density,rad1,rad2,rad,xc,yc; | |||
double density,radius1,radius2,rad,xc,yc; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this about?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rad1 and rad2 are unfortunately already defined numeric constants in dlgs.h - this change is basically a naming collision fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, Windows has some ugly global things.
src/win/getdelim.c
Outdated
getline (char **lineptr, size_t *n, FILE *stream) | ||
{ | ||
return getdelim (lineptr, n, '\n', stream); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Newline, modelines
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have now installed clang, and I'll make a change to getdelim.c and getdelim.h to see that the format is OK (will also add header comments). If yes, I'll update the other files I have modified.
src/win/getdelim.h
Outdated
@@ -0,0 +1,7 @@ | |||
#ifndef __GETDELIM_H__ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
header, Newline, modelines
src/win/getrusage.c
Outdated
|
||
// modelines: These editor modelines have been set for all relevant files by tools/update_modelines.sh | ||
// vim: shiftwidth=2 expandtab tabstop=2 cindent | ||
// kate: tab-indents: off; indent-width 2; replace-tabs on; indent-mode cstyle; remove-trailing-spaces modified; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
modelines
src/win/rlimit.c
Outdated
@@ -0,0 +1,211 @@ | |||
////////////////////////////////////////////////////////////// | |||
// | |||
// Sample implementation of getrlimit() and setrlimit() for Win32. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
header
src/win/rlimit.c
Outdated
dwWritten = _write( handle, buffer, count ); | ||
} | ||
return dwWritten; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Newline, modelines
Where can we see the code diff? :) |
The main difference in this versions are compilation flags (native or generic) for CPU optimisations and Multibyte code character sets (see code snippet in my post above). |
As i hope you understand, that did not answer my question. |
You answer for this one? |
Thanks for the offer, but I am not really interested in using darktable on Windows but getting this PR merged. And in order to do that I need to find the problems and help figuring out where they come from and how to fix them. |
I'll have more time later today, I'll check on running DT on VMs. Surely I have a Win 10 VM somewhere (but so far have not tried running DT on that), and will check on a Windows 7 VM, but will take more time (as I don't have it at hand)., My gut feeling it has to do something with OpenCL, but one never knows. |
@houz : also a short update on an earlier suggestion of you: have been testing the |
That if you C++-ify the code, and add a unittest; which fails without, and passes with the patch, i'd consider merging it. PS: once there is 'official' win build, everyone who wants to help with that (currently by providing their own build), ideally, should cooperate. In other words, 3rd party builds would be, ... unwanted?. |
@peterbud that's good news. Having some special cases hard coded in the cmake files to be installed is fine. I am mostly worried about constantly chasing after specific DLL names, scripts breaking with every update of msys, … |
@houz: On Windows 10 VM (Hyper-V) darktable just installs fine, and starts properly. |
If you'd appreciate more testing on real Windows, I offer one machine with Win 10 and one with Win 7 64 bit (both Intel Core i7). For sure I can test pre-built binaries. If it'd help a lot, I can also try to prepare the build environment on these machines. |
@houz : I have managed to install |
@houz : now I have finished the first pass with cmake
Do you have any suggestion for these? So far I can keep them on the manual list as before which I have compiled by hand using package dependency tree and package content list. |
This means darktablerc is always using Unix style line endings, but also makes possible proting darktablerc files between platforms
These libraries you mention are not plugins, and therefore aren't needed in the package. You can take a look at https://github.com/darktable-org/darktable/blob/master/packaging/macosx/make-app-bundle and https://github.com/darktable-org/darktable/blob/master/packaging/macosx/darktable.bundle as the reference - gtk-mac-bundler does something very similar to BundleUtilities and you can see which extra files I have to add to Mac bundle. |
They are not dt plugins, but still needed at runtime by things we link against. Maybe we can use some wildcards and glob from cmake to get the files we want? That would at least make our scripts somewhat robust against system updates. |
Hmm, and what doesn't work without these files? Because I might have to adjust Mac packaging too then. |
By plugins I meant for example gphoto drivers, those are separate dynamic libraries and indeed are needed. |
Removing DTDependencies component
The installer works on the Windows 10 machine I built in on, but not on my Windows 7 machine. (Rephrase: the installer works on Windows 7 but not DT itself.) @Lirein 's generic build works on my Windows 10 machine as well (the i7 build didn't work). Oh, and that red circle: there seems to be a bug in the rounded corner for that side - it is mostly rounded, but has a ~2px square jutting out from the corner. His generic build also works on my Windows 7 machine. Maybe it has to do with compilation flags for CPU optimization... |
I just tried your latest changes, however, when trying to build the installer package it fails with an error about the filename of one file being too long. The interesting bit being that it's something not needed at all. a regular
|
Strange, I have not had similar problem, but I don't have Visual Studio installed on my machine :)
It looks like it can be forced to use |
This will prevent using VS dumpbin tool when Visual Studio is also installed Keep in minfd that objdump is SLOW Also removing unnecessary install step in CI
@houz is there anything needed from my side to merge the remaining changes and close this PR? |
I'll have access to the Windows 8/10 laptop this weekend for further tests. If that doesn't show major issues I will merge then. |
After some more tests I think this is good to go. I have some local fixes ontop of this, and some more issues to tackle afterwards, but keeping these few changes left over in a PR won't help anyone. |
First of all I know the question of a Windows build has been controversial, and generated a lot of emotion in the past. All I'm asking is to look into this PR in an objective way, setting emotions aside.
In the last 2 months I have been working on creating a decent Windows port of darktable. I believe this software is brilliant, and would deserve to reach a broader (but less fortunate) audience who are still only familiar with Windows.
For the port I have used the MSYS2 Mingw64 environment, which enabled to use the existing build system without any major change. The resulting code is native 64-bit, no dependency on propriatery libraries, only what comes with Windows.
To see the whole effort, please note that first I had to port to Windows the following libraries (as no ports were available on MSYS2 or on Windows at all):
The other libraries have been already ported and were available.
Also I have realized that there is no decent GTK3 binary distribution so I have also contributed to a project which now can deliver a solid binary distribution for GTK3
Along the way I have added all the necessary Windows specific code, where I have tried to make as few surgical changes as possible, and not interfering with the existing code. During the port I had to add a few functions which were missing from MSCRT. I have tested the current PR is still building properly on Ubuntu.
I have documented the necessary steps to prepare the Windows build in the /packaging/windows/BUILD.txt file in detail.
The current version is a fully functional darktable which runs nicely on Windows, supports all basic lighttable and darkroom functionality + OpenCL, lensfun, tethering, slideshow and map.
What is missing is the printing and color management functionality, that is more complex and requires more time.
Finally I'm planning to create a decent installer package using cmake and NSIS later, as users are expecting a binary distribution format.
Any constructive feedback is welcome.