diff --git a/CHANGELOG.md b/CHANGELOG.md index 49fcf5d7e9..18234744bd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -82,7 +82,9 @@ END_UNRELEASED_TEMPLATE * (tests) No more coverage warnings are being printed if there are no sources. ([#2762](https://github.com/bazel-contrib/rules_python/issues/2762)) * (gazelle) Ancestor `conftest.py` files are added in addition to sibling `conftest.py`. - ([#3497](https://github.com/bazel-contrib/rules_python/issues/3497)) + ([#3497](https://github.com/bazel-contrib/rules_python/issues/3497)) Note + that this behavior can be reverted to the pre-VERSION_NEXT_FEATURE behavior by setting the new + `python_include_ancestor_conftest` directive to `false`. * (pypi) `pip_parse` no longer silently drops PEP 508 URL-based requirements (`pkg @ https://...`) when `extract_url_srcs=False` (the default for `pip_repository`). @@ -115,6 +117,12 @@ END_UNRELEASED_TEMPLATE {obj}`PyExecutableInfo.venv_python_exe`. * (tools/wheelmaker.py) Added support for URL requirements according to PEP 508 in Requires-Dist metadata. ([#3569](https://github.com/bazel-contrib/rules_python/pull/3569)) +* (gazelle) A new directive `python_include_ancestor_conftest` has been added. + When `false`, ancestor `conftest` targets are not automatically added to + {bzl:obj}`py_test` target dependencies. This `false` behavior is how things + were in `rules_python` before VERSION_NEXT_FEATURE. The default is `true`, as the prior behavior + was technically incorrect. + ([#3596](https://github.com/bazel-contrib/rules_python/pull/3596)) {#v1-8-4} ## [1.8.4] - 2026-02-10 diff --git a/gazelle/docs/annotations.md b/gazelle/docs/annotations.md index 728027ffda..b6eb96d2cd 100644 --- a/gazelle/docs/annotations.md +++ b/gazelle/docs/annotations.md @@ -116,6 +116,7 @@ deps = [ ``` +(annotation-include-pytest-conftest)= ## `include_pytest_conftest` :::{versionadded} 1.6.0 diff --git a/gazelle/docs/directives.md b/gazelle/docs/directives.md index c7936d539d..628dce5ae6 100644 --- a/gazelle/docs/directives.md +++ b/gazelle/docs/directives.md @@ -175,6 +175,11 @@ The Python-specific directives are: * Default: `false` * Allowed Values: `true`, `false` +[`# gazelle:python_include_ancestor_conftest bool`](#python-include-ancestor-conftest) +: Controls whether ancestor conftest targets are added to {bzl:obj}`py_test` target + dependencies. + * Default: `true` + * Allowed Values: `true`, `false` ## `python_extension` @@ -720,3 +725,67 @@ previously-generated or hand-created rules. :::{error} Detailed docs are not yet written. ::: + +## `python_include_ancestor_conftest` + +Version VERSION_NEXT_FEATURE includes a fix ({gh-pr}`3498`) for a long-standing issue +({gh-issue}`3497`) where ancestor `conftest.py` files were not automatically +added as dependencies of {bzl:obj}`py_test` targets. + +However, some people may not want this behavior (see https://xkcd.com/1172/). +Thus the `python_include_ancestor_conftest` directive controls this behavior. +It defaults to `true`, which causes all ancestor `conftest.py` files to be +included as dependencies for {bzl:obj}`py_test` targets. + +Setting the directive to `false` reverts to the pre-VERSION_NEXT_FEATURE behavior. + +For example, given this directory tree (not shown: intermediary `BUILD.bazel` +files) + +``` +./ +├── conftest.py +└── one/ + ├── conftest.py + └── two/ + ├── conftest.py + └── three/ + ├── BUILD.bazel + ├── conftest.py + └── my_test.py +``` + +Gazelle will generate this target for `foo_test.py` by default: + +```starlark +py_test( + name = "foo_test", + srcs = ["foo_test.py"], + deps = [ + ":conftest", # same as "//one:two/three:conftest" + "//:conftest", + "//one:conftest", + "//one/two:conftest", + ], +) +``` + +But when `python_include_ancestor_conftest` is `false`, only the sibling +`:conftest` target will be included as a dependency: + +:::{tip} +The [`include_pytest_conftest` annotation](annotation-include-pytest-conftest) +controls whether the sibling `:conftest` target is added to {bzl:obj}`py_test` +target dependency list. +::: + +```starlark +# gazelle:python_include_ancestor_conftest false +py_test( + name = "foo_test", + srcs = ["foo_test.py"], + deps = [ + ":conftest", + ], +) +``` diff --git a/gazelle/python/configure.go b/gazelle/python/configure.go index 88b15e91eb..1fe95a1683 100644 --- a/gazelle/python/configure.go +++ b/gazelle/python/configure.go @@ -74,6 +74,7 @@ func (py *Configurer) KnownDirectives() []string { pythonconfig.ExperimentalAllowRelativeImports, pythonconfig.GenerateProto, pythonconfig.PythonResolveSiblingImports, + pythonconfig.PythonIncludeAncestorConftest, } } @@ -261,6 +262,12 @@ func (py *Configurer) Configure(c *config.Config, rel string, f *rule.File) { log.Fatal(err) } config.SetResolveSiblingImports(v) + case pythonconfig.PythonIncludeAncestorConftest: + v, err := strconv.ParseBool(strings.TrimSpace(d.Value)) + if err != nil { + log.Fatal(err) + } + config.SetIncludeAncestorConftest(v) } } diff --git a/gazelle/python/generate.go b/gazelle/python/generate.go index ebca10671c..2495c42d20 100644 --- a/gazelle/python/generate.go +++ b/gazelle/python/generate.go @@ -68,7 +68,7 @@ func matchesAnyGlob(s string, globs []string) bool { // findConftestPaths returns package paths containing conftest.py, from currentPkg // up through ancestors, stopping at module root. -func findConftestPaths(repoRoot, currentPkg, pythonProjectRoot string) []string { +func findConftestPaths(repoRoot, currentPkg, pythonProjectRoot string, includeAncestorConftest bool) []string { var result []string for pkg := currentPkg; ; pkg = filepath.Dir(pkg) { if pkg == "." { @@ -77,6 +77,12 @@ func findConftestPaths(repoRoot, currentPkg, pythonProjectRoot string) []string if _, err := os.Stat(filepath.Join(repoRoot, pkg, conftestFilename)); err == nil { result = append(result, pkg) } + // We traverse up the tree to find conftest files and we start in + // the current package. Thus if we find one in the current package + // and do not want ancestors, we break early. + if !includeAncestorConftest { + break + } if pkg == "" { break } @@ -402,7 +408,6 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes setAnnotations(*annotations). generateImportsAttribute() - pyBinary := pyBinaryTarget.build() result.Gen = append(result.Gen, pyBinary) @@ -526,13 +531,13 @@ func (py *Python) GenerateRules(args language.GenerateArgs) language.GenerateRes for _, pyTestTarget := range pyTestTargets { shouldAddConftest := pyTestTarget.annotations.includePytestConftest == nil || - *pyTestTarget.annotations.includePytestConftest + *pyTestTarget.annotations.includePytestConftest if shouldAddConftest { - for _, conftestPkg := range findConftestPaths(args.Config.RepoRoot, args.Rel, pythonProjectRoot) { + for _, conftestPkg := range findConftestPaths(args.Config.RepoRoot, args.Rel, pythonProjectRoot, cfg.IncludeAncestorConftest()) { pyTestTarget.addModuleDependency( Module{ - Name: importSpecFromSrc(pythonProjectRoot, conftestPkg, conftestFilename).Imp, + Name: importSpecFromSrc(pythonProjectRoot, conftestPkg, conftestFilename).Imp, Filepath: filepath.Join(conftestPkg, conftestFilename), }, ) diff --git a/gazelle/python/testdata/directive_python_include_ancestor_conftest/BUILD.in b/gazelle/python/testdata/directive_python_include_ancestor_conftest/BUILD.in new file mode 100644 index 0000000000..e69de29bb2 diff --git a/gazelle/python/testdata/directive_python_include_ancestor_conftest/BUILD.out b/gazelle/python/testdata/directive_python_include_ancestor_conftest/BUILD.out new file mode 100644 index 0000000000..c7adad8336 --- /dev/null +++ b/gazelle/python/testdata/directive_python_include_ancestor_conftest/BUILD.out @@ -0,0 +1,8 @@ +load("@rules_python//python:defs.bzl", "py_library") + +py_library( + name = "conftest", + testonly = True, + srcs = ["conftest.py"], + visibility = ["//:__subpackages__"], +) diff --git a/gazelle/python/testdata/directive_python_include_ancestor_conftest/MODULE.bazel b/gazelle/python/testdata/directive_python_include_ancestor_conftest/MODULE.bazel new file mode 100644 index 0000000000..e69de29bb2 diff --git a/gazelle/python/testdata/directive_python_include_ancestor_conftest/README.md b/gazelle/python/testdata/directive_python_include_ancestor_conftest/README.md new file mode 100644 index 0000000000..956e65155b --- /dev/null +++ b/gazelle/python/testdata/directive_python_include_ancestor_conftest/README.md @@ -0,0 +1,23 @@ +# Directive: `python_include_ancestor_conftest` + +This test case asserts that the `# gazelle:python_include_ancestor_conftest` +directive correctly includes or excludes ancestor `conftest` targets in +`py_test` target dependencies. + +The test also asserts that the directive can be applied at any level and that +child levels will inherit the value: + ++ The root level does not set the directive (it defaults to True). ++ The next level, `one/`, inherits that value. ++ The next level, `one/two/`, sets the directive to False; consequently the + `py_test` target only includes the sibling `:conftest` target. + + The `one/two/no_conftest/` directory does not contain a `conftest.py` file + thereby asserting that we correctly do not include any `conftest` targets + whatsoever. ++ The final level, `one/two/three/`, sets the directive back to True, meaning + the `py_test` target includes a total of 4 `conftest` targets. + + The `one/two/three/no_conftest/` directory does not contain a `conftest.py` + file and thus asserts that the code includes _only_ ancestor `conftest` + targets. + +See [Issue #3595](https://github.com/bazel-contrib/rules_python/issues/3595). diff --git a/gazelle/python/testdata/directive_python_include_ancestor_conftest/WORKSPACE b/gazelle/python/testdata/directive_python_include_ancestor_conftest/WORKSPACE new file mode 100644 index 0000000000..e69de29bb2 diff --git a/gazelle/python/testdata/directive_python_include_ancestor_conftest/conftest.py b/gazelle/python/testdata/directive_python_include_ancestor_conftest/conftest.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/gazelle/python/testdata/directive_python_include_ancestor_conftest/one/BUILD.in b/gazelle/python/testdata/directive_python_include_ancestor_conftest/one/BUILD.in new file mode 100644 index 0000000000..e69de29bb2 diff --git a/gazelle/python/testdata/directive_python_include_ancestor_conftest/one/BUILD.out b/gazelle/python/testdata/directive_python_include_ancestor_conftest/one/BUILD.out new file mode 100644 index 0000000000..9d0405e92f --- /dev/null +++ b/gazelle/python/testdata/directive_python_include_ancestor_conftest/one/BUILD.out @@ -0,0 +1,17 @@ +load("@rules_python//python:defs.bzl", "py_library", "py_test") + +py_library( + name = "conftest", + testonly = True, + srcs = ["conftest.py"], + visibility = ["//:__subpackages__"], +) + +py_test( + name = "my_test", + srcs = ["my_test.py"], + deps = [ + ":conftest", + "//:conftest", + ], +) diff --git a/gazelle/python/testdata/directive_python_include_ancestor_conftest/one/conftest.py b/gazelle/python/testdata/directive_python_include_ancestor_conftest/one/conftest.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/gazelle/python/testdata/directive_python_include_ancestor_conftest/one/my_test.py b/gazelle/python/testdata/directive_python_include_ancestor_conftest/one/my_test.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/gazelle/python/testdata/directive_python_include_ancestor_conftest/one/two/BUILD.in b/gazelle/python/testdata/directive_python_include_ancestor_conftest/one/two/BUILD.in new file mode 100644 index 0000000000..3805e248e2 --- /dev/null +++ b/gazelle/python/testdata/directive_python_include_ancestor_conftest/one/two/BUILD.in @@ -0,0 +1 @@ +# gazelle:python_include_ancestor_conftest false diff --git a/gazelle/python/testdata/directive_python_include_ancestor_conftest/one/two/BUILD.out b/gazelle/python/testdata/directive_python_include_ancestor_conftest/one/two/BUILD.out new file mode 100644 index 0000000000..edb91a38f5 --- /dev/null +++ b/gazelle/python/testdata/directive_python_include_ancestor_conftest/one/two/BUILD.out @@ -0,0 +1,16 @@ +load("@rules_python//python:defs.bzl", "py_library", "py_test") + +# gazelle:python_include_ancestor_conftest false + +py_library( + name = "conftest", + testonly = True, + srcs = ["conftest.py"], + visibility = ["//:__subpackages__"], +) + +py_test( + name = "my_test", + srcs = ["my_test.py"], + deps = [":conftest"], +) diff --git a/gazelle/python/testdata/directive_python_include_ancestor_conftest/one/two/conftest.py b/gazelle/python/testdata/directive_python_include_ancestor_conftest/one/two/conftest.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/gazelle/python/testdata/directive_python_include_ancestor_conftest/one/two/my_test.py b/gazelle/python/testdata/directive_python_include_ancestor_conftest/one/two/my_test.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/gazelle/python/testdata/directive_python_include_ancestor_conftest/one/two/no_conftest/BUILD.in b/gazelle/python/testdata/directive_python_include_ancestor_conftest/one/two/no_conftest/BUILD.in new file mode 100644 index 0000000000..e69de29bb2 diff --git a/gazelle/python/testdata/directive_python_include_ancestor_conftest/one/two/no_conftest/BUILD.out b/gazelle/python/testdata/directive_python_include_ancestor_conftest/one/two/no_conftest/BUILD.out new file mode 100644 index 0000000000..764e2b4172 --- /dev/null +++ b/gazelle/python/testdata/directive_python_include_ancestor_conftest/one/two/no_conftest/BUILD.out @@ -0,0 +1,6 @@ +load("@rules_python//python:defs.bzl", "py_test") + +py_test( + name = "my_test", + srcs = ["my_test.py"], +) diff --git a/gazelle/python/testdata/directive_python_include_ancestor_conftest/one/two/no_conftest/my_test.py b/gazelle/python/testdata/directive_python_include_ancestor_conftest/one/two/no_conftest/my_test.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/gazelle/python/testdata/directive_python_include_ancestor_conftest/one/two/three/BUILD.in b/gazelle/python/testdata/directive_python_include_ancestor_conftest/one/two/three/BUILD.in new file mode 100644 index 0000000000..987cd7dc20 --- /dev/null +++ b/gazelle/python/testdata/directive_python_include_ancestor_conftest/one/two/three/BUILD.in @@ -0,0 +1 @@ +# gazelle:python_include_ancestor_conftest true diff --git a/gazelle/python/testdata/directive_python_include_ancestor_conftest/one/two/three/BUILD.out b/gazelle/python/testdata/directive_python_include_ancestor_conftest/one/two/three/BUILD.out new file mode 100644 index 0000000000..605aa00f88 --- /dev/null +++ b/gazelle/python/testdata/directive_python_include_ancestor_conftest/one/two/three/BUILD.out @@ -0,0 +1,21 @@ +load("@rules_python//python:defs.bzl", "py_library", "py_test") + +# gazelle:python_include_ancestor_conftest true + +py_library( + name = "conftest", + testonly = True, + srcs = ["conftest.py"], + visibility = ["//:__subpackages__"], +) + +py_test( + name = "my_test", + srcs = ["my_test.py"], + deps = [ + ":conftest", + "//:conftest", + "//one:conftest", + "//one/two:conftest", + ], +) diff --git a/gazelle/python/testdata/directive_python_include_ancestor_conftest/one/two/three/conftest.py b/gazelle/python/testdata/directive_python_include_ancestor_conftest/one/two/three/conftest.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/gazelle/python/testdata/directive_python_include_ancestor_conftest/one/two/three/my_test.py b/gazelle/python/testdata/directive_python_include_ancestor_conftest/one/two/three/my_test.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/gazelle/python/testdata/directive_python_include_ancestor_conftest/one/two/three/no_conftest/BUILD.in b/gazelle/python/testdata/directive_python_include_ancestor_conftest/one/two/three/no_conftest/BUILD.in new file mode 100644 index 0000000000..e69de29bb2 diff --git a/gazelle/python/testdata/directive_python_include_ancestor_conftest/one/two/three/no_conftest/BUILD.out b/gazelle/python/testdata/directive_python_include_ancestor_conftest/one/two/three/no_conftest/BUILD.out new file mode 100644 index 0000000000..c1bddccc30 --- /dev/null +++ b/gazelle/python/testdata/directive_python_include_ancestor_conftest/one/two/three/no_conftest/BUILD.out @@ -0,0 +1,12 @@ +load("@rules_python//python:defs.bzl", "py_test") + +py_test( + name = "my_test", + srcs = ["my_test.py"], + deps = [ + "//:conftest", + "//one:conftest", + "//one/two:conftest", + "//one/two/three:conftest", + ], +) diff --git a/gazelle/python/testdata/directive_python_include_ancestor_conftest/one/two/three/no_conftest/my_test.py b/gazelle/python/testdata/directive_python_include_ancestor_conftest/one/two/three/no_conftest/my_test.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/gazelle/python/testdata/directive_python_include_ancestor_conftest/test.yaml b/gazelle/python/testdata/directive_python_include_ancestor_conftest/test.yaml new file mode 100644 index 0000000000..36dd656b39 --- /dev/null +++ b/gazelle/python/testdata/directive_python_include_ancestor_conftest/test.yaml @@ -0,0 +1,3 @@ +--- +expect: + exit_code: 0 diff --git a/gazelle/pythonconfig/pythonconfig.go b/gazelle/pythonconfig/pythonconfig.go index a1271af3be..17db9aae0d 100644 --- a/gazelle/pythonconfig/pythonconfig.go +++ b/gazelle/pythonconfig/pythonconfig.go @@ -116,6 +116,15 @@ const ( // like "import a" can be resolved to sibling modules. When disabled, they // can only be resolved as an absolute import. PythonResolveSiblingImports = "python_resolve_sibling_imports" + // PythonIncludeAncestorConftest represents the directive that controls + // whether ancestor conftest.py files are added as dependencies to py_test + // targets. When enabled (the default), ancestor conftest.py files are + // included as deps. + // See also https://github.com/bazel-contrib/rules_python/pull/3498, which + // fixed previous behavior that was incorrectly _not_ adding the files and + // https://github.com/bazel-contrib/rules_python/issues/3595 which requested + // that the behavior be configurable. + PythonIncludeAncestorConftest = "python_include_ancestor_conftest" ) // GenerationModeType represents one of the generation modes for the Python @@ -209,6 +218,7 @@ type Config struct { generatePyiSrcs bool generateProto bool resolveSiblingImports bool + includeAncestorConftest bool } type LabelNormalizationType int @@ -250,6 +260,7 @@ func New( generatePyiSrcs: false, generateProto: false, resolveSiblingImports: false, + includeAncestorConftest: true, } } @@ -288,6 +299,7 @@ func (c *Config) NewChild() *Config { generatePyiSrcs: c.generatePyiSrcs, generateProto: c.generateProto, resolveSiblingImports: c.resolveSiblingImports, + includeAncestorConftest: c.includeAncestorConftest, } } @@ -629,6 +641,16 @@ func (c *Config) ResolveSiblingImports() bool { return c.resolveSiblingImports } +// SetIncludeAncestorConftest sets whether ancestor conftest files are added to py_test targets. +func (c *Config) SetIncludeAncestorConftest(includeAncestorConftest bool) { + c.includeAncestorConftest = includeAncestorConftest +} + +// IncludeAncestorConftest returns whether ancestor conftest files are added to py_test targets. +func (c *Config) IncludeAncestorConftest() bool { + return c.includeAncestorConftest +} + // FormatThirdPartyDependency returns a label to a third-party dependency performing all formating and normalization. func (c *Config) FormatThirdPartyDependency(repositoryName string, distributionName string) label.Label { conventionalDistributionName := strings.ReplaceAll(c.labelConvention, distributionNameLabelConventionSubstitution, distributionName)