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

Package system on libgit2 #11196

Merged
merged 90 commits into from
Sep 29, 2015
Merged

Package system on libgit2 #11196

merged 90 commits into from
Sep 29, 2015

Conversation

wildart
Copy link
Member

@wildart wildart commented May 8, 2015

Package system switch to libgit2 promises faster and resource efficient package management.

This work originated in #11143.

Pending:

@wildart wildart mentioned this pull request May 8, 2015
@tkelman tkelman added the packages Package management and loading label May 8, 2015
@IainNZ
Copy link
Member

IainNZ commented May 9, 2015

This is heroic stuff

@@ -0,0 +1,210 @@
module GitConst

const OBJ_ANY = Cint(-2)
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it make sense to use enums here now that they are supported?

@tkelman
Copy link
Contributor

tkelman commented May 16, 2015

You're doing a kickass job in here, btw.

@ViralBShah
Copy link
Member

This is fantastic. I wonder the likelihood of having this go into 0.4.

try
LibGit2.GitRepo(cache) # open repo, free it at the end
catch err
rethrow(err)
Copy link
Member

Choose a reason for hiding this comment

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

Does this catch block do anything?

Copy link
Member Author

Choose a reason for hiding this comment

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

Exactly. 👍

Copy link
Member

Choose a reason for hiding this comment

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

Funny answer for a question. :-p

@wildart
Copy link
Member Author

wildart commented May 16, 2015

@ViralBShah That was an idea, but it depends on 0.4 release date. At this point, there are few places left to convert. I didn't expect that libgit2 would be such a present. I count for another week to finish rewrite. After all, this thing would require some thorough testing.

@tkelman
Copy link
Contributor

tkelman commented May 16, 2015

We're still on -dev for a little while longer, but probably not too much. If you can get this fully working on all platforms in the next few weeks, then we should think about switching. I'll need a few days to do some surgery to take command-line git out of the Windows binaries (and similar but not quite as invasive for the Mac binaries), if we're going to be ambitious enough for a complete replacement.

We could possibly merge a conservative version of this for 0.4 that leaves both command-line git and libgit2 simultaneously around, and only uses libgit2 for the parts of Pkg where you feel it will work robustly.

@tkelman tkelman mentioned this pull request May 16, 2015
@wildart
Copy link
Member Author

wildart commented May 16, 2015

@tkelman I do not have access to OSX. Help appreciated.

@tkelman
Copy link
Contributor

tkelman commented May 16, 2015

Neither do I. I can help you tweak appveyor to run the pkg test when you think it's ready for that, for now it isn't building this PR since it's apparently un-mergeable, but that's probably for the best at the moment.

@tkelman
Copy link
Contributor

tkelman commented May 16, 2015

It'll probably still time out, but may as well uncomment this line

#- osx # this build is running out of time frequently now, disable it until we can make the tests it runs faster / fewer
to get this PR testing on OSX

@wildart wildart force-pushed the art/pkg-libgit2 branch 4 times, most recently from dd8f3bf to c6612f1 Compare May 18, 2015 03:58
@tkelman
Copy link
Contributor

tkelman commented May 19, 2015

There was some broken homebrew stuff on OSX Travis that I fixed, and it looks like this passes! Worth rebasing, and tweaking appveyor now you think?

Also good to add the new license header style on any newly added files

@ViralBShah
Copy link
Member

I am happy to test on OS X, and I am sure so will @StefanKarpinski

@wildart wildart force-pushed the art/pkg-libgit2 branch from da0f43d to 10129bd Compare May 19, 2015 23:30
@jakebolewski
Copy link
Member

Thanks @wildart for being persistent and addressing all our points over these past months!

jakebolewski added a commit that referenced this pull request Sep 29, 2015
@jakebolewski jakebolewski merged commit 43e7ff1 into master Sep 29, 2015
@hayd hayd deleted the art/pkg-libgit2 branch September 29, 2015 23:19
@vtjnash
Copy link
Member

vtjnash commented Sep 30, 2015

this looks truly monumental. i'm excited!

@jiahao
Copy link
Member

jiahao commented Sep 30, 2015

🍰

@StefanKarpinski
Copy link
Member

Oh sweet! This is really exciting.

@tkelman
Copy link
Contributor

tkelman commented Sep 30, 2015

Note that we'll probably have to split up any libgit2 tests that rely on internet connectivity, put them either in pkg or a new file that doesn't run by default (since distro buildbots don't have internet connectivity) but we manually add to travis and appveyor.

@wildart
Copy link
Member Author

wildart commented Sep 30, 2015

What's happened? I was thinking about how to spit Pkg before merging and ... I guess it's for the best cause this PR has become so bloated I have a hard time tracking issues.

Thanks everybody for reviewing. @tkelman You've been a great help.

@tomasaschan
Copy link
Member

Great work, everyone involved! I'm really looking forward to using this! :)

@KristofferC
Copy link
Member

tngnevc

@ScottPJones
Copy link
Contributor

@wildart How will this be split, technically? Will you put generic function definitions in Base, i.e.
function name end
and then via a "using PkgDev" add the actual methods for those (but they'd still be accessible as:
Pkg.blahblahblah?
I think that approach might be nicer than having to do PkgDev.blahblahblah, and remember which things are in Base, and which are extensions.

@wildart
Copy link
Member Author

wildart commented Sep 30, 2015

Definitely PkgDev, no monkey business with Base.

@ScottPJones
Copy link
Contributor

Is it really monkey business to put generic definitions (which were added late in 0.4) for all the functions?
It seems like a nice technique that could be used in a lot of places where "advanced" stuff is planned to be moved out of Base in the future.

@jakebolewski
Copy link
Member

You can import Base.Pkg into PkgDev and then re-export the methods, that way PkgDev becomes a strict superset in functionality (no cognitive burden required).

@ScottPJones
Copy link
Contributor

But then you still have both Pkg and PkgDev, and only some methods are available in Pkg.
That seems like a bother to me.

@wildart
Copy link
Member Author

wildart commented Sep 30, 2015

The whole idea of PkgDev is separating a development functionality from Pkg which should target user experience only. No harm in typing 3 more letters.

@hayd
Copy link
Member

hayd commented Sep 30, 2015

Also #5155.

@tkelman
Copy link
Contributor

tkelman commented Nov 12, 2015

@wildart did you ever get a chance to structure the Git REPL mode as a package? A few people have expressed interest in using it, would be cool to have available to try out somewhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
packages Package management and loading
Projects
None yet
Development

Successfully merging this pull request may close these issues.