implement go-licenses/v2 by Bobgy · Pull Request #67 · google/go-licenses

Bobgy · 2021-06-16T11:54:19Z

Closed in favor of #70

this PR is a port from https://github.com/Bobgy/go-mod-licenses

Refer to the README included for comparisons with similar tools including go-licenses v1.
This is still under development, see the roadmap & TODOs section for what needs to happen before finalizing a stable release.

This new tool should be able to resolve:

traverse vanity urls for repository location for license url #32 vanity urls are curled and extracted to find the repo
Provide binaries and releases #21 now the tool can be released with licenses as a standalone tar.gz
Repository url paths should be versioned #33 urls are now versioned according to go modules info
Libraries are ignored when goroot and gopath share the same prefix #38 now we use modules info, no custom parsing
go-licenses doesn't find license in UNLICENSE files in repos #39 we scan all files for licenses, so we can find UNLICENSE files too
Go modules support? #53 proper go modules support
cannot register licenses from archive #56 after upgrading to google/licenseclassifier/v2, we can now control where to load licenses archive, therefore we can release a standalone binary with license archive together to ease usage in other environments without go runtime
bug: Odd lack of dependency licenses for moby/buildkit #64 is a subproblem of proper go modules support

and might help the following (but needs confirmation):

golang.org/x/sys and other packages don't seem to work #18

Bobgy · 2021-06-16T12:05:42Z

Hi @wlynch, I finally got some time to fully clean up and migrate the tool here : )
Feedbacks welcomed!

Because the initial PR is huge, I'd hope we focus on the P0 blockers first. We can log any non-blocking items as follow up issues.

wlynch

Thanks for the PR!

Unfortunately this is a very large change. I've left some initial comments as a first pass, but we'll likely need to do multiple iterations to resolve everything. Something that would greatly help me out for reviewing is if we could break this up into separate PRs. Here's some individual chunks I see that might be good places to break this apart:

Using module cache for license source.
Subfile license checking.
Config file customization.
- Config file overrides.
Single licenses.txt output.
Licenses from binary

Thanks!

wlynch · 2021-06-17T20:46:49Z

v2/goutils/list.go

+}
+
+func ListModules() ([]Module, error) {
+	out, err := exec.Command("go", "list", "-m", "-json", "all").Output()


Prefer using https://pkg.go.dev/golang.org/x/tools over invoking the CLI.

In particular, you probably want https://pkg.go.dev/golang.org/x/tools@v0.1.3/go/packages. This is what v1 already does - the missing piece is using the Module field to know where to access the module cache for source files.

P2

Because this is already abstracted in a package, can be transparently improved with minimum side effect in the future.

After taking a quick look, https://cs.opensource.google/go/x/tools/+/refs/tags/v0.1.3:go/packages/golist.go
packages also use go list under the hood it seems.

Note that the json flag makes output programmatically parsable, so I don't worry much about using the CLI directly.

Do you have concrete scenarios this may be problematic?

v2/deps/go_binary.go

v2/deps/go_binary_test.go

v2/deps/go_binary.go

v2/goutils/list.go

wlynch · 2021-06-18T17:32:11Z

v2/cmd/csv.go

+			errorArgs := []interface{}{"module", goModule.ImportPath}
+			errorArgs = append(errorArgs, args...)
+			klog.ErrorS(err, "Failed", errorArgs...)


Would this be equivalent?

Suggested change

errorArgs := []interface{}{"module", goModule.ImportPath}

errorArgs = append(errorArgs, args...)

klog.ErrorS(err, "Failed", errorArgs...)

klog.ErrorS(err, "Failed", "module", goModule.ImportPath, args...)

Unfortunately, that's a compile error, because when using args..., golang only allows passing an array, it's not supported to specify some arguments first and then the args...

wlynch · 2021-06-18T17:37:54Z

v2/cmd/csv.go

+		}
+	}
+	if errorCount > 0 {
+		return fmt.Errorf("Failed to scan licenses for %v module(s)", errorCount)


This is suppressing the error(s) that encountered, which is going to make it difficult for users to see what failed and if they can do anything about it. Consider including the error here.

It might also be worthwhile to try and process all modules prior to writing the output to a file, this way we don't accidentally write partial output.

Note that the default logging level is warning and users will easily see the entire list of errors because I added klog logging as I encounter each error (and collect them at the same time for a summary at the end).

When I examine licenses, the partially written file is super useful, because I don't need to wait the entire command to finish.

Can you elaborate more why the partially written file can be harmful?

wlynch · 2021-06-18T17:38:01Z

v2/cmd/csv.go

+	}
+	klog.InfoS("Done: scan licenses of dependencies", "licenseCount", licenseCount, "moduleCount", len(goModules))
+	return nil
+}


