kpeeters / cadabra2

A field-theory motivated approach to computer algebra.
https://cadabra.science/
GNU General Public License v3.0
230 stars 37 forks source link

Windows build functionality #52

Closed bluelips closed 5 years ago

bluelips commented 7 years ago

Support for windows builds using microsoft visual studio 2017 community.

kpeeters commented 7 years ago

Thanks for your efforts on this! I have very little Windows experience; do you think this route is better than the MSYS2 one I have outlined in the README? Are you planning to make the GUI frontend build as well?

bluelips commented 7 years ago

I tried the MSYS2 route but ran into problems. With cmake both should be available and supporting msvc2017 shouldn't preclude msys2 if that's what you prefer. From my experience msvc is actually pretty good and widely used among people targeting windows. As it's now freely available there doesn't seem to be a big downside (aside from the usual closed source aspects inherent to windows).

Looking into the gui now. Cheers!

kpeeters commented 6 years ago

I appreciate all the work that you are doing, but in order to merge any of this, you really need to write down a bit about why you do things this way. There are a lot of things which look like they should not be necessary (like including bits of gtk inside the cadabra source tree). If you really think you can't do without that, you'll need to convince me that this is the only way.

So I'm not meaning to be negative, just like to understand why you do things the way you did them.

kpeeters commented 6 years ago

Once more: please don't just dump pull requests. I have someone here working on the windows port as well, and it seems like a total waste to have two people do things in parallel, but I can only merge your changes if you explain why you want certain things changed (especially when you start changing core code).

If you don't do that, I won't be able to merge any of your changes, and you're doing it only for yourself. That would really be a pity.

bluelips commented 6 years ago

Oops, I didn't realize this workflow was "dumping pull requests", sorry about that. I didn't realize you had the system setup to get emails every time I check in to a branch in my fork, so was simply doing updates. I haven't initiated any pull requests except the first time. I am setting up travis ci to run through a different branch, so I can track down why it's failing on travis but working in my linux vm and the windows build.

In regards to including the Adwaita gtk theme, it isn't included in the windows gtk build that I managed to get running. GTK through msvc has been quite a hassle and is seemingly unsupported outside msys/mingw. That's fine if the rest of one's codebase is also in msys, but they aren't compatible through c++ libraries as various library conventions are different. One of my interests in cadabra was to tie it in with another project that already has substantial windows dependencies. From looking at it, there were only a few files referenced from the Adwaita theme, so I may just try to reference the hicolor theme instead for the windows build.

If you have someone else doing equivalent work, then cheers and best of luck! If a merge doesn't work out in the future, it's not the end of the world although it would be nice. The requisite build process is tedious though, so I had hoped to shift things more to the windows way, which is focused on everything being self contained. The tradeoff in general is more disk space usage but less dependency hell. Also there is much more acceptance of binary distributions of libraries which means one doesn't need to deal with the entire dependency tree, just the leaf of interest.

In terms of the few changes to the core system, they have been bug fixes that restored existing notebooks to functionality, or simple portability fixes. Like, the usual convention is that assert calls are compiled out of release builds, and thus should never contain necessary state modification. However, the can_apply class of functions in certain cases seems to set necessary state, like the tb variable of the young_project_tensor, so the release build fails while the debug build works.\

I have tried to include documentation of each change in the changelist as well as keeping a windows readme up to date, but if you have other questions, feel free to ask. Cheers!

bluelips commented 6 years ago

Hey! This should be a reasonably close pass at a functional build. I will be testing the binaries on other machines but in the meantime, do you have any concerns for the merge? Thanks and looking forward to getting re-synced!

kpeeters commented 6 years ago

We are getting pretty close here too, but following a procedure which should be a lot easier to duplicate for other people (I am frankly amazed that you had the stamina to follow all this through; compared to the single line of 'sudo apt install ...' on Debian this is just a total nightmare). But I suspect that you solved a number of problems which we are still struggling with.

So I think that what will happen is that we will cherry pick from your branch, instead of doing a flat-out merge, as there are simply too many changes for me to see how they influence things on the Linux side, and I want to know why things are done in a certain way, not just that they work.

I'll be in touch later this week because I need to discuss this with my student who is working on this.

kpeeters commented 6 years ago

What is the reason why you are not using Glib::get_user_config_dir() on windows? I have used this before and it seemed to work fine?

bluelips commented 6 years ago

Just doing cherry picks is unfortunate. It would make any merging or contributions down the line prohibitively difficult. If the explanation and knowledge transfer is the holdup, I'm happy to answer questions or add documentation as needed.

You are correct this ended up being a bunch of work :) The big motivation over the msys approach is to have a build environment that works in the windows too, outside of msys. Building with msys precludes direct incorporation into other projects which are already dependent on visual studio for other reasons. I still think a windows msys version is excellent though, so if your student has questions or problems, I would even be willing to help get that version moving forward if we could merge these changes as well. It is certainly more amenable to testing and continuous integration. While I haven't used it personally, it looks like there is an alternative to travis at tea-ci.org that supports msys.

One of the big hangups has been the gtk/glib dependency, which is fairly unsupported in the windows toolchain. That's also the reason I wanted an alternative to Glib::get_user_config_dir() in the cadabra server. That way, the next person coming along doesn't have to pull in an enormous dependency in order to get the base server going. I can look at refactoring that a bit to make it cleaner and more explicit.

The other big change that I would probably see as objectionable from your perspective is the restructuring of the cmake dependencies. This was just a failing on my part to get sub-directory based cmake projects to link correctly in msvc. It was a common problem reported on stackoverflow though, so I think it may be a cmake flaw in part. Consequently, I needed to directly include much of the source in child projects such as the client. However, there did seem to be a precedent like for example the cadabra_server target explicitly includes source from core despite already having the core project as a dependency. If those changes are too objectionable, there might be another workaround with more effort. I also think substantial cmake refactoring is possible as there is quite a bit of redundancy and some odd version mismatches in boost and python support.

Other than that, I'm happy to explore possible changes that would improve your understanding. I get the sense this is far and away the biggest pull request for this project and it can look quite intimidating to merge, especially when you have previously been carrying the load alone and probably have the complete mental model of the project already in your head.

Regardless, there is now a working windows build that I was able to successfully test on a non-development machine!

Also, to get a taste of where I had envisioned going next, and perhaps explain a bit of the motivation, I invite you to look at https://github.com/bluelips/fourd which is a virtual reality environment supporting 4,1 (4 space, 1 time) dimensions. I had hoped to include cadabra inside the vr environment, particularly as a driver for more arbitrary higher dimensional visualization. The premise is that we manage to visualize three spacial dimensions on a two dimensional screen, so with a four dimensional viewer (as VR gives intuitive depth perception), to visualize four spacial dimensions. It actually works out! I've had kids as young as 12 successfully navigate a 4,1 environment with immediate intuition.

I should also explicitly say thanks! Cadabra is pretty excellent and your effort on it is impressive! Cheers! :)

kpeeters commented 6 years ago

Let me start by saying that I greatly appreciate your efforts; the comment about cherrypicking was in no way meant to downplay your contribution. The main thing I am not happy with is that I cannot see anyone but the most persistent users like you repeat this procedure. If the goal of using MSVC is to get more developers on board, it needs to be much, much simpler. The route we're taking here, therefore, is to base things on the 'vcpkg' package repository, which contains all the required dependencies already prepared for MSVC. But that means that, while many of your changes will be needed (I have started to put some into the master branch), I cannot possibly do a merge (I don't see how that would save me time; I'd end up taking out a lot of things by hand afterwards again, and would have a worse understanding of the 'why' of your changes too). Hope this makes some sense.

I'll get in touch about fourd by email if you don't mind.

kpeeters commented 6 years ago

In case you didn't spot it, Windows is now officially supported on the master branch, and there is a binary installer available from the Cadabra download page. We have taken some inspiration from part of your pull requests, but use VCPKG as much as possible to avoid having to build any of the dependencies ourselves.