indygreg / indygreg/PyOxidizer

Global constants not available in Starlark files that pyoxidizer.bzl loads

Open
#331 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Rust
Stars
6.2k
Forks
256
PR merge metrics
No merged PRs in 30d

Description

@indygreg closed #328 by committing 2dbe0d2, thereby adding support for Starlark's [`load` statement](https://github.com/bazelbuild/starlark/blob/master/spec.md#load-statements). This makes it easier to share target definitions, leaving minimal configuration to downstream projects. Ideally loaded modules would have access to the same global symbols as the main `pyoxidizer.bzl` that `pyoxidizer build` is executing. Currently, [global constants](https://pyoxidizer.readthedocs.io/en/latest/config_globals.html#global-constants) don't work, even though [global functions](https://pyoxidizer.readthedocs.io/en/latest/config_globals.html#global-functions) do. Per https://github.com/indygreg/PyOxidizer/issues/328#issuecomment-731657497 I'm opening this ticket for further discussion.

Current installed version of PyOxidizer: 6875c5fd1f5c6df3c1a24734868393a1ae19cb57

```console
$ pyoxidizer --version
PyOxidizer 0.11.0-pre
commit: 6875c5fd1f5c6df3c1a24734868393a1ae19cb57
source: /Users/wks/.cargo/git/checkouts/pyoxidizer-04b02466db45c615/6875c5f
pyembed crate location: path = "/Users/wks/.cargo/git/checkouts/pyoxidizer-04b02466db45c615/6875c5f/pyembed"
```

# Failing Test Case
Place the following snippets into an otherwise empty directory.

**`pyoxidizer.bzl`**
```bzl
load("helper.bzl", "test")
resolve_targets()
```

**`helper.bzl`**
```bzl
def test():
cwd = CWD
triple = BUILD_TARGET_TRIPLE
config_path = CONFIG_PATH
print(cwd, triple, config_path)
register_target("test", test, default=True)
```

I would expect `pyoxidizer build` to print the values of the global constants. This is the **`pyoxidizer build` output**.
```console
$ pyoxidizer build
resolving 1 targets
resolving target test
error[CM01]: Variable 'CWD' not found
--> helper.bzl:2:11
|
2 | cwd = CWD
| ^^^ Variable was not found

error: Variable 'CWD' not found
```
Commenting out the first and the second lines of `test()` show that each of the three global constants are unavailable.

# Current Workaround

The following modified files work as I was hoping the example above would.

**`pyoxidizer.bzl`**
```bzl
def project_settings():
return dict(
CWD = CWD,
BUILD_TARGET_TRIPLE = BUILD_TARGET_TRIPLE,
CONFIG_PATH = CONFIG_PATH,
)
register_target("project_settings", project_settings)
load("helper.bzl", "test")
resolve_targets()
```

**`helper.bzl`**
```bzl
def test(project_settings):
cwd = project_settings["CWD"]
triple = project_settings["BUILD_TARGET_TRIPLE"]
config_path = project_settings["CONFIG_PATH"]
print(cwd, triple, config_path)
register_target("test", test, depends=["project_settings"], default=True)
```

**Output from `pyoxidizer build`**
```console
$ pyoxidizer build
resolving 1 targets
resolving target test
resolving target project_settings
/Users/wks/Documents/pyox-test/. x86_64-apple-darwin ./pyoxidizer.bzl
error: NoneType does not implement build()
```

# Discussion

I'll respond to points from https://github.com/indygreg/PyOxidizer/issues/328#issuecomment-731657497, intentionally out of order.

> However, any variable mutations within the `load()`ed file will not be reflected outside that file's `Environment`.

That's good. I think other behavior would surprise Python programmers. Certainly it would surprise me.

> Furthermore, if we attempt to clone and pass the child environment containing the variables to the loaded file's context, we get an error due to mutably borrowing a frozen variable. I'm unsure what is up here. It is possible we might be able to work around it.

The main `pyoxidizer.bzl` environment appears to be created here:

https://github.com/indygreg/PyOxidizer/blob/6875c5fd1f5c6df3c1a24734868393a1ae19cb57/pyoxidizer/src/starlark/eval.rs#L64-L79

> The `Environment` (which is the thing containing variables) is different for the file calling `load()` and the file be loaded. These can't be the same exact object: the best we can do here is clone the current environment: that should make the variables from the calling file available to the file being loaded.

Correct me if I'm wrong—the cloning appears to happen here (omitting the error handling):

https://github.com/indygreg/PyOxidizer/blob/6875c5fd1f5c6df3c1a24734868393a1ae19cb57/pyoxidizer/src/starlark/eval.rs#L99-L111

Perhaps one solution would be to apply [`crate::starlark::env::populate_environment`](https://github.com/indygreg/PyOxidizer/blob/6875c5fd1f5c6df3c1a24734868393a1ae19cb57/pyoxidizer/src/starlark/env.rs#L185-L232) at [line 73](https://github.com/indygreg/PyOxidizer/blob/6875c5fd1f5c6df3c1a24734868393a1ae19cb57/pyoxidizer/src/starlark/eval.rs#L73) above to `file_loader_env`? Or would we need to apply to `file_loader_env` just the few lines of `populate_environment` shown below?

https://github.com/indygreg/PyOxidizer/blob/6875c5fd1f5c6df3c1a24734868393a1ae19cb57/pyoxidizer/src/starlark/env.rs#L211-L219

Contributor guide

No contributing guide indexed for this repository

Research direction

Start in pyoxidizer/src/starlark/eval.rs around environment creation and loading, then read populate_environment in pyoxidizer/src/starlark/env.rs. Reproduce the issue with the supplied pyoxidizer.bzl and helper.bzl files using pyoxidizer build. Done means the loaded helper can access CWD, BUILD_TARGET_TRIPLE, and CONFIG_PATH and the build prints their values without the missing-variable error.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
build-system, tooling
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Clearly specified
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.