This func would definitely benefit from some tests. I'd recommend breaking this logic out into it's own func that takes in a io.Writer.

P1

Agree, I started everything in this file and gradually extracted stuff out. Ideally, all the logic should not be in cmd folder.

wlynch · 2021-06-18T17:44:45Z

v2/dict/dict.go

+	return records, nil
+}
+
+func LoadLicenseDict(r io.Reader) (LicenseDict, error) {


AFAICT this isn't used?

P0, good catch!

wlynch · 2021-06-18T17:48:50Z

v2/cmd/save.go

+		info, err := loadInfo(config.Module.Csv.Path)
+		if err != nil {
+			klog.ErrorS(err, "Failed: load license info csv")
+			os.Exit(1)
+		}
+		err = complyWithLicenses(info, *config)


Requiring a csv to be generated and save is changing the semantic of the save command (e.g. save is now a 2 step process instead of 1).

I'd prefer to keep similar semantics for save, though I think adding a flag to read from csv could be a reasonable alternative.

P1

Yes, I was aware this was one of the major differences. Let me have a look at the existing behavior and see how save can be done without csv.

In all my cases, I needed to configure some overrides before actually going to the save step. I am curious why that's different for go-licenses v1. Is it because it doesn't scan the full folder?

Bobgy · 2021-06-19T00:25:09Z

@wlynch thank you so much for putting time to review this thoroughly : )

I understand your concerns, so let me confirm -- we'll proceed like:

We won't merge this PR. We break the PR up into smaller chunks one by one and get them reviewed. Additionally, I will create a tracking issue to track progress.
For existing comments, I will address the quick ones right away, and try to triage others. I'd use P0 as merge blockers, and then P1 and P2.

How does the structure sound to you?

Co-authored-by: Billy Lynch <wlynch92@gmail.com>

Bobgy · 2021-06-20T14:50:37Z

Close, in favor of #70

Bobgy added 2 commits June 16, 2021 19:51

initial

259755a

fix

a474079

google-cla bot added the cla: yes Contributor license agreement signed (https://cla.developers.google.com) label Jun 16, 2021

Bobgy changed the title ~~feat!: initial PR for go-licenses/v2~~ go-licenses/v2 Jun 16, 2021

Bobgy changed the title ~~go-licenses/v2~~ implement go-licenses/v2 Jun 16, 2021

Bobgy added 3 commits June 16, 2021 19:56

add p0 todos

4738d12

clean up

534b3ec

add licenses

6f6e05b

Bobgy mentioned this pull request Jun 17, 2021

Go modules support? #53

Closed

Bobgy added 4 commits June 18, 2021 05:55

feat: support replace directive in go.mod

09280f3

implement some features

4e907c7

add customize allowed license type request

0fa3e13

feat: support overriding license types

f4dbbf5

Bobgy mentioned this pull request Jun 18, 2021

chore(backend): update apiserver licenses kubeflow/pipelines#5889

Merged

1 task

wlynch suggested changes Jun 18, 2021

View reviewed changes

Update v2/cmd/csv.go

fdd91dd

Co-authored-by: Billy Lynch <wlynch92@gmail.com>

Bobgy mentioned this pull request Jun 19, 2021

native go modules support proposal (originally named v2 proposal) #70

Closed

11 tasks

Bobgy closed this Jun 20, 2021

Bobgy mentioned this pull request Jun 25, 2021

improve robustness of linking to license on hosting website #73

Open

Comments

Conversation

Bobgy commented Jun 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Bobgy commented Jun 16, 2021

Uh oh!

wlynch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Bobgy Jun 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Bobgy commented Jun 19, 2021

Uh oh!

Bobgy commented Jun 20, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Bobgy commented Jun 16, 2021 •

edited

Loading

Bobgy Jun 19, 2021 •

edited

Loading