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

Expose current BUILD files globals to macros #19284

Closed
wants to merge 3 commits into from

Conversation

kaos
Copy link
Member

@kaos kaos commented Jun 9, 2023

It came up in slack (linen) a question about accessing global symbols declared in the current BUILD file.

Although this may be a sign of bad design to need this, it was not too difficult to support. (also, the error message is confusing as **, as it suggests it should work by including the BUILD file globals in the output for registered symbols).

As such I submit this PR as POC and for discussion.

There are three commits in this PR, and if we decide to not support accessing the current BUILD files globals from macros, the first two commits still has good improvements for error reporting in case of unknown symbols (by including the location of the error properly).

If we do decide to keep all of it, I'll add a few lines to the docs before proceeding.

--

From a prelude (macro file) you may access the current BUILD files global from a BUILD scope.

Adjusted example from the above linked thread: (added BUILD. prefix to the variable reference in the macro):

def my_special_wrapper(**kwargs):
    real_target(something=BUILD.MY_SPECIAL_GLOBAL_VARIABLE, **kwargs)

and a BUILD file with something like

MY_SPECIAL_GLOBAL_VARIABLE = "something"

my_special_wrapper()

@jsirois
Copy link
Member

jsirois commented Jun 9, 2023

Although this may be a sign of bad design to need this

I'll just note Python doesn't work this way; so its definitely weird to expect this should work I think. If I write a library function in Python (the macro), I cannot see the callers symbols unless I go to lengths using the traceback module or the like to walk up the stack, get the calling module, then get its globals by mucking with sys.modules[...].

@kaos
Copy link
Member Author

kaos commented Jun 9, 2023

Although this may be a sign of bad design to need this

I'll just note Python doesn't work this way; so its definitely weird to expect this should work I think. If I write a library function in Python (the macro), I cannot see the callers symbols unless I go to lengths using the traceback module or the like to walk up the stack, get the calling module, then get its globals by mucking with sys.modules[...].

Thanks, yes. I've not entirely convinced myself this is a good idea either. But if we choose to not have this I think we want to do something about the error message presented, as that doesn't consider this distinction at all.

--
I've opened a dedicated PR for just the error message tweaks that got into this one.

@kaos
Copy link
Member Author

kaos commented Jun 9, 2023

But if we choose to not have this I think we want to do something about the error message presented, as that doesn't consider this distinction at all.

Then again, this doesn't really fix that any way, as you must prefix the BUILD globals with BUILD. (was done to keep the implementation easier/sane to avoid having to cleanup the macro globals between every BUILD file.)

It was a fun exercise ;P

@alonsodomin
Copy link
Contributor

alonsodomin commented Jun 14, 2023

I'm not really on support of something like this even if Python itself did have this behavior. In a way, this looks to me like a language wart and even though Python is the primary language supported by Pants, being a monorepo multi-language tool, we shouldn't expect all the users to be familiar with this kind of constructs as not all may be Python developers themselves.

Besides, while macros are helpful, they also are somewhat obscure and in large repos may become quite complicated to find out where the given value is coming from.

The way I'm mostly familiar to achieve something like this would be by using a callback in the calling function that encloses around the outer context. That in a way would complicate the syntax so I'm unsure about its UX.

All that said, don't take it as a blocking opinion, if we decide to go with this let's at least ensure we have really good error messages that allow to traceback where the value of that global variable has been taken from.

@kaos
Copy link
Member Author

kaos commented Jun 14, 2023

Thanks for chiming in @alonsodomin

I don't think we will land this, as it's rather niche/obscure and there are better ways to solve it on the user side of things (such as with callbacks or curried methods etc).

@kaos
Copy link
Member Author

kaos commented Jun 14, 2023

I'll mark it as draft to indicate it as more of a POC/proposal, and close in a week unless it picks up attention in favour.

@kaos kaos marked this pull request as draft June 14, 2023 14:05
@kaos kaos closed this Jun 26, 2023
@kaos kaos deleted the kaos/build-file-globals-in-macros branch July 17, 2023 01:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants