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

Silence stdout/stderr during Pants setup. #375

Merged
merged 5 commits into from
Mar 18, 2024
Merged

Silence stdout/stderr during Pants setup. #375

merged 5 commits into from
Mar 18, 2024

Conversation

kaos
Copy link
Member

@kaos kaos commented Mar 13, 2024

Closes pantsbuild/pants#20315
Broken out from #351

@kaos kaos requested a review from huonw March 13, 2024 12:54
@kaos kaos mentioned this pull request Mar 13, 2024
Copy link
Contributor

@huonw huonw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for splitting this up. In addition to the details of the file:

  • how does a user know how to find this file, if they need to? Are there docs we could augment with a sentence? (There's the additional issue of the venv path being non-obvious with hashes in it, but that may be impossible to solve?)
  • is this something we can add a test for do it doesn't regress?

tools/src/scie_pants/install_pants.py Outdated Show resolved Hide resolved
@kaos
Copy link
Member Author

kaos commented Mar 14, 2024

is this something we can add a test for so it doesn't regress?

I'm sure we could.. do you think it's worth looking into?

I assume it's the part about not leaking output to stdout during bootstrapping.. checking the presence of the pants-install.log file could also be done, but perhaps trickier as we'd need to find out the path.

In that case, it feels like we might prefer looking into adding support for something to query the lift runtime for the various resulting paths etc. perhaps something like SCIE=lift.expand scie-pants scie.bindings.

@kaos kaos requested a review from huonw March 14, 2024 14:07
@huonw
Copy link
Contributor

huonw commented Mar 14, 2024

I'm sure we could.. do you think it's worth looking into?

I assume it's the part about not leaking output to stdout during bootstrapping..

Yeah, just keeping stdout clean. My preference would be yes, but if it's a tonne of effort, maybe not worth it? 🤷

@kaos
Copy link
Member Author

kaos commented Mar 15, 2024

I'm sure we could.. do you think it's worth looking into?
I assume it's the part about not leaking output to stdout during bootstrapping..

Yeah, just keeping stdout clean. My preference would be yes, but if it's a tonne of effort, maybe not worth it? 🤷

I had a long reply that started out in favour of not testing this, but which in the end concluded that a test is worthwhile, as we want to ensure stdout is clean, regardless of what is being done during bootstrap, so it would protect not only against this fix in particular, but against all future changes that introduces new stuff during bootstrapping as well. I'll look into it :)

@@ -238,7 +248,7 @@ fn test_pants_bin_name_handling(scie_pants_scie: &Path) {
false,
r#"
[GLOBAL]
pants_version = "2.15.0rc5"
pants_version = "2.18.0"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By using the same pants version consistently, we avoid having to download and bootstrap more versions than necessary.

@kaos
Copy link
Member Author

kaos commented Mar 17, 2024

@huonw OK, I've added a test verifying bootstrap doesn't pollute stdout. PTAL :)

Copy link
Contributor

@huonw huonw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@kaos kaos merged commit d336b8f into main Mar 18, 2024
8 checks passed
@kaos kaos deleted the kaos/pants-20315 branch March 18, 2024 07:56
huonw added a commit to pantsbuild/pantsbuild.org that referenced this pull request Apr 27, 2024
kaos added a commit that referenced this pull request May 22, 2024
~Look at the Python tag in the Pants PEX filename~ Cycle through all
supported Python versions and check the PEX download URL using HEAD for
each one to infer the Python version to use when installing Pants, when
using a version of Pants that we don't already know which version of
Python to use.

~Drop support for `PANTS_SHA`.~ Done in #376.

~Silence the install step, capturing the output to a `pants-install.log`
file in the science cache.~ Done in #375.

---------

Co-authored-by: Huon Wilson <wilson.huon@gmail.com>
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.

Pants leaking install logs to stdout while installing a new version
2 participants