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

Special-case the Pants repo. #33

Closed
jsirois opened this issue Dec 19, 2022 · 22 comments · Fixed by #75
Closed

Special-case the Pants repo. #33

jsirois opened this issue Dec 19, 2022 · 22 comments · Fixed by #75
Assignees
Labels
enhancement New feature or request

Comments

@jsirois
Copy link
Contributor

jsirois commented Dec 19, 2022

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.

@jsirois jsirois added the enhancement New feature or request label Dec 19, 2022
@sureshjoshi
Copy link
Member

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
Copy link
Contributor Author

jsirois commented Dec 19, 2022

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
Copy link
Contributor Author

jsirois commented Dec 19, 2022

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
Copy link
Member

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
Copy link
Contributor Author

jsirois commented Dec 19, 2022

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.

@kaos kaos added the in progress Denotes an assigned issue is being actively worked on. label Jan 26, 2023
@kaos kaos self-assigned this Jan 26, 2023
@benjyw
Copy link
Contributor

benjyw commented Jan 26, 2023

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
Copy link
Member

kaos commented Jan 26, 2023

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
Copy link
Contributor

benjyw commented Jan 26, 2023

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
Copy link
Member

kaos commented Jan 26, 2023

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
Copy link
Contributor Author

jsirois commented Jan 26, 2023

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
Copy link
Contributor Author

jsirois commented Jan 26, 2023

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
Copy link
Member

kaos commented Jan 26, 2023

@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 :) )

@kaos kaos closed this as completed in #75 Feb 3, 2023
kaos added a commit that referenced this issue Feb 3, 2023
The choice of config section for the `pants.toml` config file was made
to have something that won't choke the regular Pants configuration
verification and also not show up in any online help.

Closes #33

When `delegate_bootstrap=true` in the `[DEFAULT]` section of 
`pants.toml` _and_ `pants_version` is unspecified (`PANTS_SHA` 
is also considered here) bootstrapping Pants is delegated to `./pants`.
@benjyw
Copy link
Contributor

benjyw commented Feb 3, 2023

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
Copy link
Contributor Author

jsirois commented Feb 3, 2023

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
Copy link
Member

kaos commented Feb 3, 2023

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
Copy link
Member

kaos commented Feb 3, 2023

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
Copy link
Contributor Author

jsirois commented Feb 3, 2023

Yeah, perhaps we could fix Pants here.

@jsirois
Copy link
Contributor Author

jsirois commented Feb 3, 2023

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

@kaos
Copy link
Member

kaos commented Feb 3, 2023

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
Copy link
Contributor Author

jsirois commented Feb 3, 2023

Yeah, there is already another ick:

scie-pants/src/main.rs

Lines 171 to 175 in 41900c4

if let Some(build_root) = build_root {
env.push((
"PANTS_BUILDROOT_OVERRIDE".into(),
build_root.as_os_str().to_os_string(),
));

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
Copy link
Member

kaos commented Feb 3, 2023

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
Copy link
Contributor Author

jsirois commented Feb 3, 2023

That sounds about right to me.

@jsirois jsirois removed the in progress Denotes an assigned issue is being actively worked on. label May 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants