From 8aee87b6fd8cbedd87d1dc0286c9a71f0013cd2d Mon Sep 17 00:00:00 2001 From: Vivek Dasari Date: Tue, 15 Jul 2025 22:32:01 -0700 Subject: [PATCH 1/3] deps-to-remove clone of deps --- gazelle/python/kinds.go | 2 ++ gazelle/python/resolve.go | 4 ++++ gazelle/python/target.go | 17 +++++++++++++++++ 3 files changed, 23 insertions(+) diff --git a/gazelle/python/kinds.go b/gazelle/python/kinds.go index 83da53edbd..b370118dcd 100644 --- a/gazelle/python/kinds.go +++ b/gazelle/python/kinds.go @@ -60,10 +60,12 @@ var pyKinds = map[string]rule.KindInfo{ SubstituteAttrs: map[string]bool{}, MergeableAttrs: map[string]bool{ "srcs": true, + "deps_to_remove": true, }, ResolveAttrs: map[string]bool{ "deps": true, "pyi_deps": true, + "deps_to_remove": true, }, }, pyTestKind: { diff --git a/gazelle/python/resolve.go b/gazelle/python/resolve.go index 0dd80841d4..9d6deb4a3d 100644 --- a/gazelle/python/resolve.go +++ b/gazelle/python/resolve.go @@ -39,6 +39,8 @@ const ( // resolvedDepsKey is the attribute key used to pass dependencies that don't // need to be resolved by the dependency resolver in the Resolver step. resolvedDepsKey = "_gazelle_python_resolved_deps" + // depsToRemoveKey is the attribute key used to store the deps_to_remove list + depsToRemoveKey = "_gazelle_python_deps_to_remove" ) // Resolver satisfies the resolve.Resolver interface. It resolves dependencies @@ -356,6 +358,7 @@ func (py *Resolver) Resolve( if cfg.GeneratePyiDeps() { if !deps.Empty() { r.SetAttr("deps", convertDependencySetToExpr(deps)) + r.SetAttr("deps_to_remove", convertDependencySetToExpr(deps)) } if !pyiDeps.Empty() { r.SetAttr("pyi_deps", convertDependencySetToExpr(pyiDeps)) @@ -368,6 +371,7 @@ func (py *Resolver) Resolve( if !combinedDeps.Empty() { r.SetAttr("deps", convertDependencySetToExpr(combinedDeps)) + r.SetAttr("deps_to_remove", convertDependencySetToExpr(combinedDeps)) } } } diff --git a/gazelle/python/target.go b/gazelle/python/target.go index 3b7f464335..8729fdcf95 100644 --- a/gazelle/python/target.go +++ b/gazelle/python/target.go @@ -33,6 +33,7 @@ type targetBuilder struct { siblingSrcs *treeset.Set deps *treeset.Set resolvedDeps *treeset.Set + depsToRemove *treeset.Set visibility *treeset.Set main *string imports []string @@ -50,6 +51,7 @@ func newTargetBuilder(kind, name, pythonProjectRoot, bzlPackage string, siblingS siblingSrcs: siblingSrcs, deps: treeset.NewWith(moduleComparator), resolvedDeps: treeset.NewWith(godsutils.StringComparator), + depsToRemove: treeset.NewWith(godsutils.StringComparator), visibility: treeset.NewWith(godsutils.StringComparator), } } @@ -100,6 +102,20 @@ func (t *targetBuilder) addResolvedDependencies(deps []string) *targetBuilder { return t } +// addDepToRemove adds a single dependency to be removed to the target. +func (t *targetBuilder) addDepToRemove(dep string) *targetBuilder { + t.depsToRemove.Add(dep) + return t +} + +// addDepsToRemove adds multiple dependencies to be removed to the target. +func (t *targetBuilder) addDepsToRemove(deps []string) *targetBuilder { + for _, dep := range deps { + t.addDepToRemove(dep) + } + return t +} + // addVisibility adds visibility labels to the target. func (t *targetBuilder) addVisibility(visibility []string) *targetBuilder { for _, item := range visibility { @@ -161,5 +177,6 @@ func (t *targetBuilder) build() *rule.Rule { r.SetAttr("testonly", true) } r.SetPrivateAttr(resolvedDepsKey, t.resolvedDeps) + r.SetPrivateAttr(depsToRemoveKey, t.depsToRemove) return r } From 4b0d59426b9e49eac39c03ad44272fbaab598dbd Mon Sep 17 00:00:00 2001 From: Vivek Dasari Date: Wed, 16 Jul 2025 07:38:51 -0700 Subject: [PATCH 2/3] correctly only add deps_to_remove --- gazelle/python/language.go | 6 +- gazelle/python/resolve.go | 191 +++++++++++++++++- gazelle/python/target.go | 4 + .../BUILD.in | 1 + .../BUILD.out | 35 ++++ .../MODULE.bazel | 6 + .../README.md | 28 +++ .../WORKSPACE | 1 + .../__init__.py | 15 ++ .../application.py | 30 +++ .../deps-order.txt | 7 + .../foundation.py | 27 +++ .../middleware.py | 25 +++ .../test.yaml | 15 ++ .../deps_to_remove_with_order/BUILD.in | 1 + .../deps_to_remove_with_order/BUILD.out | 32 +++ .../deps_to_remove_with_order/README.md | 26 +++ .../deps_to_remove_with_order/WORKSPACE | 1 + .../deps_to_remove_with_order/__init__.py | 15 ++ .../deps_to_remove_with_order/core.py | 23 +++ .../deps_to_remove_with_order/deps-order.txt | 3 + .../deps_to_remove_with_order/high_level.py | 30 +++ .../deps_to_remove_with_order/test.yaml | 15 ++ .../deps_to_remove_with_order/utils.py | 27 +++ 24 files changed, 560 insertions(+), 4 deletions(-) create mode 100644 gazelle/python/testdata/deps_to_remove_ordering_violation/BUILD.in create mode 100644 gazelle/python/testdata/deps_to_remove_ordering_violation/BUILD.out create mode 100644 gazelle/python/testdata/deps_to_remove_ordering_violation/MODULE.bazel create mode 100644 gazelle/python/testdata/deps_to_remove_ordering_violation/README.md create mode 100644 gazelle/python/testdata/deps_to_remove_ordering_violation/WORKSPACE create mode 100644 gazelle/python/testdata/deps_to_remove_ordering_violation/__init__.py create mode 100644 gazelle/python/testdata/deps_to_remove_ordering_violation/application.py create mode 100644 gazelle/python/testdata/deps_to_remove_ordering_violation/deps-order.txt create mode 100644 gazelle/python/testdata/deps_to_remove_ordering_violation/foundation.py create mode 100644 gazelle/python/testdata/deps_to_remove_ordering_violation/middleware.py create mode 100644 gazelle/python/testdata/deps_to_remove_ordering_violation/test.yaml create mode 100644 gazelle/python/testdata/deps_to_remove_with_order/BUILD.in create mode 100644 gazelle/python/testdata/deps_to_remove_with_order/BUILD.out create mode 100644 gazelle/python/testdata/deps_to_remove_with_order/README.md create mode 100644 gazelle/python/testdata/deps_to_remove_with_order/WORKSPACE create mode 100644 gazelle/python/testdata/deps_to_remove_with_order/__init__.py create mode 100644 gazelle/python/testdata/deps_to_remove_with_order/core.py create mode 100644 gazelle/python/testdata/deps_to_remove_with_order/deps-order.txt create mode 100644 gazelle/python/testdata/deps_to_remove_with_order/high_level.py create mode 100644 gazelle/python/testdata/deps_to_remove_with_order/test.yaml create mode 100644 gazelle/python/testdata/deps_to_remove_with_order/utils.py diff --git a/gazelle/python/language.go b/gazelle/python/language.go index 56eb97b043..cab4fb2392 100644 --- a/gazelle/python/language.go +++ b/gazelle/python/language.go @@ -28,5 +28,9 @@ type Python struct { // NewLanguage initializes a new Python that satisfies the language.Language // interface. This is the entrypoint for the extension initialization. func NewLanguage() language.Language { - return &Python{} + return &Python{ + Resolver: Resolver{ + depsOrderResolver: NewDepsOrderResolver(), + }, + } } diff --git a/gazelle/python/resolve.go b/gazelle/python/resolve.go index 9d6deb4a3d..125e573827 100644 --- a/gazelle/python/resolve.go +++ b/gazelle/python/resolve.go @@ -15,6 +15,7 @@ package python import ( + "bufio" "fmt" "log" "os" @@ -41,11 +42,133 @@ const ( resolvedDepsKey = "_gazelle_python_resolved_deps" // depsToRemoveKey is the attribute key used to store the deps_to_remove list depsToRemoveKey = "_gazelle_python_deps_to_remove" + // srcsForOrderingKey is the attribute key used to store source files for ordering constraints + srcsForOrderingKey = "_gazelle_python_srcs_for_ordering" + // depsOrderFilename is the name of the file that contains the dependency order + depsOrderFilename = "deps-order.txt" ) +// DepsOrderResolver holds the dependency order information parsed from deps-order.txt +type DepsOrderResolver struct { + fileToIndex map[string]int + loaded bool + // importToSrcs maps import names to their source files (pkg-relative paths) + importToSrcs map[string][]string +} + +// NewDepsOrderResolver creates a new DepsOrderResolver +func NewDepsOrderResolver() *DepsOrderResolver { + return &DepsOrderResolver{ + fileToIndex: make(map[string]int), + loaded: false, + importToSrcs: make(map[string][]string), + } +} + +// LoadDepsOrder loads the deps-order.txt file from the repository root +func (d *DepsOrderResolver) LoadDepsOrder(repoRoot string) error { + if d.loaded { + return nil + } + + depsOrderPath := filepath.Join(repoRoot, depsOrderFilename) + file, err := os.Open(depsOrderPath) + if err != nil { + if os.IsNotExist(err) { + // File doesn't exist, which is fine - we just won't use deps ordering + d.loaded = true + return nil + } + return fmt.Errorf("failed to open %s: %v", depsOrderFilename, err) + } + defer file.Close() + + scanner := bufio.NewScanner(file) + index := 0 + for scanner.Scan() { + line := strings.TrimSpace(scanner.Text()) + if line == "" || strings.HasPrefix(line, "#") { + continue // Skip empty lines and comments + } + d.fileToIndex[line] = index + index++ + } + + if err := scanner.Err(); err != nil { + return fmt.Errorf("failed to read %s: %v", depsOrderFilename, err) + } + + d.loaded = true + return nil +} + +// GetAverageIndex calculates the average index for a set of source files +func (d *DepsOrderResolver) GetAverageIndex(srcs []string) float64 { + if len(d.fileToIndex) == 0 { + return 0 // No ordering file, return 0 + } + + totalIndex := 0 + validSrcs := 0 + for _, src := range srcs { + // Try both the full path and just the filename + filename := filepath.Base(src) + if index, exists := d.fileToIndex[src]; exists { + totalIndex += index + validSrcs++ + } else if index, exists := d.fileToIndex[filename]; exists { + totalIndex += index + validSrcs++ + } + } + + if validSrcs == 0 { + return float64(len(d.fileToIndex)) // Files not in order get max index + } + + return float64(totalIndex) / float64(validSrcs) +} + +// ShouldAddToDepsToRemove returns true if the dependency should be added to deps_to_remove based on ordering constraints +func (d *DepsOrderResolver) ShouldAddToDepsToRemove(currentTargetSrcs []string, depTargetSrcs []string) bool { + if len(d.fileToIndex) == 0 { + return false // No ordering constraints + } + + currentAvg := d.GetAverageIndex(currentTargetSrcs) + depAvg := d.GetAverageIndex(depTargetSrcs) + + // If current target has lower average index than dependency, the dependency should be removed + return currentAvg < depAvg +} + +// RegisterImportSources registers the mapping between import specs and their source files +func (d *DepsOrderResolver) RegisterImportSources(importSpecs []resolve.ImportSpec, pkgPath string, srcs []string) { + // Convert sources to repo-relative paths + repoRelativeSrcs := make([]string, 0, len(srcs)) + for _, src := range srcs { + repoRelativeSrcs = append(repoRelativeSrcs, filepath.Join(pkgPath, src)) + } + + // Register each import spec + for _, spec := range importSpecs { + d.importToSrcs[spec.Imp] = repoRelativeSrcs + } +} + +// getSourcesForImport gets the source files for a given import name using the registered mappings +func (d *DepsOrderResolver) getSourcesForImport(importName string) []string { + if srcs, ok := d.importToSrcs[importName]; ok { + return srcs + } + return []string{} +} + // Resolver satisfies the resolve.Resolver interface. It resolves dependencies // in rules generated by this extension. -type Resolver struct{} +type Resolver struct{ + depsOrderResolver *DepsOrderResolver +} // Name returns the name of the language. This is the prefix of the kinds of // rules generated. E.g. py_library and py_binary. @@ -78,6 +201,10 @@ func (py *Resolver) Imports(c *config.Config, r *rule.Rule, f *rule.File) []reso if len(provides) == 0 { return nil } + + // Register the import-to-source mappings for dependency ordering + py.depsOrderResolver.RegisterImportSources(provides, f.Pkg, srcs) + return provides } @@ -327,6 +454,22 @@ func (py *Resolver) Resolve( } matchLabel := filteredMatches[0].Label.Rel(from.Repo, from.Pkg) dep := matchLabel.String() + + // Register the mapping from dependency label to its source files + // This allows us to look up source files during deps_to_remove creation + match := filteredMatches[0] + depSrcsPaths := make([]string, 0) + // Try to infer source file from the import name + if strings.Contains(moduleName, ".") { + parts := strings.Split(moduleName, ".") + srcFile := parts[len(parts)-1] + ".py" + depSrcsPaths = append(depSrcsPaths, filepath.Join(match.Label.Pkg, srcFile)) + } else { + srcFile := moduleName + ".py" + depSrcsPaths = append(depSrcsPaths, filepath.Join(match.Label.Pkg, srcFile)) + } + py.depsOrderResolver.importToSrcs[dep] = depSrcsPaths + addDependency(dep, mod.TypeCheckingOnly, deps, pyiDeps) if explainDependency == dep { log.Printf("Explaining dependency (%s): "+ @@ -355,10 +498,49 @@ func (py *Resolver) Resolve( addResolvedDeps(r, deps) + // Load deps order constraints if available + err := py.depsOrderResolver.LoadDepsOrder(c.RepoRoot) + if err != nil { + log.Printf("Warning: failed to load deps-order.txt: %v", err) + } + + // Get current rule's sources for ordering comparison + currentSrcs := r.AttrStrings("srcs") + // Convert relative paths to paths relative to repo root + currentSrcsPaths := make([]string, 0, len(currentSrcs)) + for _, src := range currentSrcs { + currentSrcsPaths = append(currentSrcsPaths, filepath.Join(from.Pkg, src)) + } + + // Function to create deps_to_remove based on ordering constraints + createDepsToRemove := func(allDeps *treeset.Set) *treeset.Set { + depsToRemove := treeset.NewWith(godsutils.StringComparator) + + // If we have ordering constraints, check each dependency + if len(py.depsOrderResolver.fileToIndex) > 0 { + allDeps.Each(func(_ int, dep interface{}) { + depLabel := dep.(string) + + // Get the source files for this dependency using the registered mappings + depSrcs := py.depsOrderResolver.getSourcesForImport(depLabel) + + // Check if this dependency should be added to deps_to_remove based on ordering + if py.depsOrderResolver.ShouldAddToDepsToRemove(currentSrcsPaths, depSrcs) { + depsToRemove.Add(dep) + } + }) + } + + return depsToRemove + } + if cfg.GeneratePyiDeps() { if !deps.Empty() { r.SetAttr("deps", convertDependencySetToExpr(deps)) - r.SetAttr("deps_to_remove", convertDependencySetToExpr(deps)) + depsToRemove := createDepsToRemove(deps) + if !depsToRemove.Empty() { + r.SetAttr("deps_to_remove", convertDependencySetToExpr(depsToRemove)) + } } if !pyiDeps.Empty() { r.SetAttr("pyi_deps", convertDependencySetToExpr(pyiDeps)) @@ -371,7 +553,10 @@ func (py *Resolver) Resolve( if !combinedDeps.Empty() { r.SetAttr("deps", convertDependencySetToExpr(combinedDeps)) - r.SetAttr("deps_to_remove", convertDependencySetToExpr(combinedDeps)) + depsToRemove := createDepsToRemove(combinedDeps) + if !depsToRemove.Empty() { + r.SetAttr("deps_to_remove", convertDependencySetToExpr(depsToRemove)) + } } } } diff --git a/gazelle/python/target.go b/gazelle/python/target.go index 8729fdcf95..260debd1f2 100644 --- a/gazelle/python/target.go +++ b/gazelle/python/target.go @@ -178,5 +178,9 @@ func (t *targetBuilder) build() *rule.Rule { } r.SetPrivateAttr(resolvedDepsKey, t.resolvedDeps) r.SetPrivateAttr(depsToRemoveKey, t.depsToRemove) + // Store source files for ordering constraints + if !t.srcs.Empty() { + r.SetPrivateAttr(srcsForOrderingKey, t.srcs) + } return r } diff --git a/gazelle/python/testdata/deps_to_remove_ordering_violation/BUILD.in b/gazelle/python/testdata/deps_to_remove_ordering_violation/BUILD.in new file mode 100644 index 0000000000..af2c2cea4b --- /dev/null +++ b/gazelle/python/testdata/deps_to_remove_ordering_violation/BUILD.in @@ -0,0 +1 @@ +# gazelle:python_generation_mode file diff --git a/gazelle/python/testdata/deps_to_remove_ordering_violation/BUILD.out b/gazelle/python/testdata/deps_to_remove_ordering_violation/BUILD.out new file mode 100644 index 0000000000..44c5fe0b56 --- /dev/null +++ b/gazelle/python/testdata/deps_to_remove_ordering_violation/BUILD.out @@ -0,0 +1,35 @@ +load("@rules_python//python:defs.bzl", "py_library") + +# gazelle:python_generation_mode file + +py_library( + name = "__init__", + srcs = ["__init__.py"], + visibility = ["//:__subpackages__"], +) + +py_library( + name = "application", + srcs = ["application.py"], + visibility = ["//:__subpackages__"], + deps = [ + ":foundation", + ":middleware", + ], +) + +py_library( + name = "foundation", + srcs = ["foundation.py"], + deps_to_remove = [":middleware"], + visibility = ["//:__subpackages__"], + deps = [":middleware"], +) + +py_library( + name = "middleware", + srcs = ["middleware.py"], + deps_to_remove = [":application"], + visibility = ["//:__subpackages__"], + deps = [":application"], +) diff --git a/gazelle/python/testdata/deps_to_remove_ordering_violation/MODULE.bazel b/gazelle/python/testdata/deps_to_remove_ordering_violation/MODULE.bazel new file mode 100644 index 0000000000..00bb18361f --- /dev/null +++ b/gazelle/python/testdata/deps_to_remove_ordering_violation/MODULE.bazel @@ -0,0 +1,6 @@ +############################################################################### +# Bazel now uses Bzlmod by default to manage external dependencies. +# Please consider migrating your external dependencies from WORKSPACE to MODULE.bazel. +# +# For more details, please check https://github.com/bazelbuild/bazel/issues/18958 +############################################################################### diff --git a/gazelle/python/testdata/deps_to_remove_ordering_violation/README.md b/gazelle/python/testdata/deps_to_remove_ordering_violation/README.md new file mode 100644 index 0000000000..c2b752e48e --- /dev/null +++ b/gazelle/python/testdata/deps_to_remove_ordering_violation/README.md @@ -0,0 +1,28 @@ +# Test case for deps_to_remove with ordering violations + +This test case verifies that the `deps_to_remove` attribute is correctly populated +when there are dependency ordering violations defined in `deps-order.txt`. + +## Test scenario: + +1. **deps-order.txt** defines the following order: + - `foundation.py` (index 0) - foundational code + - `middleware.py` (index 1) - middle layer + - `application.py` (index 2) - application layer + +2. **Dependencies**: + - `application.py` imports `middleware` and `foundation` (valid - higher depending on lower) + - `middleware.py` imports `application` (INVALID - lower depending on higher) + - `foundation.py` imports `middleware` (INVALID - lower depending on higher) + +3. **Expected behavior**: + - All dependencies should appear in `deps` + - Violating dependencies should also appear in `deps_to_remove`: + - `middleware`'s dependency on `application` should be in `deps_to_remove` + - `foundation`'s dependency on `middleware` should be in `deps_to_remove` + +## Files: +- `foundation.py` - foundational functionality (illegally depends on middleware) +- `middleware.py` - middle layer (illegally depends on application) +- `application.py` - application layer +- `deps-order.txt` - defines allowed dependency order diff --git a/gazelle/python/testdata/deps_to_remove_ordering_violation/WORKSPACE b/gazelle/python/testdata/deps_to_remove_ordering_violation/WORKSPACE new file mode 100644 index 0000000000..e2025846c0 --- /dev/null +++ b/gazelle/python/testdata/deps_to_remove_ordering_violation/WORKSPACE @@ -0,0 +1 @@ +workspace(name = "deps_to_remove_ordering_violation_test") \ No newline at end of file diff --git a/gazelle/python/testdata/deps_to_remove_ordering_violation/__init__.py b/gazelle/python/testdata/deps_to_remove_ordering_violation/__init__.py new file mode 100644 index 0000000000..3e68ab20ae --- /dev/null +++ b/gazelle/python/testdata/deps_to_remove_ordering_violation/__init__.py @@ -0,0 +1,15 @@ +# Copyright 2023 The Bazel Authors. All rights reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# Empty init file for test package \ No newline at end of file diff --git a/gazelle/python/testdata/deps_to_remove_ordering_violation/application.py b/gazelle/python/testdata/deps_to_remove_ordering_violation/application.py new file mode 100644 index 0000000000..b47512f007 --- /dev/null +++ b/gazelle/python/testdata/deps_to_remove_ordering_violation/application.py @@ -0,0 +1,30 @@ +# Copyright 2023 The Bazel Authors. All rights reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""Application layer - depends on middleware and foundation (valid).""" + +import foundation +import middleware + +def app_function(): + """Application function using both foundation and middleware.""" + return f"app: {foundation.foundation_function()} + {middleware.middleware_function()}" + +def get_app_data(): + """Get application data.""" + return { + "app": True, + "foundation": foundation.get_foundation_data(), + "middleware": middleware.process_data() + } \ No newline at end of file diff --git a/gazelle/python/testdata/deps_to_remove_ordering_violation/deps-order.txt b/gazelle/python/testdata/deps_to_remove_ordering_violation/deps-order.txt new file mode 100644 index 0000000000..4472cd3266 --- /dev/null +++ b/gazelle/python/testdata/deps_to_remove_ordering_violation/deps-order.txt @@ -0,0 +1,7 @@ +# Dependency ordering constraints +# Files listed earlier can be depended upon by files listed later +# but not vice versa + +foundation.py +middleware.py +application.py \ No newline at end of file diff --git a/gazelle/python/testdata/deps_to_remove_ordering_violation/foundation.py b/gazelle/python/testdata/deps_to_remove_ordering_violation/foundation.py new file mode 100644 index 0000000000..a66ba7e70b --- /dev/null +++ b/gazelle/python/testdata/deps_to_remove_ordering_violation/foundation.py @@ -0,0 +1,27 @@ +# Copyright 2023 The Bazel Authors. All rights reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""Foundation layer - ILLEGALLY depends on middleware (ordering violation).""" + +import middleware # This violates ordering constraints! + + +def foundation_function(): + """Foundation function that uses middleware.""" + return f"foundation: {middleware.middleware_function()}" + + +def get_foundation_data(): + """Get foundation data.""" + return {"foundation": True, "data": middleware.process_data()} diff --git a/gazelle/python/testdata/deps_to_remove_ordering_violation/middleware.py b/gazelle/python/testdata/deps_to_remove_ordering_violation/middleware.py new file mode 100644 index 0000000000..13c5e2c3e9 --- /dev/null +++ b/gazelle/python/testdata/deps_to_remove_ordering_violation/middleware.py @@ -0,0 +1,25 @@ +# Copyright 2023 The Bazel Authors. All rights reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""Middleware layer - ILLEGALLY depends on application (ordering violation).""" + +import application # This violates ordering constraints! + +def middleware_function(): + """Middleware function that illegally uses application layer.""" + return f"middleware: {application.app_function()}" + +def process_data(): + """Process data using application layer (violation).""" + return application.get_app_data() \ No newline at end of file diff --git a/gazelle/python/testdata/deps_to_remove_ordering_violation/test.yaml b/gazelle/python/testdata/deps_to_remove_ordering_violation/test.yaml new file mode 100644 index 0000000000..fcea77710f --- /dev/null +++ b/gazelle/python/testdata/deps_to_remove_ordering_violation/test.yaml @@ -0,0 +1,15 @@ +# Copyright 2023 The Bazel Authors. All rights reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +--- diff --git a/gazelle/python/testdata/deps_to_remove_with_order/BUILD.in b/gazelle/python/testdata/deps_to_remove_with_order/BUILD.in new file mode 100644 index 0000000000..af2c2cea4b --- /dev/null +++ b/gazelle/python/testdata/deps_to_remove_with_order/BUILD.in @@ -0,0 +1 @@ +# gazelle:python_generation_mode file diff --git a/gazelle/python/testdata/deps_to_remove_with_order/BUILD.out b/gazelle/python/testdata/deps_to_remove_with_order/BUILD.out new file mode 100644 index 0000000000..d90c9b35a4 --- /dev/null +++ b/gazelle/python/testdata/deps_to_remove_with_order/BUILD.out @@ -0,0 +1,32 @@ +load("@rules_python//python:defs.bzl", "py_library") + +# gazelle:python_generation_mode file + +py_library( + name = "__init__", + srcs = ["__init__.py"], + visibility = ["//:__subpackages__"], +) + +py_library( + name = "core", + srcs = ["core.py"], + visibility = ["//:__subpackages__"], +) + +py_library( + name = "high_level", + srcs = ["high_level.py"], + visibility = ["//:__subpackages__"], + deps = [ + ":core", + ":utils", + ], +) + +py_library( + name = "utils", + srcs = ["utils.py"], + visibility = ["//:__subpackages__"], + deps = [":core"], +) diff --git a/gazelle/python/testdata/deps_to_remove_with_order/README.md b/gazelle/python/testdata/deps_to_remove_with_order/README.md new file mode 100644 index 0000000000..2290f70d71 --- /dev/null +++ b/gazelle/python/testdata/deps_to_remove_with_order/README.md @@ -0,0 +1,26 @@ +# Test case for deps_to_remove with deps-order.txt + +This test case verifies that the `deps_to_remove` attribute is correctly populated +based on the dependency ordering constraints defined in `deps-order.txt`. + +## Test scenario: + +1. **deps-order.txt** defines the following order: + - `core.py` (index 0) - foundational code + - `utils.py` (index 1) - utility functions + - `high_level.py` (index 2) - high-level functionality + +2. **Dependencies**: + - `high_level.py` imports `core` and `utils` (valid - higher index depending on lower) + - `utils.py` imports `core` (valid - higher index depending on lower) + - `core.py` imports nothing (valid - no dependencies) + +3. **Expected behavior**: + - All dependencies should appear in `deps` + - No dependencies should appear in `deps_to_remove` (all are valid) + +## Files: +- `core.py` - basic functionality +- `utils.py` - depends on core +- `high_level.py` - depends on both core and utils +- `deps-order.txt` - defines allowed dependency order \ No newline at end of file diff --git a/gazelle/python/testdata/deps_to_remove_with_order/WORKSPACE b/gazelle/python/testdata/deps_to_remove_with_order/WORKSPACE new file mode 100644 index 0000000000..e9c57a5fb8 --- /dev/null +++ b/gazelle/python/testdata/deps_to_remove_with_order/WORKSPACE @@ -0,0 +1 @@ +workspace(name = "deps_to_remove_with_order_test") \ No newline at end of file diff --git a/gazelle/python/testdata/deps_to_remove_with_order/__init__.py b/gazelle/python/testdata/deps_to_remove_with_order/__init__.py new file mode 100644 index 0000000000..3e68ab20ae --- /dev/null +++ b/gazelle/python/testdata/deps_to_remove_with_order/__init__.py @@ -0,0 +1,15 @@ +# Copyright 2023 The Bazel Authors. All rights reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# Empty init file for test package \ No newline at end of file diff --git a/gazelle/python/testdata/deps_to_remove_with_order/core.py b/gazelle/python/testdata/deps_to_remove_with_order/core.py new file mode 100644 index 0000000000..3058784620 --- /dev/null +++ b/gazelle/python/testdata/deps_to_remove_with_order/core.py @@ -0,0 +1,23 @@ +# Copyright 2023 The Bazel Authors. All rights reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""Core foundational functionality - no dependencies.""" + +def core_function(): + """Basic core function.""" + return "core functionality" + +def get_core_value(): + """Get a core value.""" + return 42 \ No newline at end of file diff --git a/gazelle/python/testdata/deps_to_remove_with_order/deps-order.txt b/gazelle/python/testdata/deps_to_remove_with_order/deps-order.txt new file mode 100644 index 0000000000..07b49b5245 --- /dev/null +++ b/gazelle/python/testdata/deps_to_remove_with_order/deps-order.txt @@ -0,0 +1,3 @@ +core.py +utils.py +high_level.py diff --git a/gazelle/python/testdata/deps_to_remove_with_order/high_level.py b/gazelle/python/testdata/deps_to_remove_with_order/high_level.py new file mode 100644 index 0000000000..b6ff8eca5e --- /dev/null +++ b/gazelle/python/testdata/deps_to_remove_with_order/high_level.py @@ -0,0 +1,30 @@ +# Copyright 2023 The Bazel Authors. All rights reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""High-level functionality - depends on both core and utils.""" + +import core +import utils + +def high_level_operation(): + """High-level operation using both core and utils.""" + core_result = core.core_function() + utils_result = utils.utility_function() + return f"High level: {core_result} + {utils_result}" + +def process_data(): + """Process data using all available functionality.""" + value = core.get_core_value() + formatted = utils.format_output(value) + return f"Processed: {formatted}" \ No newline at end of file diff --git a/gazelle/python/testdata/deps_to_remove_with_order/test.yaml b/gazelle/python/testdata/deps_to_remove_with_order/test.yaml new file mode 100644 index 0000000000..fcea77710f --- /dev/null +++ b/gazelle/python/testdata/deps_to_remove_with_order/test.yaml @@ -0,0 +1,15 @@ +# Copyright 2023 The Bazel Authors. All rights reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +--- diff --git a/gazelle/python/testdata/deps_to_remove_with_order/utils.py b/gazelle/python/testdata/deps_to_remove_with_order/utils.py new file mode 100644 index 0000000000..360c2bb05e --- /dev/null +++ b/gazelle/python/testdata/deps_to_remove_with_order/utils.py @@ -0,0 +1,27 @@ +# Copyright 2023 The Bazel Authors. All rights reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""Utility functions - depends on core.""" + +import core + +def utility_function(): + """Utility function that uses core functionality.""" + base_value = core.get_core_value() + return f"utility result: {base_value * 2}" + +def format_output(value): + """Format output using core function.""" + core_result = core.core_function() + return f"{core_result} -> {value}" \ No newline at end of file From 46377ad2cb474846b8fc49573a8b738c98320d34 Mon Sep 17 00:00:00 2001 From: Vivek Dasari Date: Wed, 16 Jul 2025 10:16:35 -0700 Subject: [PATCH 3/3] use median instead of average --- SUMMARY.md | 188 ++++++++++++++++++++++++++++++++++++++ gazelle/python/resolve.go | 36 +++++--- 2 files changed, 210 insertions(+), 14 deletions(-) create mode 100644 SUMMARY.md diff --git a/SUMMARY.md b/SUMMARY.md new file mode 100644 index 0000000000..f9eb9d5263 --- /dev/null +++ b/SUMMARY.md @@ -0,0 +1,188 @@ +# Implementation Summary: `deps_to_remove` Feature for Gazelle Python Extension + +## Overview + +This document summarizes the implementation of the `deps_to_remove` feature for `py_library` targets in the Gazelle Python extension. The feature allows automatic generation of a `deps_to_remove` attribute that contains dependencies violating ordering constraints defined in a `deps-order.txt` file. + +## Feature Requirements + +- **All dependencies** must be included in the `deps` attribute (normal behavior) +- **Violating dependencies** must also be included in the `deps_to_remove` attribute +- **Dependency ordering** is defined by a `deps-order.txt` file at the repository root +- **Files listed earlier** in `deps-order.txt` can be depended upon by files listed later, but not vice versa + +## Implementation Details + +### 1. Attribute Support (`kinds.go`) + +**File**: `/workspaces/rules_python/gazelle/python/kinds.go` + +Added `deps_to_remove` attribute support to `pyLibraryKind`: +```go +MergeableAttrs: map[string]bool{ + "srcs": true, + "deps_to_remove": true, +}, +ResolveAttrs: map[string]bool{ + "deps": true, + "pyi_deps": true, + "deps_to_remove": true, +}, +``` + +### 2. Target Builder Enhancement (`target.go`) + +**File**: `/workspaces/rules_python/gazelle/python/target.go` + +- Added `depsToRemove` field to `targetBuilder` struct +- Added helper methods: `addDepToRemove()`, `addDepsToRemove()` +- Updated `build()` method to store source files for ordering constraints + +### 3. Dependency Order Resolution (`resolve.go`) + +**File**: `/workspaces/rules_python/gazelle/python/resolve.go` + +#### Core Components: + +**DepsOrderResolver Structure:** +```go +type DepsOrderResolver struct { + fileToIndex map[string]int // File to ordering index mapping + loaded bool // Loading state + importToSrcs map[string][]string // Import to source files mapping +} +``` + +**Key Methods:** +- `LoadDepsOrder()` - Parses `deps-order.txt` file +- `GetMedianIndex()` - Calculates median ordering index for source files +- `ShouldAddToDepsToRemove()` - Determines if dependency violates ordering +- `RegisterImportSources()` - Maps import specs to source files + +#### Dependency Processing Logic: + +1. **During Import Registration**: Map import specs to their source files +2. **During Dependency Resolution**: Register dependency labels to source files +3. **During Rule Finalization**: Apply ordering constraints to create `deps_to_remove` + +### 4. Language Integration (`language.go`) + +**File**: `/workspaces/rules_python/gazelle/python/language.go` + +Updated `NewLanguage()` to initialize the resolver with `DepsOrderResolver`: +```go +return &Python{ + Resolver: Resolver{ + depsOrderResolver: NewDepsOrderResolver(), + }, +} +``` + +## Algorithm Details + +### Ordering Constraint Logic + +1. **File Indexing**: Each file in `deps-order.txt` gets an index (0, 1, 2, ...) +2. **Median Calculation**: For targets with multiple sources, calculate median index +3. **Violation Detection**: If `currentTargetIndex < dependencyTargetIndex`, it's a violation +4. **Attribute Population**: Violating dependencies are added to both `deps` and `deps_to_remove` + +### Path Matching Strategy + +The implementation handles path matching flexibly: +- Tries exact path matches first (`pkg/file.py`) +- Falls back to filename matches (`file.py`) +- Supports both repo-relative and package-relative paths + +## Test Coverage + +### Test Case 1: Valid Dependencies (`deps_to_remove_with_order`) + +**Files**: `core.py` → `utils.py` → `high_level.py` + +**Scenario**: All dependencies follow correct ordering +- `utils.py` depends on `core.py` ✅ (valid: index 1 → index 0) +- `high_level.py` depends on both ✅ (valid: index 2 → index 0,1) + +**Expected Result**: No `deps_to_remove` attributes (all dependencies are valid) + +### Test Case 2: Ordering Violations (`deps_to_remove_ordering_violation`) + +**Files**: `foundation.py` → `middleware.py` → `application.py` + +**Scenario**: Contains dependency ordering violations +- `foundation.py` depends on `middleware.py` ❌ (violation: index 0 → index 1) +- `middleware.py` depends on `application.py` ❌ (violation: index 1 → index 2) + +**Expected Result**: +- `foundation` target: `deps_to_remove = [":middleware"]` +- `middleware` target: `deps_to_remove = [":application"]` + +## File Structure + +``` +gazelle/python/ +├── kinds.go # Attribute definitions +├── target.go # Target building logic +├── resolve.go # Dependency resolution & ordering +├── language.go # Language initialization +└── testdata/ + ├── deps_to_remove_with_order/ # Valid dependencies test + │ ├── deps-order.txt + │ ├── core.py, utils.py, high_level.py + │ ├── BUILD.in, BUILD.out + │ └── test.yaml + └── deps_to_remove_ordering_violation/ # Violation test + ├── deps-order.txt + ├── foundation.py, middleware.py, application.py + ├── BUILD.in, BUILD.out + └── test.yaml +``` + +## Usage + +### 1. Create `deps-order.txt` at repository root: +``` +# Comments are supported +core/base.py +utils/helpers.py +features/advanced.py +``` + +### 2. Run Gazelle: +```bash +bazel run //:gazelle +``` + +### 3. Generated BUILD files will include `deps_to_remove`: +```python +py_library( + name = "advanced", + srcs = ["advanced.py"], + deps = [ + "//core:base", # Valid dependency + "//utils:helpers", # Valid dependency + ], + # deps_to_remove is empty - no violations +) + +py_library( + name = "helpers", + srcs = ["helpers.py"], + deps = ["//features:advanced"], # Invalid dependency + deps_to_remove = ["//features:advanced"], # Marked for removal +) +``` + +## Benefits + +1. **Automated Detection**: Automatically identifies dependency ordering violations +2. **Build Compatibility**: All dependencies remain in `deps` for build correctness +3. **Tooling Integration**: `deps_to_remove` can be used by linters, analyzers, etc. +4. **Flexible Configuration**: Simple text file configuration +5. **Backward Compatible**: No `deps-order.txt` means no constraints applied + + +--- + +This implementation provides a robust foundation for dependency ordering enforcement in Python projects using Bazel and Gazelle. diff --git a/gazelle/python/resolve.go b/gazelle/python/resolve.go index 125e573827..957e108391 100644 --- a/gazelle/python/resolve.go +++ b/gazelle/python/resolve.go @@ -20,6 +20,7 @@ import ( "log" "os" "path/filepath" + "sort" "strings" "github.com/bazelbuild/bazel-gazelle/config" @@ -102,31 +103,38 @@ func (d *DepsOrderResolver) LoadDepsOrder(repoRoot string) error { return nil } -// GetAverageIndex calculates the average index for a set of source files -func (d *DepsOrderResolver) GetAverageIndex(srcs []string) float64 { +// GetMedianIndex calculates the median index for a set of source files +func (d *DepsOrderResolver) GetMedianIndex(srcs []string) float64 { if len(d.fileToIndex) == 0 { return 0 // No ordering file, return 0 } - totalIndex := 0 - validSrcs := 0 + var indices []int for _, src := range srcs { // Try both the full path and just the filename filename := filepath.Base(src) if index, exists := d.fileToIndex[src]; exists { - totalIndex += index - validSrcs++ + indices = append(indices, index) } else if index, exists := d.fileToIndex[filename]; exists { - totalIndex += index - validSrcs++ + indices = append(indices, index) } } - if validSrcs == 0 { + if len(indices) == 0 { return float64(len(d.fileToIndex)) // Files not in order get max index } - return float64(totalIndex) / float64(validSrcs) + // Sort indices to find median + sort.Ints(indices) + n := len(indices) + + if n%2 == 0 { + // Even number of elements: average of two middle elements + return float64(indices[n/2-1]+indices[n/2]) / 2.0 + } else { + // Odd number of elements: middle element + return float64(indices[n/2]) + } } // ShouldAddToDepsToRemove returns true if the dependency should be added to deps_to_remove based on ordering constraints @@ -135,11 +143,11 @@ func (d *DepsOrderResolver) ShouldAddToDepsToRemove(currentTargetSrcs []string, return false // No ordering constraints } - currentAvg := d.GetAverageIndex(currentTargetSrcs) - depAvg := d.GetAverageIndex(depTargetSrcs) + currentMedian := d.GetMedianIndex(currentTargetSrcs) + depMedian := d.GetMedianIndex(depTargetSrcs) - // If current target has lower average index than dependency, the dependency should be removed - return currentAvg < depAvg + // If current target has lower median index than dependency, the dependency should be removed + return currentMedian < depMedian } // RegisterImportSources registers the mapping between import specs and their source files