Introduce --clobber option to save command #79
Conversation
google-cla
bot
added
the
cla: yes
|
I'm having the same issue. This PR would solve my problem as well! Currently it's impossible to embed the licenses from go-license into the application itself. Since embedding licenses into the binary allows them to be delivered together with the software (which is highly encouraged by most open-source licenses), I feel like this feature should be a priority. |
|
@wlynch Any chance you could review and merge this? Thanks! |
There was a problem hiding this comment.
Thanks for the ping!
I'm a little hesitant to accept this as-is - this feels like a bug with --force, and I'm worried the distinction between --force and --clobber is going to be difficult to understand unless you dig into the code.
I think the usage with embed.FS is interesting (I do something similar with my projects with docker images produced with ko). Do you have a simplified repo handy that can be used to repro this issue? I'd like to dig into this a bit more.
I'm not super familiar with the specifics of how embed.FS works under the hood, but if it's causing go-licenses to fail mid-execution then it's likely being invoked when we process the packages, which also has me worried that it might be embedding an incomplete copy of the license data.
| @@ -42,8 +42,12 @@ var ( | |||
| // savePath is where the output of the command is written to. | |||
| savePath string | |||
| // overwriteSavePath controls behaviour when the directory indicated by savePath already exists. | |||
| // If true, the directory will be replaced. If false, the command will fail. | |||
| // If true, the directory will be replaced. If false (and clobberSavePath is true), the command will fail. | |||
There was a problem hiding this comment.
If false (and clobberSavePath is true), the command will fail.
I don't think this is actually enforced anywhere.
|
I would not disagree with this being more of a bug in Here's an example repo showing the issue: https://github.com/jimmale/examplelicensesissue |
|
Thanks for the repo! It helped for me play around with things and understand what's happening You were spot on - the main issue here is we're deleting the directory prior to the package load. Since the embed logic happens at compile time (which happens during the package load), this leads to the behavior you're seeing. I think the easiest way to address this is just flip the package loading and directory creation logic, so that we don't muck with the local state during loading. e.g. func saveMain(_ *cobra.Command, args []string) error {
classifier, err := licenses.NewClassifier(confidenceThreshold)
if err != nil {
return err
}
libs, err := licenses.Libraries(context.Background(), classifier, args...)
if err != nil {
return err
}
// ^ \ v these parts were flipped before.
if overwriteSavePath {
if err := os.RemoveAll(savePath); err != nil {
return err
}
}
// Check that the save path doesn't exist, otherwise it'd end up with a mix of
// existing files and the output of this command.
if d, err := os.Open(savePath); err == nil {
d.Close()
return fmt.Errorf("%s already exists", savePath)
} else if !os.IsNotExist(err) {
return err
}This would let us avoid the new flag altogether. The only caveat to this is that it won't work if the save directory doesn't exist from the start, though I think this is WAI since a wdyt? |
|
I've tested it, and swapping those two bits of code around would suit my use case! (Although I can contrive a few situations in which it might not suit someone else's use cases...) I agree that the behavior you described is WAI, since trying to embed a non-existent directory is a compile time error.
I'll open a separate PR momentarily. |
…ed package See discussion at google#79
|
I'm closing this PR in favor of PR #90 |
…ed package See discussion at #79
Hello,
I'm trying to use go-licenses to collect the licenses of the various dependencies of my project, so I can include them in my application using the
embed.FSinterface, however I ran into some difficulties, which lead me to making this small change to provide a--clobberoption to the go-licenses'savecommand.In my use case, I'm trying to save all of the license files to
./terms/terms/for theembedpackage to pick up. Since the directory does not exist, and theembedpackage does not allow for empty instances ofembed.FS, go-licenses doesn't work.If, however, I create the directory, go-licenses correctly informs me that the directory already exists.
$ mkdir -p ./terms/terms/ $ go-licenses save github.com/jimmale/godynamicdns --save_path="./terms/terms/" Error: ./terms/terms/ already exists [...] F0828 13:11:09.075102 7303 main.go:43] ./terms/terms/ already existsIf I use the
--forceoption, go-licenses deletes the directory early in the process, leading us back to the first scenario above.This seems to be a great use case for a
--clobberoption, which you can see demonstrated below:$ mkdir -p ./terms/terms/ $ touch ./terms/terms/make_embedFS_happy $ go-licenses save github.com/jimmale/godynamicdns --clobber --save_path="./terms/terms/" $ tree ./terms/terms/ ./terms/terms/ ├── github.com │ ├── BurntSushi │ │ └── toml │ │ └── COPYING │ ├── cpuguy83 │ │ └── go-md2man │ │ └── v2 │ │ └── md2man │ │ └── LICENSE.md │ ├── jimmale │ │ └── godynamicdns │ │ └── LICENSE │ ├── russross │ │ └── blackfriday │ │ └── v2 │ │ └── LICENSE.txt │ ├── shurcooL │ │ └── sanitized_anchor_name │ │ └── LICENSE │ ├── sirupsen │ │ └── logrus │ │ └── LICENSE │ └── urfave │ └── cli │ └── v2 │ └── LICENSE ├── golang.org │ └── x │ └── sys │ └── LICENSE └── make_embedFS_happy 22 directories, 9 files