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

Embed the license data into the library #38

Merged
merged 2 commits into from
Jul 22, 2021

Conversation

dprotaso
Copy link
Contributor

@dprotaso dprotaso commented Jun 15, 2021

Thus we no longer need the modcache present for the library to work properly

@dprotaso
Copy link
Contributor Author

cc @wlynch and @chizhg

dprotaso added 2 commits June 15, 2021 10:11
Thus we no longer need the modcache present for the library to work properly
@dprotaso dprotaso force-pushed the embed-license-data branch from 664fc81 to fa0b7c6 Compare June 15, 2021 14:12
@dprotaso
Copy link
Contributor Author

I tested this out with my fork of go-licenses after I ran go clean -modcache and it worked :)

@dprotaso
Copy link
Contributor Author

Once this merges go-licenses just requires a go.mod bump

@rspier rspier requested a review from wcn3 June 15, 2021 15:18
@rspier
Copy link
Collaborator

rspier commented Jun 15, 2021

These commits appear to be for the old v1 branch. On v2, I believe we're intentionally not embedding license data.

The description isn't clear, this mostly appears to factor out an interface for monkeypatching.

@dprotaso
Copy link
Contributor Author

Interesting didn't realize there's a v2 subfolder - go-licenses seems to use v1 atm

@dprotaso
Copy link
Contributor Author

dprotaso commented Jun 15, 2021

I think there's still value in shifting v1 away from requiring the mod cache to be present

@dprotaso
Copy link
Contributor Author

The description isn't clear, this mostly appears to factor out an interface for monkeypatching.

@rspier The intent of the PR is to prevent the license lookup code from reaching into the go mod cache via runtime.Caller(0). This approach is super fragile to the mod cache being present on the system.

The alternative is to use go1.16 embed directives to embed the licenses into the resulting binary that's consuming v1 lib.

You can repro the old behaviour very easily

$ go install github.com/google/go-licenses@latest
$ go-licenses save [package] --save_path=$(mktemp)  --force
$ go clean -modcache
$ go-licenses save [package] --save_path=$(mktemp)  --force
Error: cannot register licenses from archive: open /go/pkg/mod/github.com/google/[email protected]/licenses/licenses.db: no such file or directory ...
....

This issue actually fixes: #27

out an interface for monkeypatching.

I'm assuming you're referring to?

var ReadLicenseFile = licenses.ReadLicenseFile

I left this in place as to not break compatability/API surface

@dprotaso
Copy link
Contributor Author

@wcn3 any thoughts on this change?

@dprotaso
Copy link
Contributor Author

bump @wcn3 cc @wlynch

Copy link
Collaborator

@wcn3 wcn3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upgrading the minimum version and using the embedding feature is a big win.

Note that the v2 API is not baking the archive into the classifier, letting the user use something like this or any other approach to packaging the licenses. I'll adapt this code as a sample contribution to show one way of initializing the v2 classifier.

Apologies for the delays here. My Github notifications were going to an email that got blackholed. Thank you for making this improvement!

@wcn3 wcn3 merged commit 3043a05 into google:main Jul 22, 2021
@dprotaso dprotaso deleted the embed-license-data branch July 22, 2021 18:57
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

Successfully merging this pull request may close these issues.

3 participants