pantsbuild / scie-pants

Protects your Pants from the elements.
https://www.pantsbuild.org/docs/installation
Apache License 2.0
19 stars 18 forks source link

Special-case the Pants repo. #33

Closed jsirois closed 1 year ago

jsirois commented 1 year ago

Until the scie-pants binary can actually be used by Pants developers in the Pants repo, it would be good if it could detect it was running in the Pants repo.

Perhaps with:

[bootstrap]
delegate = true

And then just run the ./pants script in the Pants repo.

sureshjoshi commented 1 year ago

If the goal is to make the ./pants script redundant, I think it would be better to just warn/error out, in lieu of an extra configuration which needs docs and a breaking config change if we want to eliminate it eventually.

If the goal is to always keep the ./pants script, then this feature would be nice.

jsirois commented 1 year ago

I think it would be better to just warn/error out

The thing is, to warn / error out, there needs to be some signal. I was suggesting [bootstrap] delegate = true is that signal. Once you have the signal, you just call ./pants blindly. I don't see the need for docs - there is exactly 1 repo on earth that will have the knob and then remove the knob. The knob would not be documented in Pants, it would just not be aggressively errored out on as unknown. Could name it _delegate, __DELEGATE_YOU_DO_NOT_WANT_THIS_OPTION_IF_YOU_ARE_NOT_ME__ - what have you.

jsirois commented 1 year ago

We could make the signal a marker file of some sort - anything. It just needs some cooperation from the Pants repo so I don't have to add crazy tea-leaf reading logic to scie-pants in the hot path.

sureshjoshi commented 1 year ago

I was originally wondering if maybe there was auto-delegation to a local ./pants script (regardless if it was the Pants mainline repo or not. But then something in the back of my head doesn't like blindly calling bash scripts by collocated name.

However, having that explicit

[bootstrap]
delegate = true

does make that concern go away 🤷🏽

The knob would not be documented in Pants

That's a good point - it doesn't need to be documented, because it's not really a user-facing API.

jsirois commented 1 year ago

Ok, cool. Yeah, the scie-pants already suffers the cost of parsing pants.toml; so having the signal be there is definitely the most efficient and clear cut / will incur the least tech debt.

benjyw commented 1 year ago

Counterpoint - running a pants release on the Pants repo allows Pants to be self-hosting, which has interesting benefits. E.g., once we add Rust support we could build the engine with Pants, and enjoy remote caching etc.

So if that's a direction we want to go in, maybe running pants in the pants repo should do what it implies.

kaos commented 1 year ago

I'm not sure I follow the "self-hosting" argument.

Are you suggesting that pants ... should invoke Pants as a regular project where as ./pants ... would be the development invocation to run Pants with the sources from disk as we do now?

If so, I would humbly suggest renaming so we have ./pants_from_sources .. or something similar to have it distinctly different from the regular invocation style of pants ...

benjyw commented 1 year ago

Yeah, I'm suggesting that pants in the pants repo should run a pants release, and ./pants should do what ./pants_from_sources does in every other repo that has it, so I'd be fine renaming it. But other people may have other opinions on this... So I'm not suggesting we do either thing without buy-in.

kaos commented 1 year ago

Also, the linked PR suggests having scie pants delegate to ./pants as an opt-in feature, so we can do that first without closing the door on this down the line..

jsirois commented 1 year ago

So if that's a direction we want to go in, maybe running pants in the pants repo should do what it implies.

Finishing that thought, it then needs to error if pants_version is not set in the Pants repo (or set it to latest stable). How exactly is that supposed to work @benjyw with our generally out of control deprecation cycles? Do we set pants_version to the latest mainline release with every mainline (~dev) release? And then if a new option is added + used in Pants own pants.toml do all devs who pull the new option on main find out with a config error and then switch to pants from sources?

I guess I'd appreciate you walking through the full detail set.

jsirois commented 1 year ago

FWIW I'm currently hacking on #30 which makes this even more untenable. At that point pants is pants_from_sources and it expects a ./pants script in the Pants repo (any name - name is not important!) - and that script is what is custom and runs pants from sources. Basically you'd run PANTS_SOURCE=. pants to get pants to run ./pants and ... a bit out of control IIUC. @kaos I thought your whole point was you wanted to be able to not question your muscle memory and always run pants everywhere to run the appropriate version of Pants. And in the Pants repo that's from sources for all normal purposes.

kaos commented 1 year ago

@kaos I thought your whole point was you wanted to be able to not question your muscle memory and always run pants everywhere to run the appropriate version of Pants. And in the Pants repo that's from sources for all normal purposes.

Correct, I do.

At the same time, if there's desire to do what Benjy suggested I could get on board that (if the details play out) merely observed that in order for my mind to cope with that, the dev incantation would need to be further away from the regular pants than just tacking ./ in front of it.. i.e. I'm willing to re-learn my mental muscles for that case. (to be clear, I'd rather not though :) )

benjyw commented 1 year ago

I don't know exactly what the details would be, there would need to be some finessing. Maybe we take the most recent release, or the most recent stable release, or maybe indeed running from sources is the norm and running from a release is the exception, so we require you to set an env var with the version in order to run a release.

But the ability to run a released version of Pants in the pants repo is substantial (e.g., if we had Rust support we could avoid a ton of engine rebuilding), whereas having pants really invoke ./pants in that repo seems like sugar. So I don't want to foreclose on the former to support the latter. If they can coexist, fine.

jsirois commented 1 year ago

Ok, that all already works just like any other repo. Define pants_version in pants.toml or via PANTS_VERSION. Afaict, the only way in which pants works differently in the Pants repo, is by shortcutting the pants from sources mechanism with the new config pseudo option @kaos added. You can still run from sources via env var too.

kaos commented 1 year ago

that all already works just like any other repo.

Not entirely true, though.

If you attempt to run with a released Pants, boom:

PANTS_VERSION=2.16.0.dev5 pants -V
Bootstrapping Pants 2.16.0.dev5 using cpython 3.9.15
Installing pantsbuild.pants==2.16.0.dev5 into a virtual environment at /Users/andreas.stenius/Library/Caches/nce/777744d4d26ad153bf39bb2f68dbc8d1a4f1067a710fa4c9e92087b110389d72/bindings/venvs/2.16.0.dev5
New virtual environment successfully created at /Users/andreas.stenius/Library/Caches/nce/777744d4d26ad153bf39bb2f68dbc8d1a4f1067a710fa4c9e92087b110389d72/bindings/venvs/2.16.0.dev5.
09:38:59.30 [ERROR] 1 Exception encountered:

Engine traceback:
  in select
    ..
  in pants.core.util_rules.environments.determine_local_environment
    ..
  in pants.core.util_rules.environments.determine_all_environments
    ..
  in construct_scope_environments_preview
    ..

Traceback (most recent call last):
  File "/Users/andreas.stenius/src/github/kaos/pants/src/python/pants/engine/internals/selectors.py", line 623, in native_engine_generator_send
[...]
  File "/Users/andreas.stenius/src/github/kaos/pants/src/python/pants/engine/internals/selectors.py", line 118, in __await__
    result = yield self
pants.base.exceptions.BuildConfigurationError: Version mismatch: Requested version was 2.16.0.dev5, our version is 2.16.0.dev6.

Use -ldebug for more logs.
See https://www.pantsbuild.org/v2.16/docs/troubleshooting for common issues.
Consider reaching out for help: https://www.pantsbuild.org/v2.16/docs/getting-help

due to:

https://github.com/pantsbuild/pants/blob/f807172caadc2c610aa7f8d48b06f58b7ef9d453/src/python/pants/option/options_bootstrapper.py#L286-L294

kaos commented 1 year ago

Oh, or I misread the "that all already works".. from scie-pants point of view, it indeed works. It's just Pants that doesn't cope with it.

jsirois commented 1 year ago

Yeah, perhaps we could fix Pants here.

jsirois commented 1 year ago

The proliferation of jim-crackery is wide: https://github.com/pantsbuild/pants/blob/main/src/python/pants/version.py#L13-L14

kaos commented 1 year ago

The proliferation of jim-crackery is wide: https://github.com/pantsbuild/pants/blob/main/src/python/pants/version.py#L13-L14

Oh, wow! wasn't aware of that one, but that could work.. but feels.. icky. :D

Edit:

It does indeed work, and exposes another (to me already known) issue regarding the explorer backend:

_PANTS_VERSION_OVERRIDE=2.16.0.dev5 PANTS_VERSION=2.16.0.dev5 pants -V
[...]
pants.base.exceptions.BackendConfigurationError: Failed to load the pants.explorer.server.register backend: ModuleNotFoundError("No module named 'fastapi'")
[...]
jsirois commented 1 year ago

Yeah, there is already another ick: https://github.com/pantsbuild/scie-pants/blob/41900c4733e221d17f76707cf005ab463bacf938/src/main.rs#L171-L175

That env var was intended for tests only, it just doesn't have the leading underscore to look as obviously icky. So there are paths forward here and the ick could be solidified with some code comments in Pants at the very least about production use of these knobs by scie-pants.

kaos commented 1 year ago

Aha, so I get the feeling the correct thing to do would be to provide _PANTS_VERSION_OVERRIDE={pants_version} if we have delegate_bootstrap and then continue to run the normal pants flow, as that would "cancel" the delegation to ./pants and provide the "fix" to work with a released version of Pants in the pants repo.

I can put up a PR for this later tonight.

jsirois commented 1 year ago

That sounds about right to me.