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

Fix 82 2 #84

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Fix 82 2 #84

wants to merge 4 commits into from

Conversation

domste
Copy link

@domste domste commented Aug 10, 2020

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

Set the certificate path (#82 ) via post-link.sh script and removed the certificate logic in the build script, because it does not have any effect in the installation process (#83 ).

domste added 3 commits August 10, 2020 16:18
do not set certificates in build.sh because it has no effect on the installation
@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

I do have some suggestions for making it better though...

For recipe:

  • License is not an SPDX identifier (or a custom LicenseRef) nor an SPDX license expression.

Documentation on acceptable licenses can be found here.


git config --system http.sslVerify true
git config --system http.sslCAPath "${cert_file}"
git config --system http.sslCAInfo "${cert_file}"
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem like the correct thing to do on the user's machine. Is there some global / system setting we can configure?

Copy link
Member

Choose a reason for hiding this comment

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

Seems like you should rather patch git so that this becomes the http.sslCAInfo default if it isn't configured. I agree this is a bad thing to do. The git configuration is user-global, but git could be installed into multiple environments. The result is that the ~/.gitconfig would point to the cert files from whatever conda environment had git installed in it most recently. If that environment then gets deleted, or even if it isn't updated, the git from every other environment would still point to that location.

Copy link
Author

Choose a reason for hiding this comment

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

I tried it with using different environments and the gitconfig seems to be env specific.

You can check this by running following command in an environment with git installed

(git-test) root@ubuntu:/usr/local/conda-bld/linux-64# git config --list --show-origin
file:/usr/local/envs/git-test/etc/gitconfig     http.sslverify=true
file:/usr/local/envs/git-test/etc/gitconfig     http.sslcapath=/usr/local/envs/git-test/ssl/cacert.pem
file:/usr/local/envs/git-test/etc/gitconfig     http.sslcainfo=/usr/local/envs/git-test/ssl/cacert.pem

When I set the $REQUESTS_CA_BUNDLE var and install it in another environment an env-specific gitconfig is created, see below:

(git-test-custom-ca) root@ubuntu:/usr/local/conda-bld/linux-64# export REQUESTS_CA_BUNDLE=/root/my-custom-ca.pem  
(git-test-custom-ca) root@ubuntu:/usr/local/conda-bld/linux-64# conda install git --use-local


(git-test-custom-ca) root@ubuntu:/usr/local/conda-bld/linux-64# git config --list --show-origin
file:/usr/local/envs/git-test-custom-ca/etc/gitconfig   http.sslverify=true
file:/usr/local/envs/git-test-custom-ca/etc/gitconfig   http.sslcapath=/root/my-custom-ca.pem
file:/usr/local/envs/git-test-custom-ca/etc/gitconfig   http.sslcainfo=/root/my-custom-ca.pem

So the global user space should not be affected by this changes. Could you please verify to be on the save side?

One downside is that the variable $REQUESTS_CA_BUNDLE has to be defined before installation for changes to take effect.

Copy link
Member

Choose a reason for hiding this comment

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

It really doesn't seem right to do this in post-link.sh

@domste domste requested a review from scopatz September 14, 2020 11:48
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.

4 participants