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

Add pants_from_sources support. #77

Merged
merged 10 commits into from
Jan 27, 2023
Merged

Add pants_from_sources support. #77

merged 10 commits into from
Jan 27, 2023

Conversation

jsirois
Copy link
Contributor

@jsirois jsirois commented Jan 27, 2023

Closes #30

@jsirois
Copy link
Contributor Author

jsirois commented Jan 27, 2023

1 extra commit coming to add CI caching of ~/.cache/scie-pants/dev since its so slow to build the Pants used for the pants_from_sources tests - but safe to review.

src/main.rs Outdated Show resolved Hide resolved
CHANGES.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@kaos
Copy link
Member

kaos commented Jan 27, 2023

Ah, I see.. is this also closer in line with how you'd prefer to solve #33 ?

Or, doesn't this also cover the use case for #33 ?
Edit: uhm.. maybe not, if it's to magically switch behaviour only in the face of the magic config. Only, now for #33 we could re-use some of the logic in here, I believe..

@jsirois
Copy link
Contributor Author

jsirois commented Jan 27, 2023

Ah, I see.. is this also closer in line with how you'd prefer to solve #33 ?

This really bears no relation to #33 in that it is completely driven by preexisting requirements. The pants_from_sources scripts totally constrained what I could do here. There were 0 choices for me to make afaict.

#33 can be solved by using any unambiguous marker. If and only if it's decided the marker lives in pants.toml / is an env var, then the soft constraint of not grabbing more subsystem name land is to re-use bootstrap which was grabbed by PANTS_BOOTSTRAP_TOOLS before scie-pants was even an idea.

@kaos
Copy link
Member

kaos commented Jan 27, 2023

Sorry I was being very vague in my phrasing of that question. Having just reviewed the implementation I was thinking of the implementation--regardless of how we detect that we want to delegate to a ./pants bootstrap script--in main.rs I had made another lift-entry for pants-dev, where as here you made another get process function altogether. I see those as two different ways of solving this, and both could work for #33 as well, I think. So, seeing this method here now, in contrast to what I cooked up in #75 I was wondering if this approach may be preferable to having that extra lift-entry?

But, we can defer this topic to the review of #75. :)

@jsirois
Copy link
Contributor Author

jsirois commented Jan 27, 2023

But, we can defer this topic to the review of #75. :)

I answered there, but the key difference in "commands" delegation is the need for "bindings" (~install setup steps). You do not have that need.

This repo does not house a ./pants script so it needs one for build_root
detection by the ./pants script in a Pants from sources run.
@jsirois jsirois merged commit 4ad5c97 into pantsbuild:main Jan 27, 2023
@jsirois jsirois deleted the issues/30 branch January 27, 2023 08:22
@benjyw
Copy link
Contributor

benjyw commented Jan 29, 2023

Sorry I'm late to the party, but it occurs to me that PANTS_SOURCE lives in the PANTS_ namespace of Pants options. E.g., if we add a global option named --source, this would stomp it.

In fact, we already have a "source" subsystem, for source root configuration, so PANTS_SOURCE_* is occupied. This doesn't collide with PANTS_SOURCE, but I wonder if it's close enough to be confusing?

Should this be _PANTS_SOURCE? Or maybe this isn't a real problem in practice.

PS I like the env var a lot more than pants_from_sources and will be using it exclusively from now on.

@jsirois
Copy link
Contributor Author

jsirois commented Jan 29, 2023

Yeah, the current choice matches status quo, but I do agree status quo is confusing. I'm on a mission to do a narrow thing: replace ./pants and not break people. If you or others want to follow up and do other things, that's great.

@benjyw
Copy link
Contributor

benjyw commented Jan 29, 2023

Uurgh, didn't realize we were using PANTS_SOURCE inside pants_from_sources today.

I'll think about whether this is worth changing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fold pants_from_sources into scie-pants
3 participants