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

🌲 Update libgit2 v0.26.0 #77

Merged
merged 5 commits into from Feb 20, 2018

Conversation

Projects
None yet
7 participants
@billybonks
Contributor

billybonks commented Jun 22, 2017

Update libgit2 to version 0.26 so that git_repository_open_ext method supports work trees 🌴 πŸŽ„ 🌳 🌲 πŸ‘¨β€βš•οΈ

New and deleted files
screen shot 2017-06-22 at 3 38 36 pm

@billybonks billybonks changed the title from 🌲 :man_health_worker: Update libgit2 v0.26.0 to 🌲 Update libgit2 v0.26.0 Jun 22, 2017

@billybonks

This comment has been minimized.

Contributor

billybonks commented Jun 22, 2017

i seem to have broken submodules :<. but tests work well

screen shot 2017-06-22 at 3 38 50 pm

@billybonks

This comment has been minimized.

Contributor

billybonks commented Jun 23, 2017

The submodule is part of the test suite, but we install using --recursive so not sure how to ignore it libgit2/libgit2@2270ca9

@billybonks

This comment has been minimized.

Contributor

billybonks commented Jun 28, 2017

@50Wliu 50Wliu added the needs-review label Jul 17, 2017

@vcanales

This comment has been minimized.

Contributor

vcanales commented Jan 2, 2018

Hi all. Any updates on this?

@billybonks

This comment has been minimized.

Contributor

billybonks commented Jan 3, 2018

still waiting for advice from the core team for #77 (comment). basically, we recursively pull submodules. but libgit2 has an empty submodule as part of their test suite, that cause submodules pull to error.

@bbugh

This comment has been minimized.

bbugh commented Jan 5, 2018

@kevinsawicki @zcbenz @gjtorikian any chance y'all can take a look at this? It's really bothersome in Atom not having worktree support. Sounds like @billybonks just needs advice on the submodule.

@gjtorikian

This comment has been minimized.

Contributor

gjtorikian commented Jan 5, 2018

πŸ‘‹ I haven't done work on this project in some time. cc @atom/core

@vcanales

This comment has been minimized.

Contributor

vcanales commented Jan 6, 2018

From what I’ve found we’re not supposed to pull dependencies like this, ie. initializing submodules that include libgit2’s tests.

Moving to something like https://github.com/libgit2/node-gitteh seems a bit over the top, so I guess waiting for advice is what we have.

@billybonks

This comment has been minimized.

Contributor

billybonks commented Jan 9, 2018

Two ideas I have
1 - fork the repo and just remove that test ( would require more maintenance from atom so quite not ideal)
2 - import the library without submoduling libgit2. ( not sure if this is possible)

@vcanales

This comment has been minimized.

Contributor

vcanales commented Jan 9, 2018

Ok, so I've made some progress. On .travis.yml:

git:
  depth: 10
  submodules: false

before_install:
  - git submodule update --init

Since we don't really need any submodules from libgit2 (tested on current master https://travis-ci.org/vcanales/git-utils/builds/326919327), we can skip the --recursive that travis adds by default.

The issue has changed though, while building deps on @billybonks' branch + the submodules modification:

  CC(target) Release/obj.target/libgit2/deps/libgit2/src/iterator.o
../deps/libgit2/src/iterator.c:1459:44: error: no member named 'st_ctimespec' in 'struct stat'
        iter->entry.ctime.nanoseconds = entry->st.st_ctime_nsec;
                                        ~~~~~~~~~ ^
../deps/libgit2/src/unix/posix.h:27:24: note: expanded from macro 'st_ctime_nsec'
# define st_ctime_nsec st_ctimespec.tv_nsec
                       ^
../deps/libgit2/src/iterator.c:1462:44: error: no member named 'st_mtimespec' in 'struct stat'
        iter->entry.mtime.nanoseconds = entry->st.st_mtime_nsec;
                                        ~~~~~~~~~ ^
../deps/libgit2/src/unix/posix.h:26:24: note: expanded from macro 'st_mtime_nsec'
# define st_mtime_nsec st_mtimespec.tv_nsec
                       ^
2 errors generated.

Sample build: https://travis-ci.org/vcanales/git-utils/builds/326922559

@vcanales

This comment has been minimized.

Contributor

vcanales commented Jan 9, 2018

This got it building: The MTIME flag defined was initially only for MacOS, and the linux version is different. Check https://github.com/nodegit/nodegit/blob/master/vendor/libgit2.gyp for a reference

billybonks/git-utils@worktrees...vcanales:worktrees

vcanales and others added some commits Jan 9, 2018

@billybonks

This comment has been minimized.

Contributor

billybonks commented Jan 10, 2018

@vcanales nice, thanks for digging into it. though the other issue is that atom build.sh does a recursive pull on submodules. when I made this pr I tried to use the branch when building a custom atom for myself and it couldn't build for the same reason Travis couldn't build.

@vcanales

This comment has been minimized.

Contributor

vcanales commented Jan 10, 2018

@billybonks I'll take a look later! Thanks for bringing that up

@bbugh

This comment has been minimized.

bbugh commented Jan 23, 2018

Awesome, thank you for looking at this! It looks like good progress.

So many of Atom's important features don't work in worktrees, most importantly the "Exclude VCS Ignored Paths" setting doesn't work, which means that fuzzy-finder doesn't ignore VCS-ignored files.

This results in fuzzy-finder searching giant useless folder structures like node_modules and tmp/cache. It makes it almost unusable in Atom for some projects, and I'm willing to bet it's why some people report the issue with fuzzy finder (like atom/fuzzy-finder#89)

@vcanales

This comment has been minimized.

Contributor

vcanales commented Jan 23, 2018

@bbugh it's a bit surprising that this is not getting more attention! Worktrees are one of the biggest changes to git and they're not even that recent. I've been looking at reproducing the problem that @billybonks mentioned, but this has turned out to be way less simple that the previous stab. I'll keep trying though!

@daviwil

This comment has been minimized.

Member

daviwil commented Feb 20, 2018

Hey folks! Trying to get a read on where we're at with this PR. Can someone summarize what the outstanding issues are that would block us from merging this? Thanks a bunch!

@vcanales

This comment has been minimized.

Contributor

vcanales commented Feb 20, 2018

Can someone summarize what the outstanding issues are that would block us from merging this?

My take is that the issue at the base is recursively updating and initializing submodules on build, as @billybonks points out:

the other issue is that atom build.sh does a recursive pull on submodules. when I made this pr I tried to use the branch when building a custom atom for myself and it couldn't build for the same reason Travis couldn't build.

The folks at libgit2 explain:

We explicitly have test resources that have submodules that should not be initialized - these are our test resources, after all. In particular, we have (intentionally) broken repositories with (intentionally) broken submodules so that we can test these broken configurations.

If you're (manually) running git submodule ... then my encouragement would be for you to not do that.

We've already removed this issue from this module's build, but atom's build fails because of the same issue.

Thanks for the reply!

@daviwil

This comment has been minimized.

Member

daviwil commented Feb 20, 2018

@vcanales thanks a lot for the summary, that was helpful! So the last remaining issue is that Atom build's use of git submodule update --init --recursive causes this problem to appear. Let me see if there's anything we can do about that.

@daviwil

This comment has been minimized.

Member

daviwil commented Feb 20, 2018

@vcanales I don't actually see a submodule pull step in any of Atom's build or CI scripts, could you point me to the script that exhibits this issue?

@vcanales

This comment has been minimized.

Contributor

vcanales commented Feb 20, 2018

@vcanales I don't actually see a submodule pull step in any of Atom's build or CI scripts, could you point me to the script that exhibits this issue?

I was able to reproduce the issue by checking out this branch and building atom based on it -- I also didn't find where the exact dependency is being pulled though :(

@daviwil

This comment has been minimized.

Member

daviwil commented Feb 20, 2018

Just tried building Atom with this branch npm link'ed, seems to work fine. I think I'll go ahead and merge this and try to publish it for use in Atom. If anything goes wrong we can always ship another update :)

Huge thanks to @billybonks and @vcanales for their work on this PR!

@daviwil daviwil merged commit 4f18b7d into atom:master Feb 20, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@daviwil daviwil referenced this pull request Feb 20, 2018

Merged

:arrow_up: git-utils #16799

@daviwil

This comment has been minimized.

Member

daviwil commented Feb 20, 2018

Submitted this as atom/atom#16799 to ensure we don't run into any CI issues there :)

@billybonks

This comment has been minimized.

Contributor

billybonks commented Feb 22, 2018

HORRAY Thanks @daviwil

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment