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

Improve detection of the libxml2 and libxslt libraries #297

Merged
merged 1 commit into from
Feb 26, 2020

Conversation

hughmcmaster
Copy link
Contributor

This patch improves detection of the libxml2 and libxslt libraries by cleaning up some of the overly-complex build system.

The patch also improves support for using pkg-config if available.

setupinfo.py Outdated
if xml2_version and xslt_version:
return xml2_version, xslt_version

# One or both build dependencies not found.
Copy link
Member

Choose a reason for hiding this comment

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

This used to be the case for the Windows build with pre-built downloaded libraries. Failing hard here breaks this.
Either it should continue here, or we also need to integrate that part of the build here. I think not failing seems like the easiest way to keep it working for now.

Copy link
Contributor Author

@hughmcmaster hughmcmaster Feb 25, 2020

Choose a reason for hiding this comment

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

I think failing is useful, otherwise the build just fails later on, since the headers are missing.

I'd like to keep the hard failure for Linux platforms, but allow Windows builds to keep going. Would you be happy with a conditional guard? For example:

if not sys.platform.startswith('win'):
    print("Error: Please make sure the libxml2 and libxslt development packages are installed.")
    sys.exit(1)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I've pushed a slightly different solution to this.

@hughmcmaster
Copy link
Contributor Author

In general, given the complexity of the build system, I think it makes sense to more closely integrate the library version check and the 'find' library logic. So, you could potentially have a function, check_build_dependencies(), call get_library_versions() then check_min_version().

@scoder
Copy link
Member

scoder commented Feb 26, 2020

Thanks!

@scoder scoder merged commit 37088de into lxml:master Feb 26, 2020
@leon-anavi
Copy link

Hi @scoder, @hughmcmaster,

I am looking at the Yocto/OpenEmbedded recipe for python-lxml in Yocto/OE layer meta-python which for version 4.5.0 uses arguments --with-xslt-config and --with-xml2-config as per the current instructions for building from source. When I tried to upgrade it to version 4.5.1 I got the following error:

error: option --with-xslt-config not recognized

By looking at option_value and the changes from this GitHub pull request it seems that --with-xslt-config and --with-xml2-config have been replaced with --xslt-config and --xml2-config. Is my understanding correct? If yes, are there updated build instructions following this change?

Thanks,
Leon

@hughmcmaster
Copy link
Contributor Author

@leon-anavi, the build flags have been replaced by --xslt-config and --xml2-config. Although, that's more because I probably failed to consider that the macro is literal and does not prefix with (unlike autotools).

@scoder: do you want to use --with-xml2-config again (it's a very minor change).

@leon-anavi
Copy link

Thanks for the quick feedback. One more thing: I've already tried flags without with. This part works but there is another issue, probably related to the changes in setupinfo.py from the other GitHub pull request #300:

| Building lxml version 4.5.1.
| Building without Cython.
| Minimum required version of libxml2 is 2.7.0. Your system has version 0.29.2.
| Minimum required version of libxslt is 1.1.23. Your system has version 0.29.2.
| Building against libxml2 0.29.2 and libxslt 0.29.2
| Traceback (most recent call last):
|   File "/home/leon/poky-master/build/tmp/work/core2-64-poky-linux/python3-lxml/4.5.1-r0/lxml-4.5.1/setup.py", line 245, in <module>
|     **setup_extra_options()
|   File "/home/leon/poky-master/build/tmp/work/core2-64-poky-linux/python3-lxml/4.5.1-r0/lxml-4.5.1/setup.py", line 151, in setup_extra_options
|     ext_modules = setupinfo.ext_modules(
|   File "/home/leon/poky-master/build/tmp/work/core2-64-poky-linux/python3-lxml/4.5.1-r0/lxml-4.5.1/setupinfo.py", line 114, in ext_modules
|     raise RuntimeError("Dependency missing")
| RuntimeError: Dependency missing
| WARNING: exit code 1 from a shell command.

To be honest, I haven't debugged this issue in details, however release 4.5.1 with setupinfo.py from 4.5.0 builds fine (with --with-xslt-config and --with-xml2-config).

Best regards, Leon

scoder added a commit that referenced this pull request Jun 8, 2020
… again, after accidentally renaming them to "--xml2-config" and "--xslt-config" in 4.5.1.

See #297 (comment)
@scoder
Copy link
Member

scoder commented Jun 8, 2020

@leon-anavi Thanks for noticing, I pushed a fix.

Regarding your second issue, 0.29.2 looks more like a Cython version than a libxml2/libxslt version. What exact command did you use to get this error?

@leon-anavi
Copy link

@leon-anavi Thanks for noticing, I pushed a fix.

Thank you!

Regarding your second issue, 0.29.2 looks more like a Cython version than a libxml2/libxslt version. What exact command did you use to get this error?

Well, the exact command is just bitbake python3-lxml which in a nutshell executes something like:

setup.py  build --build-base=/home/leon/poky-master/build/tmp/work/core2-64-poky-linux/python3-lxml/4.5.1-r0/build  --xslt-config='pkg-config libxslt'  --xml2-config='pkg-config libxml-2.0'

The snippet above is with release 4.5.1 from pypi.org without your recent fix so with --xslt-config and --xml2-config.

If I use again 4.5.1 but with setupinfo.py from 4.5.0 (in this case --with-xslt-config and --with-xml2-config), the Yocto/OE recipe for lxml builds fine so I am suspicious that something might be going on wrong because of the changes regarding version from #300.

Thanks,
Leon

@leon-anavi
Copy link

After further debugging, the issue while building through bitbake seems because of the returned value of get_library_version. For example with flag --xml2-config='pkg-config libxml-2.0' get_library_version returns 0.29 because:

$ pkg-config libxml-2.0 --modversion
2.9.4
$ pkg-config libxml-2.0 --version
0.29

I can fixed for my specific case with a patch for if OPTION_WITH_XML2_CONFIG in get_library_versions and providing two arguments for get_library_version to use PKG_CONFIG and the library name. However, I am not sure what may such a change brake for other cases using OPTION_WITH_XML2_CONFIG.

Could you please let me know for an appropriate solution?

Best regards,
Leon

@hughmcmaster
Copy link
Contributor Author

@leon-anavi 0.29 is the version of your system's pkg-config binary.

Support for pkg-config is integrated into lxml 4.5.1. You no longer need to specify --xml2-config='pkg-config libxml-2.0' or equivalent for libxslt. If you want to use pkg-config, just build without flags: python3 setup.py build.

@leon-anavi
Copy link

Hi @hughmcmaster, @scoder,

Thank you both for the help and the suggestions. I confirm it builds fine without the flags. I am trying to upstream this change to Yocto/OE layer meta-python. I've just submitted an update for the patch and I have added git commit message trailer to both of you: https://lists.openembedded.org/g/openembedded-devel/topic/meta_python_patchv2/74793461?p=,,,20,0,0,0::recentpostdate%2Fsticky,,,20,2,0,74793461

Thanks,
Leon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants