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

[3.7] Build against libxcrypt on Linux #676

Open
wants to merge 11 commits into
base: 3.7
Choose a base branch
from

Conversation

AaronOpfer
Copy link

Creating this PR as was suggested in discussion in conda-forge/linux-sysroot-feedstock#52, modeled after other PRs along the same vein from @mbargull .

refs:

#664
conda-forge/linux-sysroot-feedstock#52

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • 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.

@conda-forge-webservices
Copy link
Contributor

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.

@jakirkham
Copy link
Member

Thanks Aaron! 🙏

Would suggest adding the text below to conda-forge.yml's azure section and re-render

azure:
  settings_win:
    pool:
      vmImage: vs2017-win2016

Based this off the similar changeset for 3.8 branch in commit
716d0ef
@AaronOpfer
Copy link
Author

Gave that a try, but seems like no luck. @jakirkham

Copy link
Member

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

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

Thanks Aaron! 🙏

Provided another potential option below. Let's see if that works better

.azure-pipelines/azure-pipelines-win.yml Outdated Show resolved Hide resolved
conda-forge.yml Outdated Show resolved Hide resolved
@@ -139,6 +139,7 @@ requirements:
- ncurses # [not win]
- libffi # [not win]
- libnsl # [linux]
- libxcrypt # [linux]
Copy link
Member

Choose a reason for hiding this comment

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

@beckermr , just to confirm, is this what we want here? Is there anything else we need?

Copy link
Member

Choose a reason for hiding this comment

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

Assuming it has a proper run export, this should be everything.

Copy link
Member

Choose a reason for hiding this comment

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

@mbargull already converted the current python versions so you can compare to that recipe to be sure.

Copy link
Member

Choose a reason for hiding this comment

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

That looks like what Marcel did in PR: #664

Though would be happy to hear feedback from him

Looks like we already cover the import crypt test case

Copy link
Member

Choose a reason for hiding this comment

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

I'm not against rebuilding against libxcrypt.
However, rebuilding this against an updated global pinning is probably not what would @AaronOpfer and others help.
We do not rebuild packages that depend on Python 3.6/3.7 anymore, which means that if we only rebuild conda-forge::python=3.6,3.7, we'll likely add updated dependency constraints which would be incompatible with Python extension modules on conda-forge that have been built against the same packages but only in older versions.
(Looking at the changes to .ci_support/*.yaml here, it doesn't look like much has changed, apart from the dropped openssl=1.1.1 builds; but there could still be things like tightened run_exports in some of the dependencies -- i.e., someone should look at the actual constraints generated.)

Meaning, we might not want a full rerendering here to try to build against older libs (and have someone verify this by comparing with previous python=3.6,3.7 builds).

So, to me libcrypt/libxcrypt is actually not the most important thing we should look out for here, but rather the other dependencies.

(FYI, Aaron's issue does probably not concern the actual linked libcrypt at all, but rather the fact that Python.h includes crypt.h for Python <=3.8; in >=3.9 it is only used for the crypt module and in >=3.13 it will not be used at all.)

Copy link
Member

Choose a reason for hiding this comment

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

That's a good point Marcel!

We can revert the bulk of the variant changes (though some things like the Visual Studio version are out of our hands)

Also if you haven't already, please read over this discussion for more context: conda-forge/linux-sysroot-feedstock#52 (comment)

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the feedback. I was also concerned about creating a variant that does not fit the same role as the last built py37. FWIW, I have consulted with my users and discovered they are still using the openssl 1.1.1 variant of Python 3.7.12, so discarding that variant would be suboptimal.

I'm very much new to the conda feedstock system still. Do we believe we would fix this by modifying the recipe/conda_build_config.yaml and rerendering as the .ci_support/README suggests? Or would we hand-edit the files as we're doing artifact archeology on an old recipe?

Copy link
Author

Choose a reason for hiding this comment

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

One more note: if CF maintainers would like to take over and directly edit this PR instead of puppeteering this conda-feedstock novice, please feel free to do so 🙂

Copy link
Member

Choose a reason for hiding this comment

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

I have consulted with my users and discovered they are still using the openssl 1.1.1 variant of Python 3.7.12

Um I hope not. OpenSSL 1.1.1 has already reached EOL

Do we believe we would fix this by modifying the recipe/conda_build_config.yaml and rerendering as the .ci_support/README suggests? Or would we hand-edit the files as we're doing artifact archeology on an old recipe?

Was thinking we could just

git checkout 3.7 -- .ci_support
git add .ci_support
git commit -m 'Restore `.ci_support` from `3.7`'

Copy link
Member

Choose a reason for hiding this comment

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

Um I hope not. OpenSSL 1.1.1 has already reached EOL

Python 3.7 never officially gained support for OpenSSL 3. We only did that in conda-forge.

@AaronOpfer
Copy link
Author

OK; all builds are green. The remaining question is: how do we inspect the resulting packages runtime data to make sure they're matching or reasonably close to matching the original Python 3.7.12 builds?

Comment on lines -26 to -44
bzip2:
max_pin: x
libffi:
max_pin: x.x
ncurses:
max_pin: x.x
python:
min_pin: x.x
max_pin: x.x
readline:
max_pin: x
sqlite:
max_pin: x
tk:
max_pin: x.x
xz:
max_pin: x.x
zlib:
max_pin: x.x
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't we also need to match the pins for previous pin_run_as_build packages to "period-appropriate" versions in the local CBC?

@AaronOpfer
Copy link
Author

FYI, my company no longer needs this urgently. We ended up hacking up references to the old sysroot_linux-64 for py36 and py37 targets since I tried to get the ball rolling a few weeks ago.

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.

6 participants