Add functions to test if a file likely to be legal notice tools/godep#245

[client9]

Issue #245

Currently un-used functions to test if a file is a likely to be software license or legal file.

I'm happy to take a stab a solve the copying problem but I'm not that familiar with the godep code and will need a pointer to the right area.

regards,

n

[freeformz]

Looks like a good start.
Any reason why we shouldn't include this as a library though?

Wrt using it ...
godep (generally speaking) uses absolute paths to files it finds in the repo. copySrc uses vcs.listFiles() to get that list and copyPkgFile to conditionally copy the files into the vendor directory.

So IMO IsLicenseFile and IsLegalFile should take into consideration that it may get a full (or partial) path to a filename, not just the basename

[client9]

Oh you can include it as a library. Maybe thats better since godep isn't really structured as a library (thats too bad since the VCS stuff might be nice to reuse else where). I was under the (wrong) impression you didn't use external dependencies in godep. I will work on cleaning up my library.

I just added some code to copySrc that seems to do the right thing. But Im new to this code base. Suggestions most welcome.

I'll make a fake app and try it out with cactus's statsd library.

re: So IMO IsLicenseFile and IsLegalFile should take into consideration that it may get a full (or partial) path to a filename, not just the base name

thanks for tips. I'll take a look at other calls to copySrc

onward

[freeformz]

@client9 vcs.listFiles() already produces a list of all possible filenames (with absolute paths) that would be considered for copying. Each of those is passed to copyPkgFile, where filtering takes place (arguably the filtering should be in a separate function) and if not filtered out the file is then copied. Please take a look at those functions as you can probably just hook into copyPkgFile.

[client9]

ahh great. Will look!

[client9]

Hmm, my understanding of the code is that if it's a normal dependency directory, then
everything gets copied unless it ends in _test.go (sometimes).

From reading the ticket, for the parent directories, we want nothing copied unless it's legal file.

Options:

• pass in another flag to copyPkgFile and sort this out in this function.
• write a copyLegalFile function which is a simplified version of copyPkgFile that just tests and copies legal-like files.

any preference or did I miss something here?

Option 1 would look like this

func copyPkgFile(vf vcsFiles, dstroot, srcroot string, w *fs.Walker, legalOnly bool) error {

and

if !legalOnly && !saveT && strings.HasSuffix(name, "_test.go") {

and

if legalOnly && !IsLegalFile(name) {
    return nil
}

or so.

[freeformz]

@client9 After reading your comment I realized that dep.dir could be a sub package as well, which I didn't consider last night, so you'll only be walking through the files in that sub package.

Another loop, using the contents of vf, which contains all tracked files in the repo (as absolute paths) is possibly the way to go?

[client9]

thanks for your review.

re: "After reading your comment I realized that dep.dir could be a sub package as well, which I didn't consider last night, so you'll only be walking through the files in that sub package."

True, but legal files in a sub package are copied already. Its really just the parents that is a problem. So the current pull solves the ticket issue (although we can clean up the patch a bit). Or are we solving different problems ?

[client9]

Ahh I put my comments in the original ticket. Sorry for the confusion.

#245 (comment)

[freeformz]

There's no need to try to find the parent, vf already contains all files for the repo that contains the package. I realized that I didn't update the listFIles() doc comment. So I did: 2115b73. Was that a source of confusion?

[client9]

oh @freeformz.. mostly me posting in issue and in pull request, and learning how you guys like doing stuff, and a new code base. Nothing you did. I'll take a look more closely...

[client9]

I played around with vcs and friends and finally understand the hint you are giving.... and realized I was "solving" a more general problem of a license is any parent directory. We just assume if the sub-package is missing a license, the license (if any) better be at the top of the package. No need to walk up.

[freeformz]

@client9 Cool, let me know when I should re-review the PR.

[client9]

hi @freeformz latest should have a simplified version, although I think it can be simplified further. Let me know what you think.

[freeformz]

@client9 Thanks, merged after a rebase/squash/version/changelog addition.

[freeformz]

dcbe29c