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

Add option to switch to external git tool (git.exe) #85

Open
sergey-ostapenko opened this issue May 29, 2018 · 8 comments
Open

Add option to switch to external git tool (git.exe) #85

sergey-ostapenko opened this issue May 29, 2018 · 8 comments
Assignees

Comments

@sergey-ostapenko
Copy link
Contributor

sergey-ostapenko commented May 29, 2018

We currently trying to use mbt to build our monorepo on Windows and we faced few problems:

  1. Libgit2 does not work correctly with long paths and it is not going to be fixed.
  2. Libgit2 does not work correctly in docker container for some reason, and always returns empty request results for repository.

In both cases git.exe has correct behavior and could be used as backup tool.
Can we consider such option? It could be useful in a lot of other corner cases, and definitely can make tool more usable.

@buddhike
Copy link
Member

buddhike commented May 29, 2018

Thanks for reporting this issue.

  1. Long path is definitely going to be an issue. Which version of Windows are you using? I will get back to you on this after checking a couple of things.

  2. Are you running it in a Windows container? Which mbt command is used?

@sergey-ostapenko
Copy link
Contributor Author

sergey-ostapenko commented May 29, 2018

  1. Windows 10. Version 1709 with long paths enabled in operation system. And I see the same problem on Windows Server 2016 machine.
    You can see the details and original issue on libgit2 here: Support core.longpaths on Windows libgit2/libgit2#3053
  2. Yes, I tried to use mbt from Windows Docker container. Result of the following commands seems to be empty for me:
    mbt describe local --all
    mbt describe commit <sha of HEAD>
    After additional experiment it seems that we have this problem only in mounted directories. I will double check it one more time.

@buddhike
Copy link
Member

Thanks. Trying to reproduce the container scenario here. What's the base image your reference in your Dockerfile?

@sergey-ostapenko
Copy link
Contributor Author

sergey-ostapenko commented May 29, 2018

We are creating container from very basic image:
FROM microsoft/windowsservercore
I also trying to perform additional tests and for now it seems that additional condition to reproduce the bug is to have repository directory mounted from host machine docker run --rm -it -v c:\Host\Path:c:\work <image>

@buddhike
Copy link
Member

On this end, only mbt describe local --all is returning an empty list. Looking at the root cause at the moment.

In terms of the long path issue, I think it's best to fix it in libgit2. Switching to git cli introduces other issues associated with parsing stdout (in fact this is the main reason why we used libgit2). Having said that, using plumbing commands in git (e.g. git ls-tree | diff-tree etc..) seems like a safe bet as they are built for programatic access.

Implementing this would be a matter of having an implementation of Repo interface (https://github.com/mbtproject/mbt/blob/master/lib/system.go#L68) to for git cli. Then at the time we compose system, we could use that implementation based on a flag (https://github.com/mbtproject/mbt/blob/master/lib/system.go#L422).

Is this something that you could give us a hand with?

@buddhike buddhike self-assigned this Jun 18, 2018
@buddhike
Copy link
Member

Quick update on this. It looks like an issue in the filepath.Walk API on mounted volumes in Windows. I created a simple bin to test (see below) and it does not work under the conditions mentioned in this bug report.

I want to take a look at Walk implementation and also try to get some assistance from go team (it could very well be an issue with docker/windows as well).

package main

import (
	"flag"
	"fmt"
	"os"
	"path/filepath"
)

func main() {
	path := flag.String("path", "", "path to walk")
	flag.Parse()
	fmt.Printf("walking path %v\n", *path)

	err := filepath.Walk(*path, func(path string, info os.FileInfo, err error) error {
		if err != nil {
			panic(err)
		}
		fmt.Printf("visiting %v\n", path)
		return nil
	})

	if err != nil {
		panic(err)
	}
}

@buddhike
Copy link
Member

buddhike commented Jul 3, 2018

@sergey-ostapenko I think this is also indirectly fixed by your local discovery optimisation. Could you please check?
(Not long path issue, just the one that didn't report local modules in the container)

@sergey-ostapenko
Copy link
Contributor Author

@BuddySpike Yes, I can confirm that now it works as expected.
I still think that having implementation of Repo that will wrap git.exe could be useful, but it could be postponed.

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

No branches or pull requests

2 participants