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

Plumb ca_certs_path to the plugin resolver. #10910

Merged
merged 1 commit into from
Oct 5, 2020

Conversation

jsirois
Copy link
Contributor

@jsirois jsirois commented Oct 5, 2020

This should be the last place ca_certs_path needs plumbing.

[ci skip-rust]
[ci skip-build-wheels]

This should be the last place `ca_certs_path` needs plumbing.

[ci skip-rust]
[ci skip-build-wheels]
Copy link
Contributor

@Eric-Arellano Eric-Arellano 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 following up on this.

@jsirois
Copy link
Contributor Author

jsirois commented Oct 5, 2020

Tested ad-hoc with:

$ git diff
diff --git a/pants.toml b/pants.toml
index f014b0c47..65225ff22 100644
--- a/pants.toml
+++ b/pants.toml
@@ -1,4 +1,10 @@
 [GLOBAL]
+# ca_certs_path = "/etc/hosts"
+ca_certs_path = "/etc/ca-certificates/extracted/ca-bundle.trust.crt"
+plugins = [
+  "setuptools>=41.0",
+]
+
 print_exception_stacktrace = true
 
 pants_distdir_legacy_paths = false

Works as shown, fails with /etc/hosts toggled in for ca_certs_path.

@jsirois
Copy link
Contributor Author

jsirois commented Oct 5, 2020

Reviewers - its probably good to vet the second commit (5355712) too. I reverted @thamenato's change in #10909 since that just bandaided over this problem.

@thamenato
Copy link
Member

I was wondering about adding that env var in the sense of what if I need any other env var to be passed to the subprocesses? I understand why lock it but shouldn't that be a bit more dynamic?

@coveralls
Copy link

coveralls commented Oct 5, 2020

Coverage Status

Coverage remained the same at 0.0% when pulling 5355712 on jsirois:ca_cert/fix_plugins into ec9f266 on pantsbuild:master.

@jsirois
Copy link
Contributor Author

jsirois commented Oct 5, 2020

Ok - @thamenato provided more detailed Pants log output offline. That shows his issue is not the guessed plugins issue, but a resolve of black + setuptools for the black rule. I'm going to revert the second commit here. The 1st commit fixes a looming problem though with plugins so stands on its own.

@jsirois jsirois merged commit 7fc06c3 into pantsbuild:master Oct 5, 2020
@jsirois jsirois deleted the ca_cert/fix_plugins branch October 5, 2020 19:56
@benjyw
Copy link
Contributor

benjyw commented Oct 5, 2020

So still a mystery as to why --certs isn't sufficient...

@jsirois
Copy link
Contributor Author

jsirois commented Oct 5, 2020

So still a mystery as to why --certs isn't sufficient...

Correct.

@jsirois
Copy link
Contributor Author

jsirois commented Oct 5, 2020

Narrowing down the mystery over here: pex-tool/pex#1058
Its only getting more mysterious though.

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.

5 participants