Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Overrides not working with parametrized default #20933

Closed
isra17 opened this issue May 18, 2024 · 2 comments · Fixed by #20934
Closed

Overrides not working with parametrized default #20933

isra17 opened this issue May 18, 2024 · 2 comments · Fixed by #20934
Labels

Comments

@isra17
Copy link
Contributor

isra17 commented May 18, 2024

Describe the bug
I took the time to create an example off the example python repo. See here:

--- a/BUILD
+++ b/BUILD
@@ -4,4 +4,8 @@
 # A macro that turns every entry in this directory's requirements.txt into a
 # `python_requirement_library` target. Refer to
 # https://www.pantsbuild.org/docs/python-third-party-dependencies.
-python_requirements(name="reqs")
+python_requirements(name="reqs", resolve="python-default")
+python_requirements(name="other-reqs", resolve="python-other")
+
+__defaults__(all=dict(resolve=parametrize("python-default", "python-other")))
+
diff --git a/helloworld/translator/BUILD b/helloworld/translator/BUILD
index 7086f94..23ddf0b 100644
--- a/helloworld/translator/BUILD
+++ b/helloworld/translator/BUILD
@@ -4,6 +4,10 @@
 # This target sets the metadata for all the Python non-test files in this directory.
 python_sources(
     name="lib",
+    overrides={
+      "translator.py": dict(resolve="python-other"),  # Not working
+      "__init__.py": dict(resolve=parametrize("python-other")),  # Working
+    }
 )

After this patch, I would expect both helloworld/translator/translator.py and helloworld/translator/__init__.py to be only in python-other resolve.

However I get the following result:

$ pants list "helloworld/translator/translator.py"
helloworld/translator/translator.py:lib@resolve=python-default
helloworld/translator/translator.py:lib@resolve=python-other

$ pants list "helloworld/translator/__init__.py"
helloworld/translator/__init__.py:lib@resolve=python-other

Note that not using parametrize in the __defaults__ also make this work.

Pants version
Which version of Pants are you using?

2.20.0

@isra17 isra17 added the bug label May 18, 2024
@isra17
Copy link
Contributor Author

isra17 commented May 19, 2024

I nailed it down to this: isra17@1cc132b

Parametrize.expand with the override will only update the address given the value is a Parametrize. Is suppose the fix is to check if an override is also one of the current address parameter and update as needed.

@kaos
Copy link
Member

kaos commented May 20, 2024

Excellent find!

I'd just like to point out that this is a address issue, the actual resolve is correct:

❯ pants peek --exclude-defaults "helloworld/translator/translator.py"
[
  {
    "address": "helloworld/translator/translator.py:lib@resolve=python-default",
    "target_type": "python_source",
    "dependencies": [],
    "resolve": "python-other",
    "source_raw": "translator.py",
    "sources": [
      "helloworld/translator/translator.py"
    ],
    "sources_fingerprint": "143d1a81fc121bc539e5fa40c79bb8e2fa3806d84420b11b323fc88fca57a61e"
  },
  {
    "address": "helloworld/translator/translator.py:lib@resolve=python-other",
    "target_type": "python_source",
    "dependencies": [],
    "resolve": "python-other",
    "source_raw": "translator.py",
    "sources": [
      "helloworld/translator/translator.py"
    ],
    "sources_fingerprint": "143d1a81fc121bc539e5fa40c79bb8e2fa3806d84420b11b323fc88fca57a61e"
  }
]

kaos pushed a commit that referenced this issue May 28, 2024
This fix a bug where default parametrize resolve could not get
overriden.

The fix ensure that an address parameters are updated on any override,
not just parametrized one.

Fixes #20933
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants