https://pantsbuild.org/ logo
Join Slack
Powered by
Hi guys, I would be grateful if someone has a clue...
# general
f
Hi guys, I would be grateful if someone has a clue how to support using drawio as a system_binary:
Copy code
system_binary(
    name="drawio",
    binary_name="drawio",
    fingerprint_args=["--version"],
)
This leads to
Copy code
ValueError: Could not find a binary with name `drawio`. The following paths were searched: /bin, /opt/drawio, /opt/homebrew/bin, /usr/bin, /usr/local/bin, /usr/sbin.
When looking at the generated __run.sh
Copy code
#!/usr/bin/env bash
# This command line should execute the same process as pants did internally.
cd /tmp/pants-sandbox-lpxUQZ
env -i  /opt/drawio/drawio --version
and executing it manually, it reveals the problem
Copy code
[134070:0105/234425.825288:ERROR:ozone_platform_x11.cc(240)] Missing X server or $DISPLAY
[134070:0105/234425.825328:ERROR:env.cc(257)] The platform failed to initialize.  Exiting.
./__run.sh: line 4: 134070 Segmentation fault      (core dumped) env -i /opt/drawio/drawio --version
Drawio relies on the DISPLAY variable which is lost after executing with 
env -i
. Is there an option to preserve a named list of environment variables when fingerprinting?
l
I’m not sure about doing it for just fingerprinting, but you can use the 
subprocess-environment
subsystem to configure which env vars are included globally: https://www.pantsbuild.org/docs/reference-subprocess-environment
I think the following in 
pants.toml
should work:
Copy code
[subprocess-environment]
env_vars.add = ["DISPLAY"]
f
Thanks for the hint. This does not work for me, however. The __run.sh file is always the same. Even setting it to
Copy code
[subprocess-environment]
env_vars = [ "FOO=BAR" ]
Does not appear in the file.
🤔 1
Btw, in the __run.sh file you also see that actually no variable is preserved. The option 
-i
completely clears the environment. The link you mentioned https://www.pantsbuild.org/docs/reference-subprocess-environment says the default includes a couple of variables like 
LANG
, but they are not there, too. The variable 
LANG
is set in the environment of the pants run.
h
Unfortunately 
subprocess-environment
only works for python processes right now, its documentation is wrong.
We need to either re-document (and re-name?) it or, more likely, make it work for all processes.
l
I assume the problem is somewhere in here? https://github.com/pantsbuild/pants/blob/main/src/python/pants/core/util_rules/system_binaries.py I’m not super familiar with pants internals though
f
Mmh, so for my usecase, to run drawio it will need the 
DISPLAY
variable for fingerprinting and also for execution as 
_runnable_dependency_shims_...
later on. The execution also lacks the option to pass certain variables. I guess this is not possible yet?
Drawio is called by an 
adhoc_tool
, I guess this is worth mentioning.
extra_env_vars
adds the variable to the adhoc_tool invocation:
Copy code
adhoc_tool(
    name="build-html",
    runnable=":sphinx-build",
    args=["-M", "html", ".", "_build/html"],
    execution_dependencies=DOC_DEPS,
    runnable_dependencies=[
        ":drawio",
        ":git",
    ],
    extra_env_vars=[ "DISPLAY" ],
    output_directories=["_build/html"],
    root_output_directory="./_build/html",
    log_output=True,
)
This however does not solve the fingerprint issue, so the drawio executable can still not be found.
h
Not quite, the problem is in process execution more generally, system binaries is about locating the binaries to execute
@full-author-61014 can you explain the fingerprinting issue?
f
To use drawio I have to define it as 
system_binary
which will fingerprint it (whether I need that or not). The environment in which this is done cannot be modified, so it always looks like this
Copy code
#!/usr/bin/env bash
# This command line should execute the same process as pants did internally.
cd /tmp/pants-sandbox-lpxUQZ
env -i  /opt/drawio/drawio --version
env -i
kills all environment variables, however drawio relies on 
DISPLAY
. That's why it fails with
Copy code
[134070:0105/234425.825288:ERROR:<http://ozone_platform_x11.cc|ozone_platform_x11.cc>(240)] Missing X server or $DISPLAY
[134070:0105/234425.825328:ERROR:<http://env.cc|env.cc>(257)] The platform failed to initialize.  Exiting.
./__run.sh: line 4: 134070 Segmentation fault      (core dumped) env -i /opt/drawio/drawio --version
Is that helpful?
Pants aborts the build with
Copy code
ValueError: Could not find a binary with name `drawio`.
h
Ah, it needs to run it to fingerprint its version, and it can't run it without DISPLAY
?
Because we don't pass those 
extra_env_vars=[ "DISPLAY" ],
through when we run it to get its version?
So I think we need two fixes here: 1) 
system_binary
should take and use an 
extra_env_vars
, and 2) we should more generally make 
subprocess-environment
live up to its documentation
f
yes, exactly
As a short term solution I'm thinking of a workaround. I can have a shell script wrapper that calls system drawio and forwards all parameters unless it detects a special fingerprinting arg. Do you see another solution?
h
Well, we'd welcome a PR to fix 1)... 🙂
f
If I have a PR, does the 
pants_version
parameter also support setting a SHA or is that only possible on each invocation as described here https://www.pantsbuild.org/docs/installation#running-pants-from-unreleased-builds?
For future reference, I just cross-checked which variables drawio actually needs, here is a working command for my system:
Copy code
env -i DISPLAY=:0.0 HOME=/home/marco DBUS_SESSION_BUS_ADDRESS=unix:path=/run/user/1004/bus drawio --version
I will create an issue for that and try to assemble a PR soon. I guess improving pants is a better investment of my time than fiddling with workarounds 🙂
👆 1
h
To test your PR you probably want to run from the raw sources, not from an unreleased build
"I guess improving pants is a better investment of my time than fiddling with workarounds" are excellent words to live by!
That should be the project motto
f
Here is my PR https://github.com/pantsbuild/pants/pull/20374. It works for me locally. I assembled this quickly, hope I did not break any dev guidelines. Test case is still missing.
6 Views
Open in Slack
PreviousNext
Powered by