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

Remove conda-forge branding #332

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

SylvainCorlay
Copy link
Member

Alternative to #331.
I think that this fix is actually preferable than the current branding solution.

@conda-forge-linter
Copy link

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 (recipe) and found some lint.

Here's what I've got...

For recipe:

  • Failed to even lint the recipe, probably because of a conda-smithy bug 😢. This likely indicates a problem in your meta.yaml, though. To get a traceback to help figure out what's going on, install conda-smithy and run conda smithy recipe-lint . from the recipe directory.

@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.

Copy link
Member

@isuruf isuruf left a comment

Choose a reason for hiding this comment

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

-1 on this. I know that there are some downstream software which use sys.version to detect a conda installed python.

@SylvainCorlay
Copy link
Member Author

-1 on this. I know that there are some downstream software which use sys.version to detect a conda installed python.

Patching sys.version to add brand the banner is hacky at best (and the fact that platform.py much be patched as well shows that it has side effects at least). Issue #330 shows that has negative consequences.

I really prefer this approach over making the regex more tolerant.

@SylvainCorlay SylvainCorlay mentioned this pull request Mar 22, 2020
5 tasks
@mingwandroid
Copy link
Contributor

Playing devil's advocate here but given

our PyPI manylinux wheel is statically linked with libpython.a. This causes the banner to not be the one from conda-forge an to cause a mismatch with conda-forge's patched validation regex.

Isn't this the thing we should fix here first. Then branding or no branding is untangled from your issue.

@jakirkham
Copy link
Member

Agree with Ray 🙂

@SylvainCorlay
Copy link
Member Author

SylvainCorlay commented Mar 22, 2020

Playing devil's advocate here but given

our PyPI manylinux wheel is statically linked with libpython.a. This causes the banner to not be the one from conda-forge an to cause a mismatch with conda-forge's patched validation regex.

Isn't this the thing we should fix here first. Then branding or no branding is untangled from your issue.

@mingwandroid @jakirkham Quite the contrary. The conda package for xeus-python is dynamically linked with libpython.so (and we prefer it so), but in the case of a wheel, some distributions of Python don't come with a shared object - and we must link statically. This is how several other packages operate to distribute PyPI wheels.

My point is that conda-forge's Python brands the Python interpreter's banner by changing the sys.version string, which has several side effects as the string is validated at runtime. I don't think this is a good thing to do at all.

@mingwandroid
Copy link
Contributor

I must not have made myself clear here.

That branding works with the dylib but not the static archive seems to me a serious bug and entirely orthogonal to whether we should brand or not.

Now if everyone was in agreement to ditch branding then OK, let's do that, delete all the changes and move on.

Bur that's not the situation and if we did do that we'd need a deprecation cycle for it.

I'm happy to look into using the anaconda branding code for conda forge, which I believe works in both cases. In my opinion this is the best path forward.

@SylvainCorlay
Copy link
Member Author

SylvainCorlay commented Mar 23, 2020

That branding works with the dylib but not the static archive seems to me a serious bug and entirely orthogonal to whether we should brand or not.

Actually, there is no "bug", since the manylinux wheel is linked against the libpython.a from manylinux and not conda, which is why it does not get the branding.

we'd need a deprecation cycle for it.

I think we can start with #331 which makes the regex compatible with both the branding and the absence thereof, and discuss the complete removal at greater length.

@mingwandroid
Copy link
Contributor

What manylinux libpython.a do you mean? AFAIK this doesn't exist. You are linking against some random libpython the distribution you ran your build upon happened to have installed I guess?

I don't want to spend time taking about random software.

@mingwandroid
Copy link
Contributor

And is it true that you are using this random python but with a python syslib from conda-forge?

@SylvainCorlay
Copy link
Member Author

You are linking against some random libpython the distribution you ran your build upon happened to have installed I guess?

No, we are linking against the right libpython.a, but preventing the manylinux docker file from deleting it. This is something several other packages are doing to embed python, such as Panda3D.

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