https://pantsbuild.org/ logo
Powered by
b

bitter-ability-32190

02/05/2022, 1:43 AM
Bikeshed on using "`./pants` " is a user-visible string 🧵
In retrofitting this code to be static: https://github.com/pantsbuild/pants/blob/eda4092620246eea42d17c6a17a4d30627493fa9/src/python/pants/backend/go/lint/gofmt/subsystem.py#L19 We dynamically use the pants bin name, which is cool, but makes declaring it statically a bit harder. However, maaaaany (at least 100) other user-visible strings just hardcode 
./pants
So, my vote would be to replace it with 
./pants
, and then visit the whole repo on how best to "print" the bin name (E.g. an OS env var)
or 
sys.argv
(if it's set)
h

happy-kitchen-89482

02/05/2022, 5:26 AM
Hmm
I think there is value in being able to parameterize this
Although we cannot do so in practice in the online docs
c

curved-television-6568

02/05/2022, 7:13 AM
🤔 also if the pants shim becomes a thing, it would become 
pants
without the dot-slash prefix. At least as a possible alternative..
b

bitter-ability-32190

02/05/2022, 1:45 PM
I think if this was injected via other means (env var or sys.argv) you'd still get the online docs right, as you generate it dynamically not statically
👍 1
Another alternative is to just use 
pants
as it is the name of the module executing. However, that's likely more confusing.
h

hundreds-father-404

02/05/2022, 2:27 PM
yeah I really wish we could define a global constant 
PANTS_BIN
that you can use consistently, regardless of 
help
message vs error message etc. Maybe we can if we'd consider dropping the proper global option and instead inspecting 
sys.argv[0]
?
argv0 wont' work: 
/Users/ericarellano/code/pants/src/python/pants/bin/pants_loader.py
. So I think the solution would be to use env var, and drop the option. Also note that the bash script we distribute 
./pants
is setting 
--pants-bin-name="${PANTS_BIN_NAME}"
. We'd need to update it to do 
export PANTS_BIN_NAME
instead. It's nice that that script has this line 
PANTS_BIN_NAME="${PANTS_BIN_NAME:-$0}"
👍 1
One downside of dropping the option is it's harder to document. Won't show up in 
./pants help
. Any ideas where we would mention this? In the 
./pants
bash script itself via comment works, along with maybe https://www.pantsbuild.org/v2.10/docs/existing-repositories#migrating-from-other-build-tools-set-custom-build-file-names. And maybe when we first introduce the script at https://www.pantsbuild.org/v2.10/docs/installation#creating-the-launch-script via an info toolbox
h

happy-kitchen-89482

02/05/2022, 3:44 PM
argv doesn't work
b

bitter-ability-32190

02/05/2022, 4:28 PM
@happy-kitchen-89482 but an ENV var would work, right? So I think the path forward would be to use that, and expose the value via a helper method, so code isn't hardcoding it everywhere.
Then maybe a dirty linter or script that validates strings don't have 
"./pants"
except for some whitelisted code
h

happy-kitchen-89482

02/05/2022, 10:05 PM
I don't get how an out-of-band env var is better than the option, which can be set via env var?
b

bitter-ability-32190

02/05/2022, 10:59 PM
I think we got wires crossed. I'm not trying to change the call-site. Once we're executing pants' Python code, I'm suggesting we set an env var from the option and use that to render user-visible strings
h

happy-kitchen-89482

02/06/2022, 1:56 AM
Ah, then I am misunderstanding
But I still don't get it. This is so we don't have to plumb the string through places?
b

bitter-ability-32190

02/06/2022, 2:09 AM
Yes, kind of. More about being consistent across all user-defined strings. But also about permission to hardcode 
./pants
where we previously weren't 😈
h

hundreds-father-404

02/06/2022, 3:42 AM
Oh my thinking was: 1. It is awkward currently to determine the pants script name because you must consume the option. To do that, we have verbose things like 
register.bootstrap.pants_bin.name
or requesting 
PantsBinName
as a parameter to your rule. It's verbose and not consistent. 2. It would be convenient to have one consistent way to access the value, a constant like 
PANTS_BIN
. Similar to how we have 
VERSION
3. To get that constant to work, it can't consume the options system. So instead, consume an env var
h

happy-kitchen-89482

02/06/2022, 4:45 PM
OK, that makes sense. But I think @bitter-ability-32190 is suggesting setting an env var internally from the option, at startup? In fact we could use the same env var name as the option does (PANTS_BIN_NAME), but just set it internally if not already set
h

hundreds-father-404

02/06/2022, 5:03 PM
If that's possible, that'd be excellent! I'm not sure how to get that working correctly with initialization ordering though -- constants are created at import time. I think we'd need a function 
pants_name()
that will lazily read in the env var. That seems good The key thing I care about is having one single, ergonomic way to get the value. No need to pass in bootstrap options
b

bitter-ability-32190

02/06/2022, 5:31 PM
Actually yeah I had envisioned reusing the same env far simplicity. And yeah, it couldn't be a constant, but a function (although it could be a constant that we monkeypatch, but that's funny)
Also panta_bin_name is probably a good function name.
Sorry I should've been clearer with my thoughs
h

happy-kitchen-89482

02/06/2022, 6:02 PM
My bad, I wasn't grokking you at all
b

bitter-ability-32190

02/08/2022, 5:47 PM
Powered by