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 _openssl-syslib-check package #11452
Conversation
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 ( I do have some suggestions for making it better though... For recipes/_openssl-syslib-check:
|
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 ( |
Is there a way to run the tests as root? Since it was possible to delete dlls from
For the moment I wrapped that part in a try-except clause, but the activation script should IMO really be tested in all variants. |
I don't think this is a good idea as this adds cycles in the dependency graph. |
No, this package doesn't depend on anything. And |
Edit: nevermind, switched the order, so this is not relevant anymore I wanted to use a trick from the blas-feedstock to install
|
@conda-forge/staged-recipes |
Hi! This is the friendly automated conda-forge-linting service. I wanted to let you know that I linted all conda-recipes in your PR ( Here's what I've got... For recipes/_openssl-syslib-check:
|
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 ( |
If git is installed in the environment, 'openssl' can *still* point to a wrong executable. This did happen in the CI run of github.com/conda-forge/pull/11452/commits/17eee6d
@conda-forge/staged-recipes
the test environment actually installs python 3.8 in both cases. This recipe must not depend on python, but I'd like to be able to test the correct python version nevertheless (because different python versions have different builds regarding |
@mingwandroid |
PS. As noted here, there's an argument to be made that the activation scripts could directly live in the openssl-feedstock, and probe for |
@mingwandroid Strictly speaking a test suite for the activation scripts is not necessary, but I thought it's a prudent thing considering the importance of the packages (if we say goodbye to the tests that need elevated privileges, I could implement this in the openssl-feedstock right away). |
I like it, yes, many thanks. My only concern is a slight overhead added at activate time, but I guess we'll see if that's a big problem. It's certainly better than the alternative! It might be prudent to have a very-early-out env. var check to mitigate this? CONDA_SKIP_OPENSSL_DLL_CHECK or something? |
@mingwandroid I have already added an early out:
I also thought about an early-out env-var, but then I guess that 2 file- Do you have some advice if/how |
PS. Besides speed, the |
@isuruf |
This is more or less ready to be added into the openssl-PR, but I still have the open question if there is a way to execute Correction: might still need a bit of work for piping the output of |
49aa297
to
1292b3e
Compare
Ping @mingwandroid @isuruf |
Hi friend! We really, really, really appreciate that you have taken the time to make a PR on In an effort to maintain this repository and increase the signal-to-noise for open PRs, the maintainers of If you'd like to keep it open, please comment/push and we will be happy to oblige! Note that very old PRs will likely need to be rebased on master so that they can be rebuilt with the most recent CI scripts. If you have any trouble, or we missed reviewing this PR in the first place (sorry!), feel free to ping the team using a special command in a comment on the PR to get the attention of the Cheers and thank you for contributing to this community effort! |
Let's keep this open a bit more. Still planning to come back to this in some form |
Hi friend! We really, really, really appreciate that you have taken the time to make a PR on In an effort to maintain this repository and increase the signal-to-noise for open PRs, the maintainers of If you'd like to keep it open, please comment/push and we will be happy to oblige! Note that very old PRs will likely need to be rebased on master so that they can be rebuilt with the most recent CI scripts. If you have any trouble, or we missed reviewing this PR in the first place (sorry!), feel free to ping the team using a special command in a comment on the PR to get the attention of the Cheers and thank you for contributing to this community effort! |
Not (completely) stale |
What's the state here? Do you plan to work more on this or should I do a review to get this forward? |
Hey @xhochy, thanks for taking an interest here! There were a few open questions for which I never got an answer - mainly revolving around testing. E.g. to test the correct behaviour in all (or even most) variants, we'd have to delete/replace system libraries in CI, which requires running the test with elevated privileges. Also, I had considered writing some tests for the activation-log output, but never got around to that due to not receiving other feedback - just reread the thread and most things are still current/open. For context, this PR started here, and then Ray had suggested:
|
Hi friend! We really, really, really appreciate that you have taken the time to make a PR on In an effort to maintain this repository and increase the signal-to-noise for open PRs, the maintainers of If you'd like to keep it open, please comment/push and we will be happy to oblige! Note that very old PRs will likely need to be rebased on Cheers and thank you for contributing to this community effort! |
Hi again! About a month ago, we commented on this PR saying it would be closed in another month if it was still inactive. It has been a month and so now it is being closed. Thank you so much for making it in the first place and contributing to the community project that is Cheers and have a great day! |
Checklist
url
) rather than a repo (e.g.git_url
) is used in your recipe (see here for more details)Out of the discussion in conda-forge/cryptography-feedstock#37 and conda-forge/openssl-feedstock#56, @mingwandroid suggested to introduce a package that both the openssl- & cryptography-feedstock can depend on, so we don't have a mess of activation scripts needing to cover all possible deployment scenarios.
This is a first draft, but comes with a test suite at least. I added some old dlls for that purpose, not sure what the opinion is on that.
I added both @isuruf and @mingwandroid as maintainers, since they were by far the most active/knowledgeable in this discussion. If this assumption is wrong, I'm happy to revert.
Still missing:
activate.sh
and enable testing for it.run_test.py
with enough permissions to change stuff inC:/Windows/System32
(see this comment)