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

CPython: Conan 2.0 #21387

Merged
merged 123 commits into from
Mar 21, 2024
Merged

CPython: Conan 2.0 #21387

merged 123 commits into from
Mar 21, 2024

Conversation

Ahajha
Copy link
Contributor

@Ahajha Ahajha commented Nov 26, 2023

Specify library name and version: cpython/all

I will be leaving the versions as-is in this PR, as changing them tends to break things, and I want to keep the scope small. A separate PR for that is here: #22599 (depends on this PR)

I believe there is some value in having a 2.7 recipe available, but probably not worth it for the CCI maintainers to deal with. If specifically needed, there is a mostly working 2.7 (and 3.7) version available a few commits back from the end of this branch.

Previously, the packages were not 100% relocatable on *nix. For now I always set PYTHONHOME in the test packages, not sure if this is a perfect idea but for now it seems to be a reasonably not-hacky solution.

Short paths are enabled. Lots of files in pycache directories are packaged - this is intentional, as they are compiled (bytecode/JIT) versions of all the standard library modules.

Known intermittent issues:

  1. Sometimes, when packaging on the Mac CI, there will be an error relating to a file already existing. If I had to guess the frequency, probably around 25% of the time this happens. I haven't been able to reliably reproduce it.

There are many CPython issues/PRs out right now:

Resolves #21357
Resolves #20695
Resolves #19374
Resolves #10808
Resolves #12373 (no project upgrade occurs)
Resolves #8198
Resolves #9890 (was only broken on Linux/Mac. Some other with_xxx options don't work, but that's a separate issue)

Unsure if it fixes this, I test locally on an M2 with apple-clang 15 so similar but not exact: #13900
This should be able to be closed after this, since this is the last recipe using the old libuuid recipe: #19084
This is a problem with CPython being static by default. In future versions, CPython doesn't even properly support static mode, but it's still annoying that the default configuration is invalid: #9333
Unsure, seems like a CPython problem itself: #8608

This PR is based on the following, and should no longer be needed:
Closes #18064

There was also an attempt to fix the libuuid issue:
Closes #19547


@ghost
Copy link

ghost commented Nov 26, 2023

@ghost ghost mentioned this pull request Nov 26, 2023
@conan-center-bot conan-center-bot added Failed Version conflict There is a version conflict when solving the dependencies graph labels Nov 26, 2023
@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@Ahajha
Copy link
Contributor Author

Ahajha commented Mar 18, 2024

Should be good to go now, I think I got all of the pre-3.8 logic.

@conan-center-bot

This comment has been minimized.

Copy link
Member

@uilianries uilianries left a comment

Choose a reason for hiding this comment

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

I reviewed the PR now, it's looking good, only minor details to be checked.

@property
def _version_tuple(self):
return tuple(self._version_number_only.split("."))
short_paths = True
Copy link
Member

Choose a reason for hiding this comment

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

Why short_paths is needed now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Python packages compiled versions of all of the modules shipped in the stdlib in a __pycache__ folder. These paths end up being very long, you can look at some of the hooks in the earlier runs of the PR to see such file names (./lib/python3.10/site-packages/setuptools/_distutils/__pycache__/versionpredicate.cpython-310.pyc, for example).

It would be a bad idea to remove these, as I believe they would just get regenerated when they are used, breaking cache immutability.

))
if self.settings.compiler == "intel-cc":
tc.configure_args.append("--with-icc")
if os.environ.get("CC") or self.settings.compiler != "gcc":
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if os.environ.get("CC") or self.settings.compiler != "gcc":
if self.settings.compiler != "gcc":

Avoid using external environment variables, it will result in not reproducible scenarios.

Copy link
Contributor Author

@Ahajha Ahajha Mar 19, 2024

Choose a reason for hiding this comment

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

This was here prior, I'll double check that it doesn't break anything if removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I can actually remove this condition entirely, seems that the flag was removed in 3.8: https://github.com/python/cpython/blob/main/Misc%2FNEWS.d%2F3.8.0a1.rst#L7485

Copy link
Member

Choose a reason for hiding this comment

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

Much better, please, remove then. Thank you for checking it!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. The condition above it was also able to be removed.

recipes/cpython/all/conanfile.py Show resolved Hide resolved
Comment on lines 411 to 417
# For debugging configure errors
try:
autotools.configure()
except ConanException:
with open(os.path.join(self.build_folder, "config.log"), 'r') as f:
self.output.info(f.read())
raise
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# For debugging configure errors
try:
autotools.configure()
except ConanException:
with open(os.path.join(self.build_folder, "config.log"), 'r') as f:
self.output.info(f.read())
raise
autotools.configure()

Let's keep it simple, please. Who is working on the recipe is able to access the build folder and read any file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough, this was mainly to debug the xvmc issue in CCI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment on lines 534 to 537
if (os.path.isfile(os.path.join(self.package_folder, "lib"))):
# FIXME not sure where this file comes from
self.output.info(f"{os.path.join(self.package_folder, 'lib')} exists, but it shouldn't.")
rm(self, "lib", self.package_folder)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (os.path.isfile(os.path.join(self.package_folder, "lib"))):
# FIXME not sure where this file comes from
self.output.info(f"{os.path.join(self.package_folder, 'lib')} exists, but it shouldn't.")
rm(self, "lib", self.package_folder)
# FIXME not sure where this file comes from
rm(self, "lib", self.package_folder)

Let's simplify: The rm only removes files, and does not raise error for file not found.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I might remove this hack altogether, I don't know if it ever actually worked. I'll check through some logs to see.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems intermittent, and actually hasn't happened recently (only checked some recent logs, not going through all of them). I think it's best to just remove it, if it comes up we can just rerun the job.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, removed the whole condition.

recipes/cpython/all/conanfile.py Outdated Show resolved Hide resolved

if self.options.env_vars:
bindir = os.path.join(self.package_folder, "bin")
self.output.info("Appending PATH environment variable: {}".format(bindir))
self.output.info(f"Appending PATH environment variable: {bindir}")
self.env_info.PATH.append(bindir)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
self.env_info.PATH.append(bindir)
self.env_info.PATH.append(bindir)
self.buildenv_info.prepend_path("PATH", bindir)
self.runenv_info.prepend_path("PATH", bindir)

Env info is Conan 1.x and will be deprecated in the future. In Conan 2.x you should use runenv or buildenv.For Python I would say both cases are valid because I see python scripts during build time, only to generate C/C++ code, but also I saw some packaged tools that are python scripts too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added these for all the existing variables, as well as a note regarding the old version's removal at some point.

python_root = self.package_folder
if self.options.env_vars:
self.output.info(f"Setting PYTHON_ROOT environment variable: {python_root}")
self.env_info.PYTHON_ROOT = python_root
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
self.env_info.PYTHON_ROOT = python_root
self.env_info.PYTHON_ROOT = python_root
self.buildenv_info.define_path("PYTHON_ROOT", python_root)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@Ahajha
Copy link
Contributor Author

Ahajha commented Mar 19, 2024

@uilianries Thanks for the review, I'll do some testing and add the suggestions later today or tomorrow.

@Ahajha
Copy link
Contributor Author

Ahajha commented Mar 20, 2024

@uilianries I believe I addressed all the review points.

My only (very minor) concern left is regarding the environment variables, I think we may be able to simplify one or more of:

  1. The variables themselves
  2. The conf info
  3. The conditions behind them and their related conf info being defined (should we use an env_vars option, or have them be unconditionally defined? I don't know if other packages do this) (others are conditional on Windows or Apple, maybe just have them be unconditional since I don't think the conditions are actually correct)
  4. The usage of both the env vars and the conf info (for example, we no longer need to specify the full path to the python executable, we can just run python and it will work)

