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

[master] bump version of embedded git for windows #13494

Merged
merged 1 commit into from
Oct 9, 2015
Merged

[master] bump version of embedded git for windows #13494

merged 1 commit into from
Oct 9, 2015

Conversation

felipenoris
Copy link
Contributor

This will optimize overall performance of Pkg module on windows.
Also, somehow I´m unable to skip firewall using https insteadOf git workaround on current git version that ships with julia for windows. With this update, everything works fine and now I´m able to use julia at work.

I would recommend some more testing on this before merging. I was unable to run make binary-dist, but make win-extras worked fine, and 7z x PortableGit.7z also worked.

I also tested the current Julia rc with the new Git folder, and it runs smoothly. I did this on win7 and win10, both 64bit.

I also noticed the unpacked Git folder is smaller (old was 260MB, new is 190MB).

@felipenoris
Copy link
Contributor Author

appveyor looks bad

@hayd
Copy link
Member

hayd commented Oct 8, 2015

master uses libgit2 rather than bundled git, this would make sense as a PR to release-0.4 branch.

@felipenoris
Copy link
Contributor Author

Alright. I´ll do that. Thanks!

@felipenoris felipenoris closed this Oct 8, 2015
@felipenoris felipenoris deleted the fn/wgit branch October 8, 2015 17:26
@felipenoris
Copy link
Contributor Author

@hayd so I guess we should remove those lines on master. Is that the case?

@hayd
Copy link
Member

hayd commented Oct 8, 2015

Sorry! Maybe git was added back in as a fall-back for some unsupported methods in Pkg. I'm not sure actually. cc @wildart @tkelman

@tkelman
Copy link
Contributor

tkelman commented Oct 8, 2015

We still bundle it on master but it's mostly unused there and I plan to take it out before 0.5 gets released. I actually wouldn't mind merging this to master first just for testing's sake

@felipenoris
Copy link
Contributor Author

@tkelman I redirected this PR to release-0.4 [https://github.com//pull/13501]. I also find it useful to merge here since it can´t be removed right now. I´ll just reopen this PR to master ok?

@tkelman
Copy link
Contributor

tkelman commented Oct 8, 2015

I think it could be removed from master, but it's also less risky to test on nightlies than the release branch.

@hayd
Copy link
Member

hayd commented Oct 8, 2015

I´ll just reopen this PR to master ok?

Github may be upset you deleted and reopened, so just open another. Sorry about the confusion.

@tkelman
Copy link
Contributor

tkelman commented Oct 8, 2015

As long as you haven't force pushed to the branch after closing it should allow you to restore the branch and reopen.

@felipenoris
Copy link
Contributor Author

It turns out appveyor got a lot happier when testing on release-0.4 branch... Maybe we should just merge to the release branch on #13501 and work on removing git from master branch.

@felipenoris felipenoris restored the fn/wgit branch October 8, 2015 18:38
@felipenoris felipenoris reopened this Oct 8, 2015
@felipenoris
Copy link
Contributor Author

@hayd, don´t worry. It was useful to test appveyor on both branches.
@tkelman, this PR and #13501 are both opened. Please, feel free to merge / cancel either one. Since you´re the one that will do the cherry-picking, you should be the one to make the call.
I also noticed Makefile is a bit different comparing master and release-0.4 branches.

@tkelman
Copy link
Contributor

tkelman commented Oct 8, 2015

@twadleigh had done some testing in this direction a few weeks or months ago but I don't recall what the verdict was. I don't want to merge #13501 to the release branch until we can test it thoroughly, but it's fine to leave both open for now.

@felipenoris felipenoris changed the title bump version of embedded git for windows [master] bump version of embedded git for windows Oct 8, 2015
@twadleigh
Copy link
Contributor

My testing was super light, git-wise, I was mostly interested in seeing if the mintty that comes along with the new git would host julia nicely.

@tkelman
Copy link
Contributor

tkelman commented Oct 8, 2015

which we could do as a side benefit on release-0.4, but your other PR would still be needed on master after removing cmd line git

@tkelman tkelman added the system:windows Affects only Windows label Oct 9, 2015
tkelman added a commit that referenced this pull request Oct 9, 2015
[master] bump version of embedded git for windows
@tkelman tkelman merged commit de30806 into JuliaLang:master Oct 9, 2015
@felipenoris
Copy link
Contributor Author

@tkelman , although the tests passed, appveyor didn´t look very happy, but I don´t know if that´s normal or unrelated to this merge. Is this ok?

INFO: Committing PackageWithFailingTests generated files
INFO: Testing PackageWithFailingTests
    From worker 3:       * pkg                  Test Failed
    From worker 3:    Expression: false
    From worker 3:     Evaluated: false
ERROR: LoadError: There was an error during testing
 [inlined code] from error.jl:21
 in record at test.jl:274
while loading C:\Users\appveyor\AppData\Local\Temp\1\

Btw, appveyor looked fine on that PR to release branch.

@tkelman
Copy link
Contributor

tkelman commented Oct 9, 2015

if you scroll up, it was probably the same error as #13436

@felipenoris felipenoris deleted the fn/wgit branch October 10, 2015 23:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
system:windows Affects only Windows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants