From 15ec6e65e7c9b6ea1ca7cf119b0097e676ef8dfb Mon Sep 17 00:00:00 2001 From: Samuel Littley Date: Mon, 28 Jul 2025 15:04:57 +0100 Subject: [PATCH 1/7] Add support for suppressing SRCS env variables (in the case of rules with large numbers of sources) --- rules/builtins.build_defs | 4 ++-- src/core/build_env.go | 24 ++++++++++++++---------- src/core/build_target.go | 3 +++ src/parse/asp/targets.go | 2 ++ 4 files changed, 21 insertions(+), 12 deletions(-) diff --git a/rules/builtins.build_defs b/rules/builtins.build_defs index a8c9fcfd2e..ddf1816674 100644 --- a/rules/builtins.build_defs +++ b/rules/builtins.build_defs @@ -13,7 +13,7 @@ def build_rule(name:str, cmd:str|dict='', test_cmd:str|dict='', debug_cmd:str='' test_outputs:list=None, system_srcs:list=None, stamp:bool=False, tag:str='', optional_outs:list=None, progress:bool=False, size:str=None, _urls:list=None, internal_deps:list=None, pass_env:list=None, local:bool=False, output_dirs:list=[], exit_on_error:bool=CONFIG.EXIT_ON_ERROR, entry_points:dict={}, env:dict={}, _file_content:str=None, - _subrepo:bool=False, no_test_coverage:bool=False): + _subrepo:bool=False, no_test_coverage:bool=False, no_src_env_vars:bool=False): pass def chr(i:int) -> str: @@ -285,7 +285,7 @@ def licenses(licences): def json(value, pretty=False) -> str: """Returns a JSON-formatted representation of a plz value. - + If pretty is true, the JSON will be pretty-printed with 2 spaces of indentation per level. """ pass diff --git a/src/core/build_env.go b/src/core/build_env.go index 912c9bf6fd..d3013a6935 100644 --- a/src/core/build_env.go +++ b/src/core/build_env.go @@ -77,7 +77,6 @@ func BuildEnvironment(state *BuildState, target *BuildTarget, tmpDir string) Bui env["TMP_DIR"] = tmpDir env["TMPDIR"] = tmpDir - env["SRCS"] = strings.Join(sources, " ") env["OUTS"] = strings.Join(outEnv, " ") env["HOME"] = tmpDir // Set a consistent hash seed for Python. Important for build determinism. @@ -87,16 +86,21 @@ func BuildEnvironment(state *BuildState, target *BuildTarget, tmpDir string) Bui if len(outEnv) == 1 { env["OUT"] = resolveOut(outEnv[0], tmpDir, target.Sandbox) } - // The SRC variable is only available on rules that have a single source file. - if len(sources) == 1 { - env["SRC"] = sources[0] - } - // Named source groups if the target declared any. - for name, srcs := range target.NamedSources { - paths := target.SourcePaths(state.Graph, srcs) - // TODO(macripps): Quote these to prevent spaces from breaking everything (consider joining with NUL or sth?) - env["SRCS_"+strings.ToUpper(name)] = strings.Join(paths, " ") + + if !target.NoSrcEnvVars { + env["SRCS"] = strings.Join(sources, " ") + // The SRC variable is only available on rules that have a single source file. + if len(sources) == 1 { + env["SRC"] = sources[0] + } + // Named source groups if the target declared any. + for name, srcs := range target.NamedSources { + paths := target.SourcePaths(state.Graph, srcs) + // TODO(macripps): Quote these to prevent spaces from breaking everything (consider joining with NUL or sth?) + env["SRCS_"+strings.ToUpper(name)] = strings.Join(paths, " ") + } } + // Named output groups similarly. for name, outs := range target.DeclaredNamedOutputs() { outs = target.GetTmpOutputAll(outs) diff --git a/src/core/build_target.go b/src/core/build_target.go index c8f83d78c3..3b02ab1a91 100644 --- a/src/core/build_target.go +++ b/src/core/build_target.go @@ -241,6 +241,9 @@ type BuildTarget struct { IsTextFile bool `print:"false"` // Marks that the target was added in a post-build function. AddedPostBuild bool `print:"false"` + // Skips generating SRCS environment variables. Used for targets which have too many sources to + // fit in an environment variable. + NoSrcEnvVars bool `name:"no_src_env_vars"` // If true, the interactive progress display will try to infer the target's progress // via some heuristics on its output. showProgress atomic.Bool `name:"progress"` diff --git a/src/parse/asp/targets.go b/src/parse/asp/targets.go index cd496a742a..24002dc54a 100644 --- a/src/parse/asp/targets.go +++ b/src/parse/asp/targets.go @@ -72,6 +72,7 @@ const ( fileContentArgIdx subrepoArgIdx noTestCoverageArgIdx + noSrcEnvVarsIdx ) // createTarget creates a new build target as part of build_rule(). @@ -112,6 +113,7 @@ func createTarget(s *scope, args []pyObject) *core.BuildTarget { target.IsTextFile = args[cmdBuildRuleArgIdx] == textFileCommand target.Local = isTruthy(localBuildRuleArgIdx) target.ExitOnError = isTruthy(exitOnErrorArgIdx) + target.NoSrcEnvVars = isTruthy(noSrcEnvVarsIdx) for _, o := range asStringList(s, args[outDirsBuildRuleArgIdx], "output_dirs") { target.AddOutputDirectory(o) } From efcf7a098dc2f2bb180a86ead5499a2ccc5831e2 Mon Sep 17 00:00:00 2001 From: Samuel Littley Date: Mon, 28 Jul 2025 15:08:44 +0100 Subject: [PATCH 2/7] Update version --- ChangeLog | 4 ++++ VERSION | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/ChangeLog b/ChangeLog index b99449cc6e..6f010e5366 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,7 @@ +Version 17.21.0 +--------------- + * Add support for suppressing SRCS env variables (#3393) + Version 17.20.0 --------------- * Bump arcat to v1.2.1, fixing a number of bugs relating to handling of ar archives (#3420) diff --git a/VERSION b/VERSION index 1f8b337687..d0abdafb91 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -17.20.0 +17.21.0 From 0689575356f7a7fca4e077c81c9cebdd7e1da09e Mon Sep 17 00:00:00 2001 From: Samuel Littley Date: Mon, 28 Jul 2025 15:45:40 +0100 Subject: [PATCH 3/7] Add new field to hash --- src/build/incrementality.go | 1 + src/build/incrementality_test.go | 1 + 2 files changed, 2 insertions(+) diff --git a/src/build/incrementality.go b/src/build/incrementality.go index 92307cef7f..678b8363dc 100644 --- a/src/build/incrementality.go +++ b/src/build/incrementality.go @@ -200,6 +200,7 @@ func ruleHash(state *core.BuildState, target *core.BuildTarget, runtime bool) [] hashBool(h, target.IsTextFile) hashBool(h, target.IsRemoteFile) hashBool(h, target.Local) + hashBool(h, target.NoSrcEnvVars) hashOptionalBool(h, target.ExitOnError) for _, require := range target.Requires { h.Write([]byte(require)) diff --git a/src/build/incrementality_test.go b/src/build/incrementality_test.go index dfb35d8ac8..bd5ca348f3 100644 --- a/src/build/incrementality_test.go +++ b/src/build/incrementality_test.go @@ -30,6 +30,7 @@ var KnownFields = map[string]bool{ "Commands": true, "NeedsTransitiveDependencies": true, "Local": true, + "NoSrcEnvVars": true, "OptionalOutputs": true, "OutputIsComplete": true, "Requires": true, From 0d39f85c4fb0d9eb21504b6da3c676173363eb74 Mon Sep 17 00:00:00 2001 From: Samuel Littley Date: Thu, 9 Oct 2025 11:22:15 +0100 Subject: [PATCH 4/7] Add source list files --- src/build/build_step.go | 7 ++++++ src/core/build_target.go | 54 +++++++++++++++++++++++++++++++++++++++- src/remote/action.go | 13 ++++++++++ 3 files changed, 73 insertions(+), 1 deletion(-) diff --git a/src/build/build_step.go b/src/build/build_step.go index b2c8e4c050..2686ba5580 100644 --- a/src/build/build_step.go +++ b/src/build/build_step.go @@ -606,6 +606,13 @@ func prepareSources(state *core.BuildState, graph *core.BuildGraph, target *core return err } } + if target.NoSrcEnvVars { + for slf := range target.SourceListFiles(graph) { + if err := fs.WriteFile(bytes.NewReader(slf.Content), filepath.Join(target.TmpDir(), slf.Dirname, slf.Filename), 0644); err != nil { + return err + } + } + } return nil } diff --git a/src/core/build_target.go b/src/core/build_target.go index 3b02ab1a91..6cf6e0b580 100644 --- a/src/core/build_target.go +++ b/src/core/build_target.go @@ -2,7 +2,9 @@ package core import ( "fmt" + "iter" "os" + "path" "path/filepath" "slices" "sort" @@ -242,7 +244,13 @@ type BuildTarget struct { // Marks that the target was added in a post-build function. AddedPostBuild bool `print:"false"` // Skips generating SRCS environment variables. Used for targets which have too many sources to - // fit in an environment variable. + // fit in an environment variable. If this is set, files will be generated in the build + // environment containing the lists of sources as follows: + // - _plz/srcs (equivalent to $SRCS) always + // - _plz/src (equivalent to $SRC) if the target has exactly one src + // - _plz/named_srcs/foo (equivalent to $SRCS_FOO) if the target has a named src called foo + // Note that in contrast to the environment variables which are space separated, these files are + // newline separated. NoSrcEnvVars bool `name:"no_src_env_vars"` // If true, the interactive progress display will try to infer the target's progress // via some heuristics on its output. @@ -1955,3 +1963,47 @@ func (slice BuildTargets) Less(i, j int) bool { func (slice BuildTargets) Swap(i, j int) { slice[i], slice[j] = slice[j], slice[i] } + +const sourceListFileDir = "_plz" +const sourceListFileNamedDir = "named_srcs" + +type SourceListFile struct { + Dirname string + Filename string + Content []byte +} + +func (target *BuildTarget) SourceListFiles(graph *BuildGraph) iter.Seq[*SourceListFile] { + return func(yield func(*SourceListFile) bool) { + sources := target.AllSourcePaths(graph) + + if !yield(&SourceListFile{ + Dirname: sourceListFileDir, + Filename: "srcs", + Content: []byte(strings.Join(sources, "\n")), + }) { + return + } + + if len(sources) == 1 { + if !yield(&SourceListFile{ + Dirname: sourceListFileDir, + Filename: "src", + Content: []byte(sources[0]), + }) { + return + } + } + + for name, srcs := range target.NamedSources { + paths := target.SourcePaths(graph, srcs) + if !yield(&SourceListFile{ + Dirname: path.Join(sourceListFileDir, sourceListFileNamedDir), + Filename: name, + Content: []byte(strings.Join(paths, "\n")), + }) { + return + } + } + } +} diff --git a/src/remote/action.go b/src/remote/action.go index 0b0daa59d5..72539cce5f 100644 --- a/src/remote/action.go +++ b/src/remote/action.go @@ -296,6 +296,19 @@ func (c *Client) uploadInputDir(ch chan<- *uploadinfo.Entry, target *core.BuildT Digest: entry.Digest.ToProto(), }) } + if target.NoSrcEnvVars { + for slf := range target.SourceListFiles(c.state.Graph) { + entry := uploadinfo.EntryFromBlob(slf.Content) + if ch != nil { + ch <- entry + } + d := b.Dir(slf.Dirname) + d.Files = append(d.Files, &pb.FileNode{ + Name: slf.Filename, + Digest: entry.Digest.ToProto(), + }) + } + } return b, nil } From df77c0f0761012b3614438e7538ba96d0c0e1a05 Mon Sep 17 00:00:00 2001 From: Samuel Littley Date: Thu, 9 Oct 2025 14:04:11 +0100 Subject: [PATCH 5/7] Add e2e tests --- test/source_list_files/BUILD | 35 +++++++++++++++++ test/source_list_files/test_repo/.plzconfig | 2 + test/source_list_files/test_repo/BUILD.test | 43 +++++++++++++++++++++ test/source_list_files/test_repo/a.txt | 0 test/source_list_files/test_repo/b.txt | 0 5 files changed, 80 insertions(+) create mode 100644 test/source_list_files/BUILD create mode 100644 test/source_list_files/test_repo/.plzconfig create mode 100644 test/source_list_files/test_repo/BUILD.test create mode 100644 test/source_list_files/test_repo/a.txt create mode 100644 test/source_list_files/test_repo/b.txt diff --git a/test/source_list_files/BUILD b/test/source_list_files/BUILD new file mode 100644 index 0000000000..b6d95ab7e9 --- /dev/null +++ b/test/source_list_files/BUILD @@ -0,0 +1,35 @@ +subinclude("//test/build_defs") + +please_repo_e2e_test( + name = "srcs_test", + plz_command = "plz build //:with_srcs", + expected_output = { + "plz-out/gen/with_srcs.txt": "a.txt\nb.txt", + }, + repo = "test_repo", +) + +please_repo_e2e_test( + name = "src_test", + plz_command = "plz build //:with_one_src", + expected_output = { + "plz-out/gen/with_one_src.txt": "a.txt", + }, + repo = "test_repo", +) + +please_repo_e2e_test( + name = "named_srcs_test", + plz_command = "plz build //:with_named_srcs", + expected_output = { + "plz-out/gen/with_named_srcs.txt": "a.txt\nb.txt", + }, + repo = "test_repo", +) + +please_repo_e2e_test( + name = "flag_not_set_test", + plz_command = "plz build //:with_flag_not_set", + repo = "test_repo", + expected_failure = True, +) diff --git a/test/source_list_files/test_repo/.plzconfig b/test/source_list_files/test_repo/.plzconfig new file mode 100644 index 0000000000..a5a48e3384 --- /dev/null +++ b/test/source_list_files/test_repo/.plzconfig @@ -0,0 +1,2 @@ +[Parse] +BuildFileName = BUILD.test diff --git a/test/source_list_files/test_repo/BUILD.test b/test/source_list_files/test_repo/BUILD.test new file mode 100644 index 0000000000..11f19da9bf --- /dev/null +++ b/test/source_list_files/test_repo/BUILD.test @@ -0,0 +1,43 @@ +build_rule( + name = "with_srcs", + srcs = [ + "a.txt", + "b.txt", + ], + outs = ["with_srcs.txt"], + cmd = "cat _plz/srcs > with_srcs.txt", + no_src_env_vars = True, +) + +build_rule( + name = "with_one_src", + srcs = [ + "a.txt", + ], + outs = ["with_one_src.txt"], + cmd = "cat _plz/src > with_one_src.txt", + no_src_env_vars = True, +) + +build_rule( + name = "with_named_srcs", + srcs = { + "foo": [ + "a.txt", + "b.txt", + ], + }, + outs = ["with_named_srcs.txt"], + cmd = "cat _plz/named_srcs/foo > with_named_srcs.txt", + no_src_env_vars = True, +) + +build_rule( + name = "with_flag_not_set", + srcs = [ + "a.txt", + "b.txt", + ], + outs = ["with_flag_not_set.txt"], + cmd = "cat _plz/srcs > with_flag_not_set.txt", +) diff --git a/test/source_list_files/test_repo/a.txt b/test/source_list_files/test_repo/a.txt new file mode 100644 index 0000000000..e69de29bb2 diff --git a/test/source_list_files/test_repo/b.txt b/test/source_list_files/test_repo/b.txt new file mode 100644 index 0000000000..e69de29bb2 From 312c6eb56e6f814561352a69e2455bc881d7beea Mon Sep 17 00:00:00 2001 From: Samuel Littley Date: Fri, 10 Oct 2025 10:20:19 +0100 Subject: [PATCH 6/7] Rename NoSrcEnvVars to SrcListFiles --- rules/builtins.build_defs | 2 +- src/build/build_step.go | 2 +- src/build/incrementality.go | 2 +- src/build/incrementality_test.go | 2 +- src/core/build_env.go | 2 +- src/core/build_target.go | 10 +++++----- src/parse/asp/targets.go | 4 ++-- src/remote/action.go | 2 +- test/source_list_files/test_repo/BUILD.test | 6 +++--- 9 files changed, 16 insertions(+), 16 deletions(-) diff --git a/rules/builtins.build_defs b/rules/builtins.build_defs index ddf1816674..5edca910d3 100644 --- a/rules/builtins.build_defs +++ b/rules/builtins.build_defs @@ -13,7 +13,7 @@ def build_rule(name:str, cmd:str|dict='', test_cmd:str|dict='', debug_cmd:str='' test_outputs:list=None, system_srcs:list=None, stamp:bool=False, tag:str='', optional_outs:list=None, progress:bool=False, size:str=None, _urls:list=None, internal_deps:list=None, pass_env:list=None, local:bool=False, output_dirs:list=[], exit_on_error:bool=CONFIG.EXIT_ON_ERROR, entry_points:dict={}, env:dict={}, _file_content:str=None, - _subrepo:bool=False, no_test_coverage:bool=False, no_src_env_vars:bool=False): + _subrepo:bool=False, no_test_coverage:bool=False, src_list_files:bool=False): pass def chr(i:int) -> str: diff --git a/src/build/build_step.go b/src/build/build_step.go index 2686ba5580..088654e856 100644 --- a/src/build/build_step.go +++ b/src/build/build_step.go @@ -606,7 +606,7 @@ func prepareSources(state *core.BuildState, graph *core.BuildGraph, target *core return err } } - if target.NoSrcEnvVars { + if target.SrcListFiles { for slf := range target.SourceListFiles(graph) { if err := fs.WriteFile(bytes.NewReader(slf.Content), filepath.Join(target.TmpDir(), slf.Dirname, slf.Filename), 0644); err != nil { return err diff --git a/src/build/incrementality.go b/src/build/incrementality.go index 678b8363dc..f233a23a82 100644 --- a/src/build/incrementality.go +++ b/src/build/incrementality.go @@ -200,7 +200,7 @@ func ruleHash(state *core.BuildState, target *core.BuildTarget, runtime bool) [] hashBool(h, target.IsTextFile) hashBool(h, target.IsRemoteFile) hashBool(h, target.Local) - hashBool(h, target.NoSrcEnvVars) + hashBool(h, target.SrcListFiles) hashOptionalBool(h, target.ExitOnError) for _, require := range target.Requires { h.Write([]byte(require)) diff --git a/src/build/incrementality_test.go b/src/build/incrementality_test.go index bd5ca348f3..a2ce75cdfb 100644 --- a/src/build/incrementality_test.go +++ b/src/build/incrementality_test.go @@ -30,7 +30,7 @@ var KnownFields = map[string]bool{ "Commands": true, "NeedsTransitiveDependencies": true, "Local": true, - "NoSrcEnvVars": true, + "SrcListFiles": true, "OptionalOutputs": true, "OutputIsComplete": true, "Requires": true, diff --git a/src/core/build_env.go b/src/core/build_env.go index d3013a6935..732b71b9fa 100644 --- a/src/core/build_env.go +++ b/src/core/build_env.go @@ -87,7 +87,7 @@ func BuildEnvironment(state *BuildState, target *BuildTarget, tmpDir string) Bui env["OUT"] = resolveOut(outEnv[0], tmpDir, target.Sandbox) } - if !target.NoSrcEnvVars { + if !target.SrcListFiles { env["SRCS"] = strings.Join(sources, " ") // The SRC variable is only available on rules that have a single source file. if len(sources) == 1 { diff --git a/src/core/build_target.go b/src/core/build_target.go index 6cf6e0b580..18dc8ed2f8 100644 --- a/src/core/build_target.go +++ b/src/core/build_target.go @@ -243,15 +243,15 @@ type BuildTarget struct { IsTextFile bool `print:"false"` // Marks that the target was added in a post-build function. AddedPostBuild bool `print:"false"` - // Skips generating SRCS environment variables. Used for targets which have too many sources to - // fit in an environment variable. If this is set, files will be generated in the build - // environment containing the lists of sources as follows: + // If true, skips generating environment variables for sources; instead files will be generated in + // the build environment containing the lists of sources as follows: // - _plz/srcs (equivalent to $SRCS) always // - _plz/src (equivalent to $SRC) if the target has exactly one src // - _plz/named_srcs/foo (equivalent to $SRCS_FOO) if the target has a named src called foo // Note that in contrast to the environment variables which are space separated, these files are - // newline separated. - NoSrcEnvVars bool `name:"no_src_env_vars"` + // newline separated. This can be used for targets which have too many sources to fit in an + // environment variable. + SrcListFiles bool `name:"src_list_files"` // If true, the interactive progress display will try to infer the target's progress // via some heuristics on its output. showProgress atomic.Bool `name:"progress"` diff --git a/src/parse/asp/targets.go b/src/parse/asp/targets.go index 24002dc54a..94c656830d 100644 --- a/src/parse/asp/targets.go +++ b/src/parse/asp/targets.go @@ -72,7 +72,7 @@ const ( fileContentArgIdx subrepoArgIdx noTestCoverageArgIdx - noSrcEnvVarsIdx + srcListFilesIdx ) // createTarget creates a new build target as part of build_rule(). @@ -113,7 +113,7 @@ func createTarget(s *scope, args []pyObject) *core.BuildTarget { target.IsTextFile = args[cmdBuildRuleArgIdx] == textFileCommand target.Local = isTruthy(localBuildRuleArgIdx) target.ExitOnError = isTruthy(exitOnErrorArgIdx) - target.NoSrcEnvVars = isTruthy(noSrcEnvVarsIdx) + target.SrcListFiles = isTruthy(srcListFilesIdx) for _, o := range asStringList(s, args[outDirsBuildRuleArgIdx], "output_dirs") { target.AddOutputDirectory(o) } diff --git a/src/remote/action.go b/src/remote/action.go index 72539cce5f..69f6eaa6eb 100644 --- a/src/remote/action.go +++ b/src/remote/action.go @@ -296,7 +296,7 @@ func (c *Client) uploadInputDir(ch chan<- *uploadinfo.Entry, target *core.BuildT Digest: entry.Digest.ToProto(), }) } - if target.NoSrcEnvVars { + if target.SrcListFiles { for slf := range target.SourceListFiles(c.state.Graph) { entry := uploadinfo.EntryFromBlob(slf.Content) if ch != nil { diff --git a/test/source_list_files/test_repo/BUILD.test b/test/source_list_files/test_repo/BUILD.test index 11f19da9bf..1caa4855c9 100644 --- a/test/source_list_files/test_repo/BUILD.test +++ b/test/source_list_files/test_repo/BUILD.test @@ -6,7 +6,7 @@ build_rule( ], outs = ["with_srcs.txt"], cmd = "cat _plz/srcs > with_srcs.txt", - no_src_env_vars = True, + src_list_files = True, ) build_rule( @@ -16,7 +16,7 @@ build_rule( ], outs = ["with_one_src.txt"], cmd = "cat _plz/src > with_one_src.txt", - no_src_env_vars = True, + src_list_files = True, ) build_rule( @@ -29,7 +29,7 @@ build_rule( }, outs = ["with_named_srcs.txt"], cmd = "cat _plz/named_srcs/foo > with_named_srcs.txt", - no_src_env_vars = True, + src_list_files = True, ) build_rule( From 704123b65a4dc33d7c3c560e01e9f604c93e96e4 Mon Sep 17 00:00:00 2001 From: Samuel Littley Date: Fri, 10 Oct 2025 12:06:58 +0100 Subject: [PATCH 7/7] Apply suggestions from code review Co-authored-by: Chris Novakovic --- ChangeLog | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ChangeLog b/ChangeLog index 6f010e5366..eb5044aa8f 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,6 +1,6 @@ Version 17.21.0 --------------- - * Add support for suppressing SRCS env variables (#3393) + * Expose build target source list via the filesystem instead of the environment with build_rule's src_list_files parameter (#3393) Version 17.20.0 ---------------