From 9651a1e2f87474b80ed0a3c7b480755c58daa65c Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Sat, 17 Aug 2024 14:37:03 +0100 Subject: [PATCH 1/9] Start converting over --- go.mod | 1 + go.sum | 2 ++ src/core/build_target.go | 15 ++------------- src/core/config.go | 1 + src/parse/asp/targets.go | 24 +++++++++++++++++++++++- third_party/go/BUILD | 7 +++++++ 6 files changed, 36 insertions(+), 14 deletions(-) diff --git a/go.mod b/go.mod index 50c5c30f7..1de07cc4c 100644 --- a/go.mod +++ b/go.mod @@ -66,6 +66,7 @@ require ( github.com/beorn7/perks v1.0.1 // indirect github.com/chzyer/readline v1.5.1 // indirect github.com/felixge/httpsnoop v1.0.4 // indirect + github.com/github/go-spdx/v2 v2.3.1 // indirect github.com/go-logr/logr v1.4.1 // indirect github.com/go-logr/stdr v1.2.2 // indirect github.com/go-ole/go-ole v1.3.0 // indirect diff --git a/go.sum b/go.sum index 4c9adffbf..2a9f552fe 100644 --- a/go.sum +++ b/go.sum @@ -61,6 +61,8 @@ github.com/felixge/httpsnoop v1.0.4 h1:NFTV2Zj1bL4mc9sqWACXbQFVBBg2W3GPvqp8/ESS2 github.com/felixge/httpsnoop v1.0.4/go.mod h1:m8KPJKqk1gH5J9DgRY2ASl2lWCfGKXixSwevea8zH2U= github.com/fsnotify/fsnotify v1.7.0 h1:8JEhPFa5W2WU7YfeZzPNqzMP6Lwt7L2715Ggo0nosvA= github.com/fsnotify/fsnotify v1.7.0/go.mod h1:40Bi/Hjc2AVfZrqy+aj+yEI+/bRxZnMJyTJwOpGvigM= +github.com/github/go-spdx/v2 v2.3.1 h1:ffGuHTbHuHzWPt53n8f9o8clGutuLPObo3zB4JAjxU8= +github.com/github/go-spdx/v2 v2.3.1/go.mod h1:2ZxKsOhvBp+OYBDlsGnUMcchLeo2mrpEBn2L1C+U3IQ= github.com/go-kit/log v0.1.0/go.mod h1:zbhenjAZHb184qTLMA9ZjW7ThYL0H2mk7Q6pNt4vbaY= github.com/go-logfmt/logfmt v0.5.0/go.mod h1:wCYkCAKZfumFQihp8CzCvQ3paCTfi41vtzG1KdI/P7A= github.com/go-logr/logr v1.2.2/go.mod h1:jdQByPbusPIv2/zmleS9BjJVeZ6kBagPoEUsqbVz/1A= diff --git a/src/core/build_target.go b/src/core/build_target.go index 2b9ea38d3..82cbd1339 100644 --- a/src/core/build_target.go +++ b/src/core/build_target.go @@ -152,8 +152,8 @@ type BuildTarget struct { // Acceptable hashes of the outputs of this rule. If the output doesn't match any of these // it's an error at build time. Can be used to validate third-party deps. Hashes []string - // Licences that this target is subject to. - Licences []string + // SPDX licence expression that this target is subject to. + Licence string // Any secrets that this rule requires. // Secrets are similar to sources but are always absolute system paths and affect the hash // differently; they are not used to determine the hash for retrieving a file from cache, but @@ -1764,17 +1764,6 @@ func (target *BuildTarget) insert(sl []string, s string) []string { return append(sl, s) } -// AddLicence adds a licence to the target if it's not already there. -func (target *BuildTarget) AddLicence(licence string) { - licence = strings.TrimSpace(licence) - for _, l := range target.Licences { - if l == licence { - return - } - } - target.Licences = append(target.Licences, licence) -} - // AddHash adds a new acceptable hash to the target. func (target *BuildTarget) AddHash(hash string) { target.Hashes = append(target.Hashes, hash) diff --git a/src/core/config.go b/src/core/config.go index 526f70388..864fb73a4 100644 --- a/src/core/config.go +++ b/src/core/config.go @@ -689,6 +689,7 @@ type Configuration struct { buildEnvStored *storedBuildEnv FeatureFlags struct { + SPDXLicencesOnly bool `help:"Licences on build targets can only be strings containing SPDX expressions, not lists."` } `help:"Flags controlling preview features for the next release. Typically these config options gate breaking changes and only have a lifetime of one major release."` Metrics struct { PrometheusGatewayURL string `help:"The gateway URL to push prometheus updates to."` diff --git a/src/parse/asp/targets.go b/src/parse/asp/targets.go index d916a1a13..f881d20a9 100644 --- a/src/parse/asp/targets.go +++ b/src/parse/asp/targets.go @@ -273,8 +273,30 @@ func populateTarget(s *scope, t *core.BuildTarget, args []pyObject) { addDependencies(s, "internal_deps", args[internalDepsBuildRuleArgIdx], t, false, true) addStrings(s, "labels", args[labelsBuildRuleArgIdx], t.AddLabel) addStrings(s, "hashes", args[hashesBuildRuleArgIdx], t.AddHash) - addStrings(s, "licences", args[licencesBuildRuleArgIdx], t.AddLicence) addStrings(s, "requires", args[requiresBuildRuleArgIdx], t.AddRequire) + if expr, ok := args[licencesBuildRuleArgIdx].(pyString); ok { + target.Licence = string(expr) + } else { + // TODO(v18): Remove all this once strings are the only option. + s.Assert(!s.state.config.FeatureFlags.SPDXLicencesOnly, "The licences argument must be a string") + l, ok := asList(args[licencesBuildRuleArgIdx]) + s.Assert(ok, "The licences argument must be a string or list (was %s)", l.Type()) + if len(l) == 1 { + // minor optimisation: avoid allocating a slice etc for the overwhelmingly common case of a single licence + str, ok := l[0].(pyString) + s.Assert(ok, "Items in the licences argument must be strings (was %s)", s.Type()) + target.Licence = string(str) + } else { + l2 := make([]string, len(l)) + for i, x := range l { + str, ok := x.(pyString) + s.Assert(ok, "Items in the licences argument must be strings (was %s)", s.Type()) + l2[i] = string(str) + } + // Passing a list to Please is implicitly a series of alternative licences + target.Licence = strings.Join(l2, " OR ") + } + } if vis, ok := asList(args[visibilityBuildRuleArgIdx]); ok && len(vis) != 0 { if v, ok := vis[0].(pyString); ok && v == "PUBLIC" { t.Visibility = core.WholeGraph diff --git a/third_party/go/BUILD b/third_party/go/BUILD index ce8633671..7493ef482 100644 --- a/third_party/go/BUILD +++ b/third_party/go/BUILD @@ -686,3 +686,10 @@ go_repo( module = "github.com/grpc-ecosystem/go-grpc-prometheus", version = "v1.2.0", ) + +go_repo( + licences = ["MIT"], + module = "github.com/github/go-spdx", + version = "v2.3.1", +) + From f6c9e85bc14d69e997910a57429caeb3155dc009 Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Sat, 17 Aug 2024 15:01:37 +0100 Subject: [PATCH 2/9] Make it all work --- go.mod | 1 + go.sum | 6 +++++ src/build/incrementality.go | 5 +--- src/build/incrementality_test.go | 2 +- src/core/BUILD | 1 + src/core/build_target.go | 26 +++++++-------------- src/core/build_target_test.go | 11 ++++++--- src/core/stamp.go | 11 ++++++--- src/core/stamp_test.go | 12 ++++++---- src/parse/asp/builtins.go | 7 ++++-- src/parse/asp/targets.go | 40 +++++++++++++++++--------------- third_party/go/BUILD | 9 +++++-- 12 files changed, 75 insertions(+), 56 deletions(-) diff --git a/go.mod b/go.mod index 1de07cc4c..7f5f20583 100644 --- a/go.mod +++ b/go.mod @@ -86,6 +86,7 @@ require ( github.com/mostynb/zstdpool-syncpool v0.0.13 // indirect github.com/opencontainers/go-digest v1.0.0 // indirect github.com/pborman/uuid v1.2.1 // indirect + github.com/peterebden/go-spdx/v2 v2.4.3 // indirect github.com/pkg/errors v0.9.1 // indirect github.com/pmezard/go-difflib v1.0.0 // indirect github.com/power-devops/perfstat v0.0.0-20240221224432-82ca36839d55 // indirect diff --git a/go.sum b/go.sum index 2a9f552fe..adca89360 100644 --- a/go.sum +++ b/go.sum @@ -189,6 +189,12 @@ github.com/peterebden/go-cli-init/v5 v5.2.1 h1:o+7EjS/PiYDvFUQRQVXJRjinmUzDjqcea github.com/peterebden/go-cli-init/v5 v5.2.1/go.mod h1:0eBDoCJjj3BWyEtidFcP0TlD14cRtOtLCrTG/OVPB74= github.com/peterebden/go-deferred-regex v1.1.0 h1:XNpUuRDU7iU59Toy+OJp9LUvsun5i3kEbe3c73oyCZg= github.com/peterebden/go-deferred-regex v1.1.0/go.mod h1:EhIu4zsN+60671cx29rzxPSIEnDd2vOia4RFSOaYpRI= +github.com/peterebden/go-spdx/v2 v2.4.1 h1:ah4SjMgKyFXqR5qkBGwKbtevTV3UK3+17OiPNw18GVM= +github.com/peterebden/go-spdx/v2 v2.4.1/go.mod h1:6KrhEd9kpULcLYV6rGkV47Rolvt2IFcaRCyewNnLkLA= +github.com/peterebden/go-spdx/v2 v2.4.2 h1:u+lPJXPPK1cJpCtDQBlUgu/cXgOg/dYUWKHUyHxakYI= +github.com/peterebden/go-spdx/v2 v2.4.2/go.mod h1:6KrhEd9kpULcLYV6rGkV47Rolvt2IFcaRCyewNnLkLA= +github.com/peterebden/go-spdx/v2 v2.4.3 h1:iiOEYy8+qvyTA3IBqVgLGpwTl5MAmU0Ej39w+8xr5gQ= +github.com/peterebden/go-spdx/v2 v2.4.3/go.mod h1:6KrhEd9kpULcLYV6rGkV47Rolvt2IFcaRCyewNnLkLA= github.com/peterebden/go-sri v1.1.1 h1:KK8yZ5/NX8YzWUY9QvhrP220QsvEKANLLAgvw35AkyU= github.com/peterebden/go-sri v1.1.1/go.mod h1:KIRxtog35NfDWec5LV/iBqqfOEPcMpePZLc7EPE6goQ= github.com/peterebden/tools v0.0.0-20190805132753-b2a0db951d2a h1:R4xz7BkSIQOS5CFmaadk2gwwOzy/u2Jvnimf1NHD2LY= diff --git a/src/build/incrementality.go b/src/build/incrementality.go index d9dc38dba..a88ac2c08 100644 --- a/src/build/incrementality.go +++ b/src/build/incrementality.go @@ -172,10 +172,7 @@ func ruleHash(state *core.BuildState, target *core.BuildTarget, runtime bool) [] h.Write([]byte(out)) } } - for _, licence := range target.Licences { - h.Write([]byte(licence)) - } - + h.Write([]byte(target.Licence)) for _, output := range target.OptionalOutputs { h.Write([]byte(output)) } diff --git a/src/build/incrementality_test.go b/src/build/incrementality_test.go index dfb35d8ac..b9553b82e 100644 --- a/src/build/incrementality_test.go +++ b/src/build/incrementality_test.go @@ -41,7 +41,7 @@ var KnownFields = map[string]bool{ "PostBuildHash": true, "outputs": true, "namedOutputs": true, - "Licences": true, + "Licence": true, "Sandbox": true, "Tools": true, "namedTools": true, diff --git a/src/core/BUILD b/src/core/BUILD index 40a3bce6b..2e2487779 100644 --- a/src/core/BUILD +++ b/src/core/BUILD @@ -13,6 +13,7 @@ go_library( "///third_party/go/github.com_coreos_go-semver//semver", "///third_party/go/github.com_google_shlex//:shlex", "///third_party/go/github.com_peterebden_go-deferred-regex//:go-deferred-regex", + "///third_party/go/github.com_peterebden_go-spdx_v2//spdxexp", "///third_party/go/github.com_pkg_xattr//:xattr", "///third_party/go/github.com_please-build_gcfg//:gcfg", "///third_party/go/github.com_please-build_gcfg//types", diff --git a/src/core/build_target.go b/src/core/build_target.go index 82cbd1339..eda709bfe 100644 --- a/src/core/build_target.go +++ b/src/core/build_target.go @@ -11,6 +11,7 @@ import ( "sync/atomic" "time" + "github.com/peterebden/go-spdx/v2/spdxexp" "golang.org/x/sync/errgroup" "github.com/thought-machine/please/src/fs" @@ -1887,27 +1888,18 @@ func (target *BuildTarget) PackageDir() string { } // CheckLicences checks the target's licences against the accepted/rejected list. -// It returns the licence that was accepted and an error if it did not match. +// It returns the licence expression that was accepted and an error if it did not match. func (target *BuildTarget) CheckLicences(config *Configuration) (string, error) { - if len(target.Licences) == 0 { + if target.Licence == "" || len(config.Licences.Accept) == 0 { return "", nil } - for _, licence := range target.Licences { - for _, reject := range config.Licences.Reject { - if strings.EqualFold(reject, licence) { - return "", fmt.Errorf("Target %s is licensed %s, which is explicitly rejected for this repository", target.Label, licence) - } - } - for _, accept := range config.Licences.Accept { - if strings.EqualFold(accept, licence) { - return licence, nil // Note licences are assumed to be an 'or', ie. any one of them can be accepted. - } - } - } - if len(config.Licences.Accept) > 0 { - return "", fmt.Errorf("None of the licences for %s are accepted in this repository: %s", target.Label, strings.Join(target.Licences, ", ")) + accepted, err := spdxexp.ExpressionSatisfies(target.Licence, config.Licences.Accept) + if err != nil { + return "", fmt.Errorf("Target %s has invalid licence %s: %s", target, target.Licence, err) + } else if accepted == "" { + return "", fmt.Errorf("The licences for %s are not accepted in this repository: %s", target, target.Licence) } - return "", nil + return accepted, nil } // BuildTargets makes a slice of build targets sortable by their labels. diff --git a/src/core/build_target_test.go b/src/core/build_target_test.go index d9a6cea38..97a672eb2 100644 --- a/src/core/build_target_test.go +++ b/src/core/build_target_test.go @@ -954,19 +954,24 @@ func TestIsTool(t *testing.T) { func TestCheckLicences(t *testing.T) { config := DefaultConfiguration() - config.Licences.Accept = []string{"BSD"} + config.Licences.Accept = []string{"BSD", "Apache-2.0"} config.Licences.Reject = []string{"GPL"} target := makeTarget1("//src/core/test_data/project", "PUBLIC") - target.Licences = []string{"BSD", "GPL"} + target.Licence = "BSD OR GPL" accepted, err := target.CheckLicences(config) assert.NoError(t, err) assert.Equal(t, "BSD", accepted) - target.Licences = []string{"MIT", "GPL"} + target.Licence = "MIT OR GPL" accepted, err = target.CheckLicences(config) assert.Error(t, err) assert.Equal(t, "", accepted) + + target.Licence = "BSD AND Apache-2.0 OR GPL" + accepted, err = target.CheckLicences(config) + assert.NoError(t, err) + assert.Equal(t, "BSD AND Apache-2.0", accepted) } func makeTarget1(label, visibility string, deps ...*BuildTarget) *BuildTarget { diff --git a/src/core/stamp.go b/src/core/stamp.go index e1e71e4b5..dd2ae667f 100644 --- a/src/core/stamp.go +++ b/src/core/stamp.go @@ -22,11 +22,15 @@ func StampFile(config *Configuration, target *BuildTarget) []byte { func populateStampInfo(config *Configuration, target *BuildTarget, info *stampInfo) { accepted, _ := target.CheckLicences(config) - info.Targets[target.Label] = targetInfo{ - Licences: target.Licences, + ti := targetInfo{ + Licence: target.Licence, AcceptedLicence: accepted, Labels: target.Labels, } + if target.Licence != "" { + ti.Licences = []string{target.Licence} + } + info.Targets[target.Label] = ti for _, dep := range target.Dependencies() { if _, present := info.Targets[dep.Label]; !present { populateStampInfo(config, dep, info) @@ -40,6 +44,7 @@ type stampInfo struct { type targetInfo struct { Labels []string `json:"labels,omitempty"` - Licences []string `json:"licences,omitempty"` + Licence string `json:"licence,omitempty"` + Licences []string `json:"licences,omitempty"` // Deprecated in favour of Licence AcceptedLicence string `json:"accepted_licence,omitempty"` } diff --git a/src/core/stamp_test.go b/src/core/stamp_test.go index 1747b9230..d3e8d71e7 100644 --- a/src/core/stamp_test.go +++ b/src/core/stamp_test.go @@ -14,14 +14,14 @@ func TestStampFile(t *testing.T) { t3 := NewBuildTarget(ParseBuildLabel("//third_party/go:errors", "")) t1.AddLabel("go") t3.AddLabel("go_get:github.com/pkg/errors") - t3.AddLicence("bsd-2-clause") + t3.Licence = "bsd-2-clause" t1.AddDependency(t2.Label) t1.resolveDependency(t2.Label, t2) t1.AddDependency(t3.Label) t1.resolveDependency(t3.Label, t3) t2.AddDependency(t3.Label) t2.resolveDependency(t3.Label, t3) - expected := []byte(`{ + const expected = `{ "targets": { "//src/core:core": { "labels": [ @@ -33,12 +33,14 @@ func TestStampFile(t *testing.T) { "labels": [ "go_get:github.com/pkg/errors" ], + "licence": "bsd-2-clause", "licences": [ "bsd-2-clause" ], - "accepted_licence": "bsd-2-clause" + "accepted_licence": "BSD-2-Clause" } } -}`) - assert.Equal(t, expected, StampFile(config, t1)) +}` + + assert.Equal(t, expected, string(StampFile(config, t1))) } diff --git a/src/parse/asp/builtins.go b/src/parse/asp/builtins.go index 276957a39..71a3fde24 100644 --- a/src/parse/asp/builtins.go +++ b/src/parse/asp/builtins.go @@ -1315,13 +1315,16 @@ func getEntryPoints(s *scope, args []pyObject) pyObject { // addLicence adds a licence to a target. func addLicence(s *scope, args []pyObject) pyObject { target := getTargetPost(s, string(args[0].(pyString))) - target.AddLicence(string(args[1].(pyString))) + if target.Licence != "" { + target.Licence += " OR " + } + target.Licence += string(args[1].(pyString)) return None } // getLicences returns the licences for a single target. func getLicences(s *scope, args []pyObject) pyObject { - return fromStringList(getTargetPost(s, string(args[0].(pyString))).Licences) + return pyList{pyString(getTargetPost(s, string(args[0].(pyString))).Licence)} } // getCommand gets the command of a target, optionally for a configuration. diff --git a/src/parse/asp/targets.go b/src/parse/asp/targets.go index f881d20a9..6287364b0 100644 --- a/src/parse/asp/targets.go +++ b/src/parse/asp/targets.go @@ -274,27 +274,29 @@ func populateTarget(s *scope, t *core.BuildTarget, args []pyObject) { addStrings(s, "labels", args[labelsBuildRuleArgIdx], t.AddLabel) addStrings(s, "hashes", args[hashesBuildRuleArgIdx], t.AddHash) addStrings(s, "requires", args[requiresBuildRuleArgIdx], t.AddRequire) - if expr, ok := args[licencesBuildRuleArgIdx].(pyString); ok { - target.Licence = string(expr) - } else { - // TODO(v18): Remove all this once strings are the only option. - s.Assert(!s.state.config.FeatureFlags.SPDXLicencesOnly, "The licences argument must be a string") - l, ok := asList(args[licencesBuildRuleArgIdx]) - s.Assert(ok, "The licences argument must be a string or list (was %s)", l.Type()) - if len(l) == 1 { - // minor optimisation: avoid allocating a slice etc for the overwhelmingly common case of a single licence - str, ok := l[0].(pyString) - s.Assert(ok, "Items in the licences argument must be strings (was %s)", s.Type()) - target.Licence = string(str) + if arg := args[licencesBuildRuleArgIdx]; arg != None { + if expr, ok := arg.(pyString); ok { + t.Licence = string(expr) } else { - l2 := make([]string, len(l)) - for i, x := range l { - str, ok := x.(pyString) - s.Assert(ok, "Items in the licences argument must be strings (was %s)", s.Type()) - l2[i] = string(str) + // TODO(v18): Remove all this once strings are the only option. + s.Assert(!s.state.Config.FeatureFlags.SPDXLicencesOnly, "The licences argument must be a string") + l, ok := asList(arg) + s.Assert(ok, "The licences argument must be a string or list (was %s)", arg.Type()) + if len(l) == 1 { + // minor optimisation: avoid allocating a slice etc for the overwhelmingly common case of a single licence + str, ok := l[0].(pyString) + s.Assert(ok, "Items in the licences argument must be strings (was %s)", str.Type()) + t.Licence = string(str) + } else { + l2 := make([]string, len(l)) + for i, x := range l { + str, ok := x.(pyString) + s.Assert(ok, "Items in the licences argument must be strings (was %s)", str.Type()) + l2[i] = string(str) + } + // Passing a list to Please is implicitly a series of alternative licences + t.Licence = strings.Join(l2, " OR ") } - // Passing a list to Please is implicitly a series of alternative licences - target.Licence = strings.Join(l2, " OR ") } } if vis, ok := asList(args[visibilityBuildRuleArgIdx]); ok && len(vis) != 0 { diff --git a/third_party/go/BUILD b/third_party/go/BUILD index 7493ef482..9e63b34db 100644 --- a/third_party/go/BUILD +++ b/third_party/go/BUILD @@ -689,7 +689,12 @@ go_repo( go_repo( licences = ["MIT"], - module = "github.com/github/go-spdx", - version = "v2.3.1", + module = "github.com/peterebden/go-spdx/v2", + version = "v2.4.3", ) +go_repo( + licences = ["MIT"], + module = "github.com/github/go-spdx/v2", + version = "v2.3.1", +) From 1d75ff462ad84f2b8005a20635d249c27c0fd4ca Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Sat, 17 Aug 2024 15:44:22 +0100 Subject: [PATCH 3/9] Lots of string replacements --- src/core/build_target.go | 4 ++-- src/parse/asp/builtins.go | 2 +- src/parse/asp/targets.go | 7 ++++--- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/core/build_target.go b/src/core/build_target.go index eda709bfe..4fb3106c2 100644 --- a/src/core/build_target.go +++ b/src/core/build_target.go @@ -1890,12 +1890,12 @@ func (target *BuildTarget) PackageDir() string { // CheckLicences checks the target's licences against the accepted/rejected list. // It returns the licence expression that was accepted and an error if it did not match. func (target *BuildTarget) CheckLicences(config *Configuration) (string, error) { - if target.Licence == "" || len(config.Licences.Accept) == 0 { + if target.Licence == "" || (len(config.Licences.Accept) == 0 && len(config.Licences.Reject) == 0) { return "", nil } accepted, err := spdxexp.ExpressionSatisfies(target.Licence, config.Licences.Accept) if err != nil { - return "", fmt.Errorf("Target %s has invalid licence %s: %s", target, target.Licence, err) + return "", fmt.Errorf("Target %s has invalid licence '%s': %s", target, target.Licence, err) } else if accepted == "" { return "", fmt.Errorf("The licences for %s are not accepted in this repository: %s", target, target.Licence) } diff --git a/src/parse/asp/builtins.go b/src/parse/asp/builtins.go index 71a3fde24..8c6a839a6 100644 --- a/src/parse/asp/builtins.go +++ b/src/parse/asp/builtins.go @@ -1318,7 +1318,7 @@ func addLicence(s *scope, args []pyObject) pyObject { if target.Licence != "" { target.Licence += " OR " } - target.Licence += string(args[1].(pyString)) + target.Licence += strings.TrimSpace(strings.ReplaceAll(string(args[1].(pyString)), " ", "-")) return None } diff --git a/src/parse/asp/targets.go b/src/parse/asp/targets.go index 6287364b0..58fa39c2d 100644 --- a/src/parse/asp/targets.go +++ b/src/parse/asp/targets.go @@ -276,7 +276,8 @@ func populateTarget(s *scope, t *core.BuildTarget, args []pyObject) { addStrings(s, "requires", args[requiresBuildRuleArgIdx], t.AddRequire) if arg := args[licencesBuildRuleArgIdx]; arg != None { if expr, ok := arg.(pyString); ok { - t.Licence = string(expr) + // TODO(v18): Remove the replace here once all licences are expected to be SPDX expressions + t.Licence = strings.ReplaceAll(string(expr), " ", "-") } else { // TODO(v18): Remove all this once strings are the only option. s.Assert(!s.state.Config.FeatureFlags.SPDXLicencesOnly, "The licences argument must be a string") @@ -286,13 +287,13 @@ func populateTarget(s *scope, t *core.BuildTarget, args []pyObject) { // minor optimisation: avoid allocating a slice etc for the overwhelmingly common case of a single licence str, ok := l[0].(pyString) s.Assert(ok, "Items in the licences argument must be strings (was %s)", str.Type()) - t.Licence = string(str) + t.Licence = strings.ReplaceAll(string(str), " ", "-") } else { l2 := make([]string, len(l)) for i, x := range l { str, ok := x.(pyString) s.Assert(ok, "Items in the licences argument must be strings (was %s)", str.Type()) - l2[i] = string(str) + l2[i] = strings.ReplaceAll(string(str), " ", "-") } // Passing a list to Please is implicitly a series of alternative licences t.Licence = strings.Join(l2, " OR ") From 905852376625ee161b46ef907e1c7c740a6d0af5 Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Sat, 17 Aug 2024 15:53:00 +0100 Subject: [PATCH 4/9] Deal with invalid original licences --- src/core/build_target.go | 2 +- src/core/config.go | 31 +++++++++++++++++++++++++++---- 2 files changed, 28 insertions(+), 5 deletions(-) diff --git a/src/core/build_target.go b/src/core/build_target.go index 4fb3106c2..81f0a221e 100644 --- a/src/core/build_target.go +++ b/src/core/build_target.go @@ -1893,7 +1893,7 @@ func (target *BuildTarget) CheckLicences(config *Configuration) (string, error) if target.Licence == "" || (len(config.Licences.Accept) == 0 && len(config.Licences.Reject) == 0) { return "", nil } - accepted, err := spdxexp.ExpressionSatisfies(target.Licence, config.Licences.Accept) + accepted, err := spdxexp.ExpressionSatisfies(target.Licence, config.AcceptedLicences()) if err != nil { return "", fmt.Errorf("Target %s has invalid licence '%s': %s", target, target.Licence, err) } else if accepted == "" { diff --git a/src/core/config.go b/src/core/config.go index 864fb73a4..d961be7f9 100644 --- a/src/core/config.go +++ b/src/core/config.go @@ -11,6 +11,7 @@ import ( "path/filepath" "reflect" "runtime" + "slices" "sort" "strconv" "strings" @@ -19,6 +20,7 @@ import ( "github.com/coreos/go-semver/semver" "github.com/google/shlex" + "github.com/peterebden/go-spdx/v2/spdxexp" "github.com/please-build/gcfg" gcfgtypes "github.com/please-build/gcfg/types" "github.com/thought-machine/go-flags" @@ -684,10 +686,6 @@ type Configuration struct { Bazel struct { Compatibility bool `help:"Activates limited Bazel compatibility mode. When this is active several rule arguments are available under different names (e.g. compiler_flags -> copts etc), the WORKSPACE file is interpreted, Makefile-style replacements like $< and $@ are made in genrule commands, etc.\nNote that Skylark is not generally supported and many aspects of compatibility are fairly superficial; it's unlikely this will work for complex setups of either tool." var:"BAZEL_COMPATIBILITY"` } `help:"Bazel is an open-sourced version of Google's internal build tool. Please draws a lot of inspiration from the original tool although the two have now diverged in various ways.\nNonetheless, if you've used Bazel, you will likely find Please familiar."` - - // buildEnvStored is a cached form of BuildEnv. - buildEnvStored *storedBuildEnv - FeatureFlags struct { SPDXLicencesOnly bool `help:"Licences on build targets can only be strings containing SPDX expressions, not lists."` } `help:"Flags controlling preview features for the next release. Typically these config options gate breaking changes and only have a lifetime of one major release."` @@ -696,6 +694,13 @@ type Configuration struct { Timeout cli.Duration `help:"timeout for pushing to the gateway. Defaults to 2 seconds." ` PushHostInfo bool `help:"Whether to push host info"` } `help:"Settings for collecting metrics."` + + // buildEnvStored is a cached form of BuildEnv. + buildEnvStored *storedBuildEnv + + // acceptedLicences is a slightly processed form of Licences.Accept + acceptedLicences []string + acceptedLicencesOnce sync.Once } // An Alias represents aliases in the config. @@ -845,6 +850,24 @@ func (config *Configuration) getBuildEnv(includePath bool, includeUnsafe bool) B return env } +// AcceptedLicences returns the set of licences accepted in this repository +func (config *Configuration) AcceptedLicences() []string { + config.acceptedLicencesOnce.Do(func() { + config.acceptedLicences = make([]string, len(config.Licences.Accept)) + for i, licence := range config.Licences.Accept { + config.acceptedLicences[i] = strings.ReplaceAll(strings.ReplaceAll(licence, " ", "-"), ",", "-") + } + if ok, invalid := spdxexp.ValidateLicenses(config.acceptedLicences); !ok { + log.Warning("The following accepted licences are not valid licence names: %s", strings.Join(invalid, ", ")) + // We now have to remove these, otherwise the SPDX parser gets upset later on + for _, licence := range invalid { + config.acceptedLicences = slices.DeleteFunc(config.acceptedLicences, func(l string) bool { return l == licence }) + } + } + }) + return config.acceptedLicences +} + // TagsToFields returns a map of string represent the properties of CONFIG object to the config Structfield func (config *Configuration) TagsToFields() map[string]reflect.StructField { tags := make(map[string]reflect.StructField) From 29be85f83aa5f7999a0cfbfd12b8c76e97d5e6f9 Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Sat, 17 Aug 2024 15:58:35 +0100 Subject: [PATCH 5/9] DO better licence normalisation --- src/parse/asp/BUILD | 1 + src/parse/asp/builtins.go | 5 ++++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/parse/asp/BUILD b/src/parse/asp/BUILD index 486fa0fef..239273939 100644 --- a/src/parse/asp/BUILD +++ b/src/parse/asp/BUILD @@ -12,6 +12,7 @@ go_library( "///third_party/go/github.com_Masterminds_semver_v3//:v3", "///third_party/go/github.com_manifoldco_promptui//:promptui", "///third_party/go/github.com_please-build_gcfg//types", + "///third_party/go/github.com_peterebden_go-deferred-regex//:go-deferred-regex", "//src/cli", "//src/cli/logging", "//src/cmap", diff --git a/src/parse/asp/builtins.go b/src/parse/asp/builtins.go index 8c6a839a6..60b43a4ce 100644 --- a/src/parse/asp/builtins.go +++ b/src/parse/asp/builtins.go @@ -15,6 +15,7 @@ import ( "github.com/Masterminds/semver/v3" "github.com/manifoldco/promptui" + "github.com/peterebden/go-deferred-regex" "github.com/thought-machine/please/src/cli" "github.com/thought-machine/please/src/core" @@ -24,6 +25,8 @@ import ( // A nativeFunc is a function that implements a builtin function natively. type nativeFunc func(*scope, []pyObject) pyObject +var normaliseLicence = deferredregex.DeferredRegex{Re: `[ (),]`} + // registerBuiltins sets up the "special" builtins that map to native code. func registerBuiltins(s *scope) { const varargs = true @@ -1318,7 +1321,7 @@ func addLicence(s *scope, args []pyObject) pyObject { if target.Licence != "" { target.Licence += " OR " } - target.Licence += strings.TrimSpace(strings.ReplaceAll(string(args[1].(pyString)), " ", "-")) + target.Licence += normaliseLicence.ReplaceAllString(strings.TrimSpace(string(args[1].(pyString))), "-") return None } From 88ab21e701fa749dcdceee4d3a571547b6175dc7 Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Sat, 17 Aug 2024 16:04:16 +0100 Subject: [PATCH 6/9] fix test --- src/build/build_step_test.go | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/build/build_step_test.go b/src/build/build_step_test.go index e414e89f7..77ed2b399 100644 --- a/src/build/build_step_test.go +++ b/src/build/build_step_test.go @@ -290,22 +290,18 @@ func TestLicenceEnforcement(t *testing.T) { // A license (non case sensitive) that is not in the list of accepted licenses will panic. assert.Panics(t, func() { - target.Licences = append(target.Licences, "Bsd") + target.Licence = "Bsd" checkLicences(state, target) }, "A target with a non-accepted licence will panic") - // Accepting bsd should resolve the panic - state.Config.Licences.Accept = append(state.Config.Licences.Accept, "BSD") - checkLicences(state, target) - // Now construct a new "bad" target. state, target = newState("//pkg:bad") state.Config.Licences.Reject = append(state.Config.Licences.Reject, "gpl") state.Config.Licences.Accept = append(state.Config.Licences.Accept, "mit") // Adding an explicitly rejected licence should panic no matter what. - target.Licences = append(target.Licences, "GPL") assert.Panics(t, func() { + target.Licence = "GPL" checkLicences(state, target) }, "Trying to add GPL should panic (case insensitive)") } From 24d7b41cbd960a474c9fa719095bc38a4bf7b6fa Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Sat, 17 Aug 2024 16:08:07 +0100 Subject: [PATCH 7/9] Add get_licence and set_licence functions --- docs/lexicon.html | 78 +++++++++++++++++++++++++++++++++++++++ rules/builtins.build_defs | 4 ++ src/parse/asp/builtins.go | 14 +++++++ 3 files changed, 96 insertions(+) diff --git a/docs/lexicon.html b/docs/lexicon.html index 351d56517..a0d33864c 100644 --- a/docs/lexicon.html +++ b/docs/lexicon.html @@ -951,6 +951,9 @@

Adds a new licence to a target. The assumption (as usual) is that if multiple are added, they are options, so any one can be accepted.

+

+ Deprecated in favour of set_licence. +

@@ -980,6 +983,45 @@

+
+

+ set_licence +

+ + set_licence(target, licence) + +

+ Sets the target's licence to the given SPDX expression. Note that no normalisation is done on it. +

+ +
+

+ + + + + + + + + + + + + + + + + + + + + + +
ArgumentDefaultType
targetstrLabel of the target to add the licence to.
licencestrSPDX expression defining licensing of the target
+
+ +

get_licences @@ -990,6 +1032,9 @@

Returns all the licences that are currently known to apply to a target.

+

+ Deprecated in favour of get_licence. +

@@ -1013,6 +1058,39 @@

+
+

+ get_licence +

+ + get_licence(target) + +

+ Returns the licence expression of the given target. +

+ +
+

+ + + + + + + + + + + + + + + + +
ArgumentDefaultType
targetstrLabel of the target to get the licence expression for.
+
+

+

package diff --git a/rules/builtins.build_defs b/rules/builtins.build_defs index cffabf11e..a79a228d2 100644 --- a/rules/builtins.build_defs +++ b/rules/builtins.build_defs @@ -266,8 +266,12 @@ def add_entry_point(target:str, name:str, out:str): def get_entry_points(target:str) -> dict: pass def add_licence(target:str, licence:str): + """Deprecated in favour of set_licence""" +def set_licence(target:str, licence: str): pass def get_licences(target:str): + """Deprecated in favour of get_licence""" +def get_licence(target:str): pass def get_command(target:str, config:str=''): pass diff --git a/src/parse/asp/builtins.go b/src/parse/asp/builtins.go index 60b43a4ce..ad2cc983b 100644 --- a/src/parse/asp/builtins.go +++ b/src/parse/asp/builtins.go @@ -73,7 +73,9 @@ func registerBuiltins(s *scope) { setNativeCode(s, "add_entry_point", addEntryPoint) setNativeCode(s, "get_entry_points", getEntryPoints) setNativeCode(s, "add_licence", addLicence) + setNativeCode(s, "set_licence", setLicence) setNativeCode(s, "get_licences", getLicences) + setNativeCode(s, "get_licence", getLicence) setNativeCode(s, "get_command", getCommand) setNativeCode(s, "set_command", setCommand) setNativeCode(s, "json", valueAsJSON) @@ -1325,11 +1327,23 @@ func addLicence(s *scope, args []pyObject) pyObject { return None } +// setLicence sets the target's licence to the given string. Note that it must be a valid SPDX identifier. +func setLicence(s *scope, args []pyObject) pyObject { + target := getTargetPost(s, string(args[0].(pyString))) + target.Licence += string(args[1].(pyString)) + return None +} + // getLicences returns the licences for a single target. func getLicences(s *scope, args []pyObject) pyObject { return pyList{pyString(getTargetPost(s, string(args[0].(pyString))).Licence)} } +// getLicence returns the licence for a single target. +func getLicences(s *scope, args []pyObject) pyObject { + return pyString(getTargetPost(s, string(args[0].(pyString))).Licence) +} + // getCommand gets the command of a target, optionally for a configuration. func getCommand(s *scope, args []pyObject) pyObject { target := getTargetPost(s, string(args[0].(pyString))) From 8c8fe822b590119a27b764f62dc732138f3187fe Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Sat, 17 Aug 2024 16:12:45 +0100 Subject: [PATCH 8/9] add assertions --- src/parse/asp/builtins.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/parse/asp/builtins.go b/src/parse/asp/builtins.go index ad2cc983b..a9cd2d480 100644 --- a/src/parse/asp/builtins.go +++ b/src/parse/asp/builtins.go @@ -1319,6 +1319,7 @@ func getEntryPoints(s *scope, args []pyObject) pyObject { // addLicence adds a licence to a target. func addLicence(s *scope, args []pyObject) pyObject { + s.Assert(!s.state.Config.FeatureFlags.SPDXLicencesOnly, "The add_licence builtin has been replaced with set_licence") target := getTargetPost(s, string(args[0].(pyString))) if target.Licence != "" { target.Licence += " OR " @@ -1336,11 +1337,12 @@ func setLicence(s *scope, args []pyObject) pyObject { // getLicences returns the licences for a single target. func getLicences(s *scope, args []pyObject) pyObject { + s.Assert(!s.state.Config.FeatureFlags.SPDXLicencesOnly, "The get_licences builtin has been replaced with get_licence") return pyList{pyString(getTargetPost(s, string(args[0].(pyString))).Licence)} } // getLicence returns the licence for a single target. -func getLicences(s *scope, args []pyObject) pyObject { +func getLicence(s *scope, args []pyObject) pyObject { return pyString(getTargetPost(s, string(args[0].(pyString))).Licence) } From 8dce30da4d40174efe5c7f9c83235543950828d5 Mon Sep 17 00:00:00 2001 From: Peter Ebden Date: Sat, 17 Aug 2024 16:40:08 +0100 Subject: [PATCH 9/9] Appease linters --- docs/BUILD | 4 ++-- src/core/config.go | 26 +++++++++++++++++--------- 2 files changed, 19 insertions(+), 11 deletions(-) diff --git a/docs/BUILD b/docs/BUILD index ea493fbab..d941af922 100644 --- a/docs/BUILD +++ b/docs/BUILD @@ -58,8 +58,8 @@ genrule( # Plugin versions to pull the docs from plugins = { "python": "v1.7.0", - "java": "v0.4.0", - "go": "v1.20.1", + "java": "v0.4.1", + "go": "v1.21.1", "cc": "v0.4.0", "shell": "v0.2.0", "go-proto": "v0.3.0", diff --git a/src/core/config.go b/src/core/config.go index d961be7f9..5e6ad380c 100644 --- a/src/core/config.go +++ b/src/core/config.go @@ -370,7 +370,10 @@ func defaultPathIfExists(conf *string, dir, file string) { // DefaultConfiguration returns the default configuration object with no overrides. // N.B. Slice fields are not populated by this (since it interferes with reading them) func DefaultConfiguration() *Configuration { - config := Configuration{buildEnvStored: &storedBuildEnv{}} + config := Configuration{ + buildEnvStored: &storedBuildEnv{}, + acceptedLicences: &storedAcceptedLicences{}, + } config.Please.SelfUpdate = true config.Please.Autoclean = true config.Please.DownloadLocation = "https://get.please.build" @@ -699,8 +702,7 @@ type Configuration struct { buildEnvStored *storedBuildEnv // acceptedLicences is a slightly processed form of Licences.Accept - acceptedLicences []string - acceptedLicencesOnce sync.Once + acceptedLicences *storedAcceptedLicences } // An Alias represents aliases in the config. @@ -748,6 +750,11 @@ type storedBuildEnv struct { Once sync.Once } +type storedAcceptedLicences struct { + Licences []string + Once sync.Once +} + // Hash returns a hash of the parts of this configuration that affect building targets in general. // Most parts are considered not to (e.g. cache settings) or affect specific targets (e.g. changing // tool paths which get accounted for on the targets that use them). @@ -852,20 +859,21 @@ func (config *Configuration) getBuildEnv(includePath bool, includeUnsafe bool) B // AcceptedLicences returns the set of licences accepted in this repository func (config *Configuration) AcceptedLicences() []string { - config.acceptedLicencesOnce.Do(func() { - config.acceptedLicences = make([]string, len(config.Licences.Accept)) + config.acceptedLicences.Once.Do(func() { + l := make([]string, len(config.Licences.Accept)) for i, licence := range config.Licences.Accept { - config.acceptedLicences[i] = strings.ReplaceAll(strings.ReplaceAll(licence, " ", "-"), ",", "-") + l[i] = strings.ReplaceAll(strings.ReplaceAll(licence, " ", "-"), ",", "-") } - if ok, invalid := spdxexp.ValidateLicenses(config.acceptedLicences); !ok { + if ok, invalid := spdxexp.ValidateLicenses(l); !ok { log.Warning("The following accepted licences are not valid licence names: %s", strings.Join(invalid, ", ")) // We now have to remove these, otherwise the SPDX parser gets upset later on for _, licence := range invalid { - config.acceptedLicences = slices.DeleteFunc(config.acceptedLicences, func(l string) bool { return l == licence }) + l = slices.DeleteFunc(l, func(l string) bool { return l == licence }) } } + config.acceptedLicences.Licences = l }) - return config.acceptedLicences + return config.acceptedLicences.Licences } // TagsToFields returns a map of string represent the properties of CONFIG object to the config Structfield