WIP: andrioni's Libgit2 work

[StefanKarpinski]

I'm making this into a pull request for easier discussion and so that I can check it out more easily.

Edit: Hijacking to make a task list.

Todo list

• Check why Travis is failing to build, even though libgit2.so apparently is generated (lines 1303 and 1304).
• Fix the Makefile to handle both sonames.
• Refactor to use immutable types
• Ensure it builds on Windows with MinGW
• Update jakebolewski/LibGit2.jl#9 with the latest changes to LibGit2.jl
• Fix Base.add! undefined variable error
• Fix -stdlib=libc++ -mmacosx-version-min=10.7 flags getting added to CC on Mac, which Cmake doesn't like
• Make sure moving flags from CC to JCFLAGS works everywhere it needs to

[jakebolewski]

@andrioni have you pushed all your changes into your LibGit2.jl fork?

[StefanKarpinski]

I can confirm that this works for me so far.

[jakebolewski]

Are we going to use CMake or write a custom Makefile for libgit?

[tkelman]

0.21.0 was released today.

I was scared this would be a nightmare on Windows until I built the package last night. Still needs a lot of work on tests, but building might not be too bad. For Windows we want -G"MSYS Makefiles" in the cmake call.

Please make the path to cmake a Make.user configurable item. I'm going to want to specify it via a full path, I used the separate installer from the cmake web site rather than through pacman in MSYS2.

[andrioni]

I think there might be some changes yet to push, and the code is definitely still dirty and WIP-y, but it does include most changes. I haven't integrated @jakebolewski's test suite yet too.

[jakebolewski]

@tkelman libgit2sharp provides the Git integration for visual studio so a lot of work has gone into making it work well on Windows.

[andrioni]

I do have to update it to 0.21 too (which was released today), but libgit2's test suite should pass, except for one issue on OS X due to UTF-8 filename normalization.

[andrioni]

Oops, now the branch is up-to-date.

[tkelman]

@jakebolewski - yes, for Visual Studio. There's an enormous divide between Visual Studio and MinGW, and you can never quite tell whether MinGW is a priority after a project has gone with cmake.

[jakebolewski]

@andrioni unfortunately the way this is written will not work cross platform. I manully padded out the structs to get the ball rolling but this will fail on 32 bit systems. @tkelman also has said that most of the tests do not work on Windows as well.

I also think we should drop the api module, it would be much cleaner and it didn't save much code duplication in the end.

[andrioni]

Is there a better way to deal with this other than handcoding the padding, especially since padding in C structs is (IIRC) implementation-defined?

[vtjnash]

the following padding is actually part of git_index_time (on a 64-bit system)

you shouldn't try to manually expand recursive structs, just make immutable types containing only other bitstypes and Julia should work out the padding correctly to the C-API

[andrioni]

I've just pushed more commits. It is now 0.21-based and passes the pkg test suite, but some operations are still only partly-libgit2 based, mainly in entry.jl.

[tkelman]

Will need to support cross-compile, for the sake of building the Windows nightlies. Unless we expect to just use a set of libgit binaries for everything, though we're not doing that for any other base deps (should we?) so it might be a little odd to do it here.

It's only called git2.dll if built with Visual Studio. For the sake of consistency and cross-compilation I think we should focus on building with MinGW (where it's libgit2.dll) for now.

[andrioni]

@tkelman, you mean in https://github.com/JuliaLang/julia/blob/libgit2/base/libgit2/api.jl#L6-L8?

[tkelman]

Yep. I imagine a lot of that code will get shuffled around as the code gets further adapted from package into base module.

[andrioni]

Oh well, it builds libgit2.so.0.21.0 on Linux, and libgit2.0.21.0.dylib on OS X.

[tkelman]

Ah, I see @StefanKarpinski is making a to-do list up top, good idea. Also this should be cross-referenced, @quinnj is making headway on addressing the test failures over in https://github.com/jakebolewski/LibGit2.jl/pull/10

[StefanKarpinski]

Credit where it is due: I think the todo list is @andrioni's, not mine. I just created the pull request. I've been largely MIA this week since I'm at a lake house in upstate New York. There's wifi, but it's hard not to spend the entire day on the lake given the gorgeous weather :-)

[andrioni]

Later today (probably after the game) I'll send to @jakebolewski the extra changes I made to LibGit2.jl to make Pkg works, mostly API updates and new functions.

[jakebolewski]

I'll also try to work through the rest of the immutable type refactor work I had started today and tomorrow.

[andrioni]

Sometimes I get a weird error while cloning a package ("Error receiving socket data: Interrupted system call"), and from what I saw half of the error message comes from libuv. Any idea on why would it happen?

[jakebolewski]

I have been working on this over the past week and once we can get the test suite working on Windows the remainder of these open issues should be able to be closed. Thanks to @tkelman for the libgit2 Windows RPM and the BinDeps script as well as @quinnj for his help sorting through Windows issues. Git merge / clone issues still need to be worked out but I feel we are over the hump with the large changes that needed to happen on the LibGit side.

[StefanKarpinski]

I want to merge this into master now that we're in the dev window for 0.4. If I merge this as is, will I break everything or can we sort it out afterwards? I don't really care that we're not using it yet, I just want to start having people on master building libgit2 as part of Julia so that we can start sorting out build issues at least.

[tkelman]

likely breakages
A) 32 bit
B) cross compile

but yes we should start making progress here one way or another

[StefanKarpinski]

I suspect that merging is a great way to force the fixing of the 32-bit problems ;-)

What's the cross compile issue?

[tkelman]

Cross compiling with cmake is an unmitigated disaster? I'm fiddling with it and am stuck at error: unrecognized command line option '-rdynamic'

[StefanKarpinski]

Ah, that. Since I don't cross-compile, I was blissfully unaware of this problem.

[tkelman]

Okay the queue wasn't too bad. Travis on osx says:

CMake Error at CMakeLists.txt:14 (PROJECT):
  The CMAKE_C_COMPILER:
    clang -stdlib=libc++ -mmacosx-version-min=10.7
  is not a full path and was not found in the PATH.
  Tell CMake where to find the compiler by setting either the environment
  variable "CC" or the CMake cache entry CMAKE_C_COMPILER to the full path to
  the compiler, or to the compiler name if it is in the PATH.

So, as I said in August, this is going to require somewhat serious OSX build system changes. -stdlib=libc++ -mmacosx-version-min=10.7 cannot be added to CC as they are now for cmake to work, they need to be put in CFLAGS and we need to be sure those CFLAGS are getting sent everywhere that either the stdlib or the min osx version are relevant. cc @staticfloat

The other alternative is pre-generating a custom set of Makefiles to avoid using cmake, maybe just on OSX where this is the biggest problem. But @Keno has done some other work on pieces of LLVM that could only be fully built with cmake, so I think getting things to work better with cmake is a better plan.

[ihnorton]

Couldn't CC be unset/reset before CMake is invoked?
(but also, setting compiler flags in the CC variable seems sort of strange...)

[staticfloat]

I think setting CC to have flags involved was just laziness. Let's just append to CFLAGS instead and see if it works.

[tkelman]

I'm about to push a commit where I move all the CC += bits to JCFLAGS instead. We'll see how much of the build that breaks.

[tkelman]

Looks like it more or less worked, the OSX build on Travis (nice, hardly any queue at all) gets to the same bootstrap failure as on Linux. Still needs to be checked that the flags are getting sent everywhere they need to be (I tweaked the to-do list up top accordingly).

[vtjnash]

It has taken quite awhile to remove all uses of CFLAGS, etc., so that system packagers (which expect to be able to override CFLAGS) work properly. Please don't break that

[vtjnash]

Also, JCFLAGS is for flags only needed for the Julia system library, whereas CC has the flags we want set everywhere

[Keno]

We might need a separate DEPSCFLAGS or something like that. I contemplated that for the libcxx cmake build, but since that was only once case it wasn't so bad.

[tkelman]

Also, JCFLAGS is for flags only needed for the Julia system library, whereas CC has the flags we want set everywhere

We can't do that anymore, not in CC. How particular do we need to be about sending different flags to dependencies vs libjulia (is that what you meant by "Julia system library")?

[tkelman]

And by necessity, this is going to break almost everything for a little while. That's the only way it'll ever actually happen. It's long overdue, let's just stumble our way through it.

[tkelman]

Hot damn, just commenting out the broken parts gave a green build on Travis (and for me locally). That's not too meaningful, but it's a start. Should we merge and pick up the pieces as we go then?

Dibs on killgitwithfire as a branch name for when we can take the bundled git out of the windows binaries.

Also writing down so I don't forget this, an interesting thing to note is libgit2 bundles copies of zlib and http-parser, possibly the same version as we use in the package? We may be able to just use those dependencies from libgit in those packages once this is up and running.

[jakebolewski]

Can we separate out building libgit2 in base with switching Pkg to use libgit2? This branch uses a version of LibGit2.jl from May and much of the library has been completely rewritten to address some of the problems mentioned in this thread. These changes were never incorporated back into this branch. Once we have libgit2 the library being reliably built as a dependency (with some trivial checks for correctness) we can start to rewrite Pkg.

LibGit2.jl is completely broken at the moment on 0.4. Tracking master has been a losing battle for awhile so I kind of gave up. Since it seems like no massively breaking changes are on the immediate horizon now seems like a good time to fix those problems and work on this again.

[tkelman]

@jakebolewski that's about what I thought. You know the most about what needs to be done to get the libgit julia bindings up to date and tested, then we can use them for Pkg.

I do think this should build the library dependency everywhere now, with the caveat that the rearrangement of the makefile flags might not be perfectly kosher yet.

[StefanKarpinski]

This seems like a good plan. Would one of you be willing to take a crack at separating the build part out into a new PR? (I'm still traveling although I'm getting there...)

[vtjnash]

as I said, you're potentially messing up the build for packagers and cross compilers. please don't do that. of course, Keno already broke it over the summer with f38b485 dropping -m32 from CC. :(

And by necessity, this is going to break almost everything for a little while. That's the only way it'll ever actually happen. It's long overdue, let's just stumble our way through it.

OK, but let's not break the build [some of the more esoteric and thus less tested ones, in this case] to do it. please?

[tkelman]

Would one of you be willing to take a crack at separating the build part out into a new PR?

Git is fighting against me but I can do it.

you're potentially messing up the build for packagers and cross compilers

The cross compile works (except for 32-bit Linux from 64-bit Linux because of the -m32 change from many months ago as you mentioned). Adding flags to CC is the wrong way to do it now that cmake is part of our build. What does adding flags to JCFLAGS instead of CC actually break, if anything? What should we do instead? You didn't answer my question of if JCFLAGS has a very particular purpose, why do those flags need to be separate from the flags sent to dependencies?

Packaging is a valid concern, as is the effect of moving the -stdlib and -mmacosx-version-min flags to a different variable. Packagers, please speak up.

[vtjnash]

moving them to JCFLAGS means that our dependencies no longer see these. JCFLAGS contains specific flags only relevant to the julia codebase. it would be either no-effect or harmful to send these everywhere, depending on which particular flag you are referring to.

if you want to move them out of CC, then you need to arrange for them to get into CFLAGS for all of the dependencies, but also still include any CFLAGS that were passed via the environment. this is certainly doable (we already do this for a few libraries that we needed to tweak), but is a pain to verify (it is also the reason there is a list at the top of which libraries use autotools configure, and which use custom setups, since you can do the same thing for all of the autotools setups and just assume it'll work, whereas the custom setups you have to manually verify it is accepting and using the flags correctly, at which point, yes, it was a whole lot easier to be lazy and just tack them onto CC, since only cmake appears to care if CC actually points at a file, the rest of the configure/make systems just care if it generates code)

[tkelman]

I'll try a different approach. Maybe make new variables CC_BASE, CXX_BASE, FC_BASE etc with just the compiler names as cmake wants, then do CC CXX and FC the way we were doing them. Kludgy but maybe an easier change.

[tkelman]

Let's see how #8820 works for just adding the build dependency. None of the changes to base from here are included.

[nalimilan]

Packaging is a valid concern, as is the effect of moving the -stdlib and -mmacosx-version-min flags to a different variable. Packagers, please speak up.

Why should it affect packagers? Because of cross-compilation? For me at least, there's no cross-compilation involved when building packages.

[tkelman]

Closing. Build system parts merged in #8820, base parts to be superseded by @jakebolewski shortly

[vtjnash]

Also note that libgit2 does not support Windows XP as of 0.21.0, so if we're using any features of libgit2 that are not available in 0.20.0, then Julia will have to do the same and drop support for XP for 0.4 onwards. This is probably not the end of the world, but something to keep in mind.

with a fair bit of digging, it seems that the lack of support is probably due to there being an assert(!XP) on the following line, and not, as was previously thought, due to the usage of the Vista thread-locking primitives (which have fallbacks for XP):
https://github.com/libgit2/libgit2/blob/5692dcf181d26069d68d460fc9179c8c96fa3795/src/win32/posix_w32.c#L400

however, it seems unlikely that anyone will hit that assert in real-world usage -- did anyone actually use junction / reparse / mount points prior to vista, except perhaps for the julia build?

julia/Make.inc

Line 812 in bfa51bb

@cmd //C mklink //J $$(call mingw_to_dos,$(2)/$(1),cd $(2) &&) $$(call mingw_to_dos,$(1),)

[tkelman]

I seem to recall also seeing some unicode / locale related comments in a few PR's to libgit2 between 0.20 and 0.21. We should probably just ask the libgit2 devs via mailing list or IRC, they don't bite.

did anyone actually use junction / reparse / mount points prior to vista

Don't think so, not commonly anyway.

except perhaps for the julia build?

Has Julia ever been compiled successfully from XP? IIRC something in LLVM failed to compile or link last time I tried, but it was months ago. Maybe it worked once upon a time, but I don't think it's been possible for at least a year or so. (ref #6002)

[vtjnash]

Has Julia ever been compiled successfully from XP? IIRC something in LLVM failed to compile or link last time I tried, but it was months ago. Maybe it worked once upon a time, but I don't think it's been possible for at least a year or so. (ref #6002)

i compiled most of julia in a piecemeal fashion on XP several months ago while doing some platform testing

I saw there was a Vista-only flag in the unicode-conversion stuff, but it was conditional on a test for the windows version, so it didn't look like an issue.

I tried asking on the IRC channel awhile back, but the right people weren't online