It works for now, so it can stay as-is, but maybe could use some more thought.
There's an odd issue with PYTHONHOME on Windows, defining it fixes one thing but breaks another, not defining breaks the first thing but fixes the other. The previous behavior was not defining it, so I left it that way with a FIXME (also, it's easier to work around, whereas the other way it breaks a lot more). I added a note in the code if you want to do some reading.

I think it might need more thought, but now it works (for now). I can add it to my TODO issue (that I believe you saw) (rather than try to fix it now) if you agree.

For context, currently we have:
Env: PATH, PYTHON, PYTHONHOME, PYTHON_ROOT
Conf: user.cpython:python, user.cpython:pythonhome, user.cpython:module_requires_pythonhome, user.cpython:python_root

I also did a bit of research on these variables:

  • PYTHON I can't find existence of anywhere, we certainly don't use it and it isn't an official variable. I would prefer to omit it if it isn't obvious why it's needed. The conf info variable might continue to be useful, currently we use it to tell CMake where the executable is.
  • PYTHONHOME is fine to include, but has a weird issue on Windows that I mentioned above. Also not quite sure what would happen on other systems (it's currently not set in the env info for those platforms, not quite sure why). We currently use the conf variable to work around this issue.
  • PYTHON_ROOT is not an official Python environment variable, I think it's specifically needed by Boost.Python (and even then, I can't find any official documentation, only random users, and nothing of substance in the last decade), my guess is the original PR included it for that specific case. Going forward, I would probably prefer to omit it entirely.

@conan-center-bot

This comment has been minimized.

@conan-center-bot
Copy link
Collaborator

Conan v1 pipeline ✔️

All green in build 12 (c5fa0c5e567a1dd8e032f82376b95c057a40d1bb):

  • cpython/3.10.0:
    All packages built successfully! (All logs)

  • cpython/3.9.7:
    All packages built successfully! (All logs)

  • cpython/3.8.12:
    All packages built successfully! (All logs)


Conan v2 pipeline ✔️

Note: Conan v2 builds are now mandatory. Please read our discussion about it.

All green in build 12 (c5fa0c5e567a1dd8e032f82376b95c057a40d1bb):

  • cpython/3.10.0:
    All packages built successfully! (All logs)

  • cpython/3.9.7:
    All packages built successfully! (All logs)

  • cpython/3.8.12:
    All packages built successfully! (All logs)

@uilianries
Copy link
Member

@Ahajha Thank you so much for your detailed information! It helps a lot to understand the current state of this PR. At this point I'm comfortable to accept it, I'll only talk to @RubenRBS first, to have a final review, but if is all okay, it should be merged by today.

Thank you again for this huge effort, this is one of most tricky recipes in CCI.

@conan-center-bot conan-center-bot merged commit e78f2d5 into conan-io:master Mar 21, 2024
23 checks passed
@jwillikers
Copy link
Contributor

image

@Ahajha
Copy link
Contributor Author

Ahajha commented Mar 21, 2024

Thanks to everyone involved for helping this along! I learned a lot from working on this PR for sure :)

@ilatypov
Copy link

ilatypov commented Jun 14, 2024

python -m pip fails missing pyexpat, socket on Windows. Sigh. The original submission took pains to use conan's dependencies for Python 2. I don't know if that worked (I cannot even download cpython 2.7.18 because it is not compatible with the new Conan API). Am I doing the right thing? If I do, could that be added as a smoke test (currently failing)?

I guess the use of cpython from conan may have a limited user base because using conan implies using Python. But it's needed for bootstrapping in the corporate requirements of using approved binary repositories (such as Conan Center). And of course the conan way of building C Python could eventually substitute the less-straightforward build systems, enabling C Python development bootstrapping.

CMD> bin\python -m pip
Traceback (most recent call last):
[...]
  File "C:\Users\XXX\cpython\bin\lib\site-packages\pip\_vendor\pkg_resources\__init__.py", line 35, in <module>
    import plistlib
  File "C:\Users\XXX\cpython\bin\Lib\plistlib.py", line 65, in <module>
    from xml.parsers.expat import ParserCreate
  File "C:\Users\XXX\cpython\bin\Lib\xml\parsers\expat.py", line 4, in <module>
    from pyexpat import *
ModuleNotFoundError: No module named 'pyexpat'

For visual studio, the patches are needed to use conan packages instead of python binaries.
See this repo and this repo.

For python 2.7, I had to apply some fixes to make it build with more recent visual studios.
These patches will never hit upstream, as 2.7 is dead.

Originally posted by @madebr in #1510 (comment)

@Ahajha
Copy link
Contributor Author

Ahajha commented Jun 14, 2024

@ilatypov There was discussion about leaving Python 2 support, but we ended up dropping it. That said, I'm more than willing to re-add it if you think there's a use case for it in Conan Center.

Regarding the failing test, do you know if this ever worked?

Also regarding "The original submission took pains to use conan's dependencies for Python 2" - Is this just saying that Python 2 was originally supported, or that it used Conan dependencies? If the latter, I don't think anything has changed - the recipe still goes to great lengths to use Conan's dependencies.

I've seen slow but steady adoption of the cpython package within ConanCenter, mainly in places where Conan's minimum Python version isn't enough. When I started this PR (branching off of a few others), I had a use case of easily switching Python versions within a multi-repo application as an alternative to essentially requiring some containerization.

All that said, I'm happy to look into the failing test and re-add Python 2.

@ilatypov
Copy link

ilatypov commented Jun 14, 2024

I hijacked this thread that showed amazing collaboration. I did not have concerns about resurrecting conan's Python 2 and, unfortunately, I don't know if the smoke test would work with conan's Python 2. It was my attempt to rely on the previous work that seemed successful in packaging conan's Python with its dependencies. (The _supports_modules attribute seems to be related to a capability of building additional modules but not to conan's own packaging of cpython?) I am concerned that Python 3 was failing where it could be used for setting up access to any other package through the trust to the binaries published in Conan Center.

Perhaps one day I could learn from this group and submit a useful pull request.

@Ahajha
Copy link
Contributor Author

Ahajha commented Jun 14, 2024

I'm a little confused as to your exact problem, but I have a guess as to the solution:

The _supports_modules attribute is representative of a Python limitation, not a Conan one. If statically linking Python on Windows, no extension modules can be built, so perhaps that's why you're seeing this issue? You could set cpython/*:shared=True if you haven't already, perhaps that's what you're looking for?

@ilatypov
Copy link

ilatypov commented Jun 14, 2024

I am not linking, just using Python from Conan Center. Its smoke test above (-m pip) is failing.

@Ahajha
Copy link
Contributor Author

Ahajha commented Jun 14, 2024

Let me clarify - Regardless of what you're doing with the package, on Windows, if cpython/*:shared=False, which is the default, then the extension modules (such as expat) will not be available. So setting cpython/*:shared=True I think will fix your issue.

I think this is a poor default regardless - I have a PR out to change it.

@ilatypov
Copy link

ilatypov commented Jun 14, 2024

It worked for 3.8.12 here, thank you. I am new to conan, so I forgot that Conan Center's binary artifacts are not the ultimate goal of this GitHub project, but only its optimization. The Conan Center's recipes are the goals (important deliverables) of this GitHub project. My building C Python for Windows from source using the available recipe took only minutes and worked great.

PS> conan install --requires=cpython/3.8.12 -o shared=True -u --build=missing -vvv --deployer=full_deploy
[...]
PS> .\full_deploy\host\cpython\3.8.12\Release\x86_64\bin\python.exe -m pip
Usage:
  C:\Users\XXX\full_deploy\host\cpython\3.8.12\Release\x86_64\bin\python.exe -m pip <command> [options]

Commands:
[...]

P.S. I wish Conan Center hosted simpler proto-conan binaries sufficient to build C Python, bootstrapping a full conan. Or it could have hosted binary proto-cpython packages downloading and untarring which would allow running python -m pip and installing conan.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment