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.

Sign up for GitHub

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

Jump to bottom

Support using scie-pants for Pants dev work. #18158

Merged
merged 1 commit into from
Feb 3, 2023
Merged

Support using scie-pants for Pants dev work. #18158

merged 1 commit into from
Feb 3, 2023

Conversation

kaos
Copy link
Member

@kaos kaos commented Feb 2, 2023

In prep for pantsbuild/scie-pants#75

This allow us to run pants ... instead of ./pants ... to run Pants from sources in the Pants repo, once the above PR has been merged and released.

@kaos kaos added the category:internal CI, fixes for not-yet-released features, etc. label Feb 2, 2023
@kaos kaos requested a review from benjyw February 3, 2023 00:36
@benjyw
Copy link
Contributor

benjyw commented Feb 3, 2023

As mentioned elsewhere, I don't necessary agree with this change. I think it would be great to have an easy way to run a released version of Pants on the Pants repo, and this is basically stomping on that possibility...

@kaos
Copy link
Member Author

kaos commented Feb 3, 2023 •
edited
Loading

As mentioned elsewhere, I don't necessary agree with this change. I think it would be great to have an easy way to run a released version of Pants on the Pants repo, and this is basically stomping on that possibility...

OK. But I guess, for now at least, the default normal mode of operation is to run pants from source. So if we had an easy from the cli/env escape hatch to disable the ./pants delegation and instead use a pants release for the invocation; would that be an acceptable approach?

Given also that there's still some open questions about what it would mean to run a pants release over the pants sources..

My idea are along the lines of PANTS_VERSION=x pants ... (or PANTS_SHA=x pants ... could override the bootstrap_delegate option to use normal bootstrapping (basically, only delegate to ./pants if configured in pants.toml and we don't have a specific version of Pants configured to run). This wouldn't incur any extra overhead, as scie-pants already examines these env vars/configuration--it also solves the issue with the pants.toml not having a version configured in the pants source repo which would otherwise be required, so this solves to birds with one stone so to speak.

@benjyw
Copy link
Contributor

benjyw commented Feb 3, 2023

I just think "pants runs a released version, ./pants" runs from sources" is pretty easy to grok. This seems to add nuance and complication, to where it's not clear what's happening.

@kaos
Copy link
Member Author

kaos commented Feb 3, 2023

What's the motivation to run a released Pants on the pants source repo from the command line?

For me, I'm starting to use pants instead of ./pants, so this is the one odd exception to the rule for how I normally invoke pants.

I guess using a released Pants make sense to fmt lint check and maybe even test the Pants sources--when checking in the code primarily, so I can see the benefit if used from the pre-commit hook for instance.

@kaos
Copy link
Member Author

kaos commented Feb 3, 2023

This seems to add nuance and complication, to where it's not clear what's happening.

Running a release Pants, you'd still have to provide a version--and this doesn't even work (yet)

pants.base.exceptions.BuildConfigurationError: Version mismatch: Requested version was 2.14.1, our version is 2.16.0.dev6.

So I'm in favour of having a single well known entry point to running pants, and the fact that it in the pants repo delegates to ./pants is not a nuance you'd need to care about on a daily basis.

With the above, there's a clear way to escape this (not to mention really easy to revert; and if you prefer to use ./pants that still works too!) so until we actually can use a released Pants on this repo, may we proceed with this change? :)

@jsirois
Copy link
Contributor

jsirois commented Feb 3, 2023 •
edited
Loading

@benjyw I'd appreciate a response to pantsbuild/scie-pants#33 (comment)

@benjyw
Copy link
Contributor

benjyw commented Feb 3, 2023

What's the motivation to run a released Pants on the pants source repo from the command line?

With experimental_run_in_sandbox, or with future first-class Rust support, we could avoid rebuilding the engine all the time.

For me, I'm starting to use pants instead of ./pants, so this is the one odd exception to the rule for how I normally invoke pants.

Yes, but you are also actually doing a different thing, so the different command makes sense?

@benjyw
Copy link
Contributor

benjyw commented Feb 3, 2023

Anyway, I won't block this but I'm not a fan of it either. However, as long as we have a way to turn it off, I can live with it.

@kaos
Copy link
Member Author

kaos commented Feb 3, 2023

What's the motivation to run a released Pants on the pants source repo from the command line?

With experimental_run_in_sandbox, or with future first-class Rust support, we could avoid rebuilding the engine all the time.

I'm a fan of reducing the amount we need to build. But if I'm not mistaken we're only rebuilding the engine when there are changes to the rust code--and in that case I want it to be recompiled in order to be up-to-date, no?

For me, I'm starting to use pants instead of ./pants, so this is the one odd exception to the rule for how I normally invoke pants.

Yes, but you are also actually doing a different thing, so the different command makes sense?

Not really. I'm still running pants. Just in a different repo, and there really isn't any alternative modes of operation for Pants in the Pants repo. My issue is to keep track of which repo I'm currently working in--and using the correct incantation for it. That's something I'd like to not have to keep actively loaded in my mind when running Pants; it's distracting. (I've made the mistake numerous times already to run pants .. here; or been about to only to have to go back and edit the command)

Anyway, I won't block this but I'm not a fan of it either. However, as long as we have a way to turn it off, I can live with it.

Noted 📓 Thank you 🙏

I suspect it'll be "off by default" whenever there's a fix for how to specify which released version we want to run with when using a released Pants instead of running from sources, or otherwise a one line config change in pants.toml to turn this off should we need to. Until then this change does not impact you and makes me a happier maintainer 🎉 ;)

@kaos kaos merged commit f807172 into pantsbuild:main Feb 3, 2023
@kaos kaos deleted the scie-pants-for-dev branch February 3, 2023 13:40
@benjyw
Copy link
Contributor

benjyw commented Feb 3, 2023

You would build the rust code, but with a released pants, so that you can enjoy caching, and then invoke that built engine in from-sources pants.

For example, today you rebuild the engine every time you switch between main and a release branch.

@benjyw
Copy link
Contributor

benjyw commented Feb 3, 2023

My point is that running from sources and running from a release are two very different things, so you're not "just running pants" at all from my POV. You're specifically and deliberately running from sources. And making it obvious seems like a good thing to me, but as I said, not going to block this.

@kaos
Copy link
Member Author

kaos commented Feb 3, 2023

Thank you, that clarifies it quite a bit. And work is under way to make the two co-exist well together. pantsbuild/scie-pants#98

Going forward, you may tweak your environment to have pants ... do what you feel make most sense to you, and setup ./pants_from_source or ./pants_from_release as the complimentary entry point for instance.. I'm sure we can work this out favourably for all :)

@jsirois
Copy link
Contributor

jsirois commented Feb 3, 2023

You would build the rust code, but with a released pants, so that you can enjoy caching, and then invoke that built engine in from-sources pants.

Ok, yeah - but that's positively space-aged right now and will not come until ~forever. The Pants repo is not pants and we need not kvetch about breaking people. These are special people who can handle it if we say, things have changed at some point in the future. AFAICT we never can make a decision in the Pants repo that closes anything off for the Pants repo - we all hack on it after all. This feels like a great instinct for Pants OSS product consumers and a mis-placed one for Pants developers themselves.

That said, and as @kaos says - you can make pants behave the way you want today already (mod a Pants ~bug @kaos is fixing).

@illicitonion illicitonion mentioned this pull request Feb 17, 2023
Merged
illicitonion added a commit that referenced this pull request Feb 17, 2023
@illicitonion
Prep for 2.16.0.dev7 (#18297)
aee627d 
Internal changes:

* go: fix go vet test flakiness
([#18296](#18296)

* Add default gha 1000:1000 user to GHA images.
([#18281](#18281))

* Don't specify interpreter_constraints when building the release pex.
([#18280](#18280))

* go: update pass-through `go test` options for Go v1.20
([#18229](#18229))

* Fix release for /tmp on a seperate filesystem.
([#18272](#18272))

* upgrade to Rust v1.67.1
([#18269](#18269))

* Prepare the 2.15.0rc6 release.
([#18263](#18263))

* Allow lambdas for `Field` and `Target` help
([#18248](#18248))

* Revert "Avoid bind-mounts for docker environments on macOS (#18225)"
([#18247](#18247))

* Refactors `experimental_shell_command`-related files to better
separate concerns
([#18223](#18223))

* Prepare the 2.15.0rc5 release.
([#18224](#18224))

* Give dedicated threads names
([#18214](#18214))

* Prepare 2.15.0rc4.
([#18196](#18196))

* Remove the `boot_script` from
`experimental_shell_command`/`experimental_run_in_sandbox`
([#18168](#18168))

* Adjusts `BinaryShims` to use native digest operations and
`immutable_input_digests`
([#18184](#18184))

* go: update build tags check from latest go sources
([#18176](#18176))

* [internal] note dependency to `scie-pants` on "testing" env vars.
([#18170](#18170))

* go: allow coverage for standard library packages
([#18171](#18171))

* Support using `scie-pants` for Pants dev work.
([#18158](#18158))

* Prepare the 2.15.0rc3 release
([#18156](#18156))

* Splits up `shell_command.py`
([#18147](#18147))
seve-martinez pushed a commit to seve-martinez/pants that referenced this pull request Feb 20, 2023
@illicitonion @seve-martinez
Prep for 2.16.0.dev7 (pantsbuild#18297)
9980d91 
Internal changes:

* go: fix go vet test flakiness
([pantsbuild#18296](pantsbuild#18296)

* Add default gha 1000:1000 user to GHA images.
([pantsbuild#18281](pantsbuild#18281))

* Don't specify interpreter_constraints when building the release pex.
([pantsbuild#18280](pantsbuild#18280))

* go: update pass-through `go test` options for Go v1.20
([pantsbuild#18229](pantsbuild#18229))

* Fix release for /tmp on a seperate filesystem.
([pantsbuild#18272](pantsbuild#18272))

* upgrade to Rust v1.67.1
([pantsbuild#18269](pantsbuild#18269))

* Prepare the 2.15.0rc6 release.
([pantsbuild#18263](pantsbuild#18263))

* Allow lambdas for `Field` and `Target` help
([pantsbuild#18248](pantsbuild#18248))

* Revert "Avoid bind-mounts for docker environments on macOS (pantsbuild#18225)"
([pantsbuild#18247](pantsbuild#18247))

* Refactors `experimental_shell_command`-related files to better
separate concerns
([pantsbuild#18223](pantsbuild#18223))

* Prepare the 2.15.0rc5 release.
([pantsbuild#18224](pantsbuild#18224))

* Give dedicated threads names
([pantsbuild#18214](pantsbuild#18214))

* Prepare 2.15.0rc4.
([pantsbuild#18196](pantsbuild#18196))

* Remove the `boot_script` from
`experimental_shell_command`/`experimental_run_in_sandbox`
([pantsbuild#18168](pantsbuild#18168))

* Adjusts `BinaryShims` to use native digest operations and
`immutable_input_digests`
([pantsbuild#18184](pantsbuild#18184))

* go: update build tags check from latest go sources
([pantsbuild#18176](pantsbuild#18176))

* [internal] note dependency to `scie-pants` on "testing" env vars.
([pantsbuild#18170](pantsbuild#18170))

* go: allow coverage for standard library packages
([pantsbuild#18171](pantsbuild#18171))

* Support using `scie-pants` for Pants dev work.
([pantsbuild#18158](pantsbuild#18158))

* Prepare the 2.15.0rc3 release
([pantsbuild#18156](pantsbuild#18156))

* Splits up `shell_command.py`
([pantsbuild#18147](pantsbuild#18147))
@kaos kaos mentioned this pull request Jul 12, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benjyw benjyw benjyw approved these changes

Assignees
No one assigned
Labels
category:internal CI, fixes for not-yet-released features, etc.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kaos @benjyw @jsirois