export content of git submodules #1909

pohly · 2018-06-25T14:36:58Z

What does this do / why do we need it?

The "git checkout-index" command is unaware of git submodules,
therefore their content was missed when exporting files. Only a few projects have submodules (for example, git-hooks in github.com/nightlyone/lockfile and idl in github.com/uber/jaeger-client-go) and it seems that so far those submodules were not needed.

But nonetheless, the extra files are part of the source tree and thus should be exported because some projects might depend on them.

What should your reviewer look out for in this PR?

The tests for this feature depend on repos that I created under github.com/pohly. I can keep them there, but it might be cleaner to clone them into your own org and then update the tests.

googlebot · 2018-06-25T14:37:03Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

googlebot · 2018-06-25T14:37:03Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

pohly · 2018-06-28T11:09:29Z

Please note that I should be covered by a company CLA between Intel and Google. Let me know if you need more details to find that CLA.

sdboyer · 2018-07-02T03:42:48Z

Wow. Who'dve thought there'd be yet another reason i'd hate git submodules.

If you could provide a bit more info on the Intel<>Google CLA, that'd be helpful - being that i'm not a Google employee, i don't have ready access to that information, and i'd rather not trip over anything here.

Does the usage of POSIX shell syntax work also on Mac OS and Windows?

On OSX, yes. i'm not sure about Windows. That tests pass on appveyor suggests it's probably fine, but if you can think of any way to test this reasonably easily, it would be helpful.

Also, a CHANGELOG entry, please!

pohly · 2018-07-03T08:34:18Z

sam boyer <[email protected]> writes:

If you could provide a bit more info on the Intel<>Google CLA, that'd be helpful - being that i'm not a Google employee, i don't have ready access to that information, and i'd rather not trip over anything here.

I'll try to sort that out with some Google employees.

> Does the usage of POSIX shell syntax work also on Mac OS and Windows? On OSX, yes. i'm not sure about Windows. That tests pass on appveyor suggests it's _probably_ fine,

I'm not sure whether existing tests cover this case. Is there any test where a repo gets vendored which has submodules? Without those, "git submodule foreach" will never invoke the shell command and might pass on Windows because of that. I had a brief look at how tests are done, but it looked a bit daunting, so I wanted to send this PR first to get some opinions before investing more time.

but if you can think of any way to test this reasonably easily, it would be helpful.

Construct two repos, include the first one in the second one with a submodule, then vendor the second one into a project. Check that the resulting "vendor" directory contains the files from the first repo.

Also, a CHANGELOG entry, please!

I had already added one in commit 078c630. The way I understand it, one has to create a PR to get a pull request number, then force-push a commit with changelog entry - at least that's how I did it.

pohly · 2018-07-23T19:18:31Z

@sdboyer I've rebased and added a testcase with recursive submodules. I don't think there were any test cases which had a submodule at all. The new test revealed that my original code only worked for exporting submodules from the main repo - it failed for submodules inside submodules.

In order to test this, I had to extend the integration testing a bit. Otherwise it would not have detected the missing submodule files. The new testing should now confirm whether the code also works on non-Linux platforms.

Regarding the CLA: I'm still working on it :-/ In the meantime consider this a RFC patch, and not a contribution - it cannot be merged yet.

pohly · 2018-07-23T19:21:30Z

About the test: the new repos are under github.com/pohly and I can keep them there in perpetuity, but it might be better to move them to github.com/sdboyer so that they are under your control. You don't need a CLA for that, do you?

pohly · 2018-07-23T21:12:11Z

Hmm, it failed on appvoyer once (https://ci.appveyor.com/project/golang/dep/build/3389) but after I changed the appvoyer config to be more verbose it succeeded without any other changes.

sdboyer · 2018-07-23T23:16:05Z

Cool - i'll aim to have a look at this next week, as this week is focused on shipping v0.5.0.

pohly · 2018-07-31T14:33:13Z

I signed it!

This is for the CLA bot - I should be on the approved contributor list now.

googlebot · 2018-07-31T14:33:19Z

CLAs look good, thanks!

googlebot · 2018-07-31T14:33:21Z

CLAs look good, thanks!

When running "internal" and "external" versions of the same test case in parallel, the test case data must be read-only to avoid race conditions. When it is read/write because the tests were asked to update the reference files with "-update", running in parallel must be disabled. This broke in practice when updating entire directory trees. The risk of hitting the race is smaller when only updating single files, but it is still a race.

Debugging a failing test with dlv is possible, but in contrast to "go test", "dlv test" doesn't change the current working directory, which is necessary for the test to find its test data.

pohly · 2018-10-29T18:15:10Z

@sdboyer I understand that your priorities may have shifted now, but as I depend on "dep" and go modules don't look like a viable alternative it would be nice to get this merged.

sdboyer · 2018-12-14T15:02:14Z

gps/vcs_source.go

+			// "<root>/foo" for bar. By stripping our root
+			// directory we get a relative path that we can
+			// append to the target directory.
+			"(cd $(grep gitdir .git | sed -e 's;gitdir: *;;' .git) && mv index origindex && trap 'mv origindex index' EXIT && git read-tree \"$sha1\" && git checkout-index -a --prefix=\""+to+"$(echo \"$toplevel\" | sed -e 's;"+r.LocalPath()+";;')/$path/\")",


grep, sed, etc., - not gonna be portable to windows 😢

Let's take an approach with two execs - call out to git to enumerate the submodules, loop over the results with a call to fs.RenameWithFallback(), then call out again with git submodule foreach. Simplicity, correctness and portability are worth the performance hit.

What command should be used to enumerate the submodules?

If it's "git submodule foreach", then I still have the same problem: what can I put into that script that is portable?

i think you should be able to get what you need from git submodule status output. Icky, but it's there.

FWIW, you mentioned before (#1909 (comment)) that the shell code might be okay because "tests pass on appveyor suggests it's probably fine". That was before I added an explicit test for submodules, but it continued to pass also after I added the new testcase. So just to be sure, automatic testing in appveyor includes running dep on Windows?

But I've rewritten the code such that it is based on the output of git submodule status anyway, with the "export tree" functionality done by the same Go code as before, just moved to a new method. It's easier to understand this way. The only thing I don't like about it is parsing the output of git submodule status, because there is no well-defined, machine-readable output format.

sdboyer · 2018-12-14T15:18:08Z

internal/fs/fs.go

+
+// CopyTree recursively copies a source directory into a target directory.
+// Target directories are created if they don't exist yet.
+func CopyTree(dest, src string) error {


We didn't have something like this already?

Not sure how hard I searched for it. It's possible that I just overlooked such an utility function. If so, can your provide a pointer?

We've got CopyDir -

dep/internal/fs/fs.go

Line 356 in 22125cf

func CopyDir(src, dst string) error {

i think that should do it.

Actually, this CopyTree wasn't something that I created ;-) I just moved it here from testproj.go. But replacing it with CopyDir still sounds like a good idea.

I've added a separate commit which removes this code duplication.

This PR is starting to collect some commits which could also be merged separately, like that cleanup commit. Please let me know whether I should create separate PRs for that or whether you are fine with merging everything in one go.

The functionality is the same and some minor differences (target directory must not exist when using fs.CopyDir) can be handled by being careful about creating the temporary directories.

So far, only the list of vendored projects and the top-level Gopk.toml/lock files were compared. This way it was impossible to write integration tests that also check the content of the vendored projects. Now a test case can specify that it's 'final' directory contains a complete set of reference files, and then all files are checked respectively updated (when --updated is given). Directories and file meta still aren't checked. The reference files of existing tests can be changed on a case-by-case basis. This new comparison mode cannot be enabled unconditionally because a few tests then fail because each test run creates _vendor-<datetime> directories which are named differently in each test run.

The "git checkout-index" command is unaware of git submodules, therefore their content was missed when exporting files. With "git submodule foreach" the same operation (read-tree + checkout-index) can also be done for all submodules, recursively. The downside is that "foreach" invokes a shell command with certain variables set. To achieve the desired functionality, we have to rely on POSIX shell support. OTOH, the command seems to get embedded by git in a larger shell script because error messages when there are syntax errors show that the command gets invoked via `eval`, so git already depends on a POSIX shell.

This new test fails if the content of two recursive submodules is not exported. Verification depends on comparing the entire "final" directory.

Without the logs it is impossible to understand why certain commands fail.

pohly · 2019-01-03T16:29:55Z

@sdboyer please have another look. There are test failures, but they seem unrelated to this PR.

mvdan · 2020-09-04T15:08:48Z

Dep was officially deprecated earlier this year, and the proposal to archive this repository was accepted. As such, I'm closing outstanding issues before archiving the repository. For any further comments, please use the proposal thread on the Go issue tracker. Thanks!

pohly requested a review from jmank88 as a code owner June 25, 2018 14:36

googlebot added the cla: no label Jun 25, 2018

pohly force-pushed the export-git-submodule branch from 7c0d5d6 to 078c630 Compare June 25, 2018 14:39

pohly requested a review from sdboyer as a code owner June 25, 2018 14:39

pohly mentioned this pull request Jun 26, 2018

Fail to get git submodule of a package after the dep ensure command #1633

Closed

pohly force-pushed the export-git-submodule branch from 078c630 to 2b9f10f Compare July 6, 2018 16:17

sdboyer mentioned this pull request Jul 9, 2018

dep ensure produces incomplete vendor results #1890

Closed

sdboyer added this to the v0.5.0 milestone Jul 11, 2018

pohly force-pushed the export-git-submodule branch from 2b9f10f to e7d68db Compare July 23, 2018 19:13

pohly requested a review from ibrasho as a code owner July 23, 2018 19:13

pohly changed the title ~~export content of git submodules~~ RFC: export content of git submodules - not a contribution (yet) Jul 23, 2018

pohly force-pushed the export-git-submodule branch 2 times, most recently from 541c402 to 2230db0 Compare July 23, 2018 20:23

sdboyer removed this from the v0.5.0 milestone Jul 23, 2018

googlebot removed the cla: no label Jul 31, 2018

googlebot added the cla: yes label Jul 31, 2018

pohly force-pushed the export-git-submodule branch from 2230db0 to 996d706 Compare July 31, 2018 14:35

pohly changed the title ~~RFC: export content of git submodules - not a contribution (yet)~~ export content of git submodules Jul 31, 2018

pohly added 2 commits September 12, 2018 09:44

document how to run "dlv" for harness tests

89ddb57

Debugging a failing test with dlv is possible, but in contrast to "go test", "dlv test" doesn't change the current working directory, which is necessary for the test to find its test data.

sdboyer suggested changes Dec 14, 2018

View reviewed changes

pohly force-pushed the export-git-submodule branch from 996d706 to 3a2fadb Compare January 3, 2019 15:48

pohly added 5 commits January 3, 2019 17:07

testing: replace CopyTree with fs.CopyDir

5bdcd16

The functionality is the same and some minor differences (target directory must not exist when using fs.CopyDir) can be handled by being careful about creating the temporary directories.

dep: add submodule testcase

d338aaf

This new test fails if the content of two recursive submodules is not exported. Verification depends on comparing the entire "final" directory.

appvoyer: verbose test execution

8447e4c

Without the logs it is impossible to understand why certain commands fail.

pohly force-pushed the export-git-submodule branch from 3a2fadb to 8447e4c Compare January 3, 2019 16:07

mvdan closed this Sep 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

export content of git submodules #1909

export content of git submodules #1909

pohly commented Jun 25, 2018 •

edited

Loading

googlebot commented Jun 25, 2018

googlebot commented Jun 25, 2018

pohly commented Jun 28, 2018

sdboyer commented Jul 2, 2018

pohly commented Jul 3, 2018 via email

pohly commented Jul 23, 2018 •

edited

Loading

pohly commented Jul 23, 2018

pohly commented Jul 23, 2018

sdboyer commented Jul 23, 2018

pohly commented Jul 31, 2018

googlebot commented Jul 31, 2018

googlebot commented Jul 31, 2018

pohly commented Oct 29, 2018

sdboyer Dec 14, 2018

pohly Dec 14, 2018

sdboyer Dec 18, 2018

pohly Jan 3, 2019

sdboyer Dec 14, 2018

pohly Dec 14, 2018

sdboyer Dec 18, 2018

pohly Dec 19, 2018

pohly Jan 3, 2019

pohly commented Jan 3, 2019

mvdan commented Sep 4, 2020

export content of git submodules #1909

export content of git submodules #1909

Conversation

pohly commented Jun 25, 2018 • edited Loading

What does this do / why do we need it?

What should your reviewer look out for in this PR?

googlebot commented Jun 25, 2018

What to do if you already signed the CLA

Individual signers

Corporate signers

googlebot commented Jun 25, 2018

What to do if you already signed the CLA

Individual signers

Corporate signers

pohly commented Jun 28, 2018

sdboyer commented Jul 2, 2018

pohly commented Jul 3, 2018 via email

pohly commented Jul 23, 2018 • edited Loading

pohly commented Jul 23, 2018

pohly commented Jul 23, 2018

sdboyer commented Jul 23, 2018

pohly commented Jul 31, 2018

googlebot commented Jul 31, 2018

googlebot commented Jul 31, 2018

pohly commented Oct 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pohly commented Jan 3, 2019

mvdan commented Sep 4, 2020

pohly commented Jun 25, 2018 •

edited

Loading

pohly commented Jul 23, 2018 •

edited

Loading