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

Fix for #1082 libraries in Linux wheels #1084

Merged

Conversation

keitherskine
Copy link
Collaborator

It appears that the libodbc.so and libltdl.so libraries are added to the linux build wheels by the "repair" step in Github Actions. This step adds any libraries it deems necessary to fulfil the "libraries" attribute in setup.py and also renames the linux build wheel files from "linux" to "manylinux". As far as I can tell, that last rename is not strictly necessary, and can't be separated out, so I'm disabling that entire repair step.

Also, a few other notes:

  • now using the latest version of cibuildwheel, 2.8.1
  • for now, continue to use Debian-based linux images (rather than the latest AlmaLinux image)
  • macos-10.15 will be removed on Aug 30, so update that
  • don't build for musllinux, which seems to be problematic
  • still not building 32-bit wheels, but we might want to re-visit that
  • Debian9 installs unixODBC 2.3.4, which is old but I don't believe that is linked into the final wheels
  • macOS wheels include the libodbc.2.dylib and libltdl.7.dylib libraries, which don't appear to be problematic, and libodbc seems to refer to unixODBC version 2.3.11 (the latest version, which is good)

Please do try out these new build wheels when you get a chance.

@v-chojas
Copy link
Contributor

macOS wheels include the libodbc.2.dylib and libltdl.7.dylib libraries, which don't appear to be problematic

I don't think that should be done either. While Mac doesn't have unixODBC by default, someone who has it (e.g. after installing msodbcsql) will encounter much confusion when pyodbc does something different than when the same connection is made in isql, and some ODBC drivers that call back into the DM will attempt to use the system one, which might not match. Suffice to say that multiple DM libs have caused a lot of problems in the past.

@gordthompson
Copy link
Collaborator

I just checked a couple of the Linux wheels in here

https://github.com/mkleehammer/pyodbc/suites/7440010116/artifacts/304197945

and they look good to me. I do understand why some people would "like" to avoid installing unixODBC themselves, but they are simply unaware of the potential problems. Whatever the outcome—and I'm +1 on this approach—we'll need to update the "Installing on Linux" parts of the wiki, so we should remember to provide a brief explanation of why it's the way it is.

As for 32-bit Windows wheels, I would have been very keen on them a few years ago when I was doing a fair bit of MS Access stuff and Office installs were still mostly 32-bit. However, neither of those things is true now, and 32-bit wheels are still available at

https://www.lfd.uci.edu/~gohlke/pythonlibs/#pyodbc

so I would be inclined to leave them out for the time being.

@gordthompson gordthompson self-requested a review July 26, 2022 22:13
Copy link
Collaborator

@gordthompson gordthompson left a comment

Choose a reason for hiding this comment

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

I have tested the Linux wheels from the "artifacts" file at

https://github.com/mkleehammer/pyodbc/actions/runs/2704829437

against

  • Alpine Linux
  • Oracle Linux 8 and 9 (as a proxy for RHEL), and
  • a couple of versions of Ubuntu

and they seem to work fine. Of course, unixODBC needed to be installed via

  • apk add unixodbc
  • sudo yum install unixODBC, and
  • sudo apt install unixodbc

respectively.

I did notice that the macosx wheels also bundle the two .dylib files

pyodbc.dylibs/libtdl.7.dylib
pyodbc.dylibs/libodbc.2.dylib

but I have no way of testing them. Besides, they may have been built on something newer than the platform that built the .so files for Linux, so they may not cause errors.

However, it might be a tiny bit awkward if the files were bundled for macosx but not for Windows or Linux. TBH, I don't know enough about the Mac universe to hazard a guess.

Anyway, I'm +1 on this overall.

@keitherskine
Copy link
Collaborator Author

As mentioned over on issue 1082, my thoughts on this PR after all the discussion are as follows:

  • include Windows 32-bit wheels, because they can be difficult to compile on your PC
  • don't include unixODBC in the Mac wheels (if that's possible, I need to check)
  • stop providing Linux wheels altogether, because pyodbc should ideally be built from source anyway on Linux
  • also, update the GitHub Action helpers from V2 to V3

I agree we should spruce up the Wiki with better information about installing pyodbc for all OS's, but particularly for Linux and its many flavours. We should update the README as well. As ever, thoughts welcome.

@gordthompson
Copy link
Collaborator

gordthompson commented Jul 27, 2022

One point in favour of providing Linux wheels (without the bundled libraries, that is):

Building pyodbc from source requires build tools, and installing them can consume a non-trivial amount of disk space. Not the several gigabytes of Visual Studio goodness required to build pyodbc on Windows, but still …

Users of distros like Alpine Linux might be particularly vocal about this. On my Alpine Linux VM with

apk install python3 unixodbc
python3 -m ensurepip

df -h shows 179.6M used on /. After I do

apk add python3-dev g++ unixodbc-dev

the used space has increased to 393.3M. So installing just the stuff to build pyodbc more than doubles the used space.

@gordthompson
Copy link
Collaborator

Also, re: 32-bit Windows wheels - If they're easy enough for us to provide, then it's probably a good idea. There could very well be quite a few older oddball Windows ODBC drivers that are 32-bit only, so if somebody needs to use one then at least we could save them the trouble of having to build pyodbc too.

@keitherskine
Copy link
Collaborator Author

Good to hear everybody's thoughts on this. They are greatly appreciated. Further to that, I've updated this PR with what I hope is the general prevailing opinions:

  • Include 32-bit Windows builds.
  • Include ARM builds for macOS.
  • Base the Linux builds on CentOS 7, not Debian 9, because it is older and hence should have wider compatibility. CentOS installs unixODBC 2.3.1 by default but that should be good enough for our purposes.
  • Don't embed any libraries in the wheels.
  • Finally, I've included the standard "source distribution" for good measure, may as well generate all the distributions at the same time.

Please take a look at the artifacts generated by this latest commit and give your thoughts on them:

https://github.com/mkleehammer/pyodbc/actions/runs/2757580203

Copy link
Collaborator

@gordthompson gordthompson left a comment

Choose a reason for hiding this comment

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

Both 32-bit and 64-bit Windows wheels for python 3.10 verified on Windows 8.1. Linux wheel for Python 3.6 verified on Ubuntu 18.04.

Looks good to me.

@keitherskine
Copy link
Collaborator Author

Many thanks for checking the latest builds against various platforms @gordthompson . I have checked the macOS Intel builds and I'm just waiting for a friend to confirm the M1 builds.

@keitherskine
Copy link
Collaborator Author

I haven't heard back from my friend about M1's but I'm going to merge this PR anyway. If we need to address this further, we can do so before the next release.

@keitherskine keitherskine merged commit c717c78 into mkleehammer:master Aug 7, 2022
v-makouz pushed a commit to v-makouz/pyodbc that referenced this pull request Sep 9, 2022
* Add support for Python 3.10, drop EOL 3.5 (mkleehammer#952)

* Remove duplicate entry in pyi stub (mkleehammer#979)

* Replace deprecated SafeConfigParser with ConfigParser (mkleehammer#953)

* Designate connection string as optional (mkleehammer#987)

* Fix spelling typos (mkleehammer#985)

Co-authored-by: Gord Thompson <gord@gordthompson.com>

* Fix for DSN Names with non-ASCII chars (mkleehammer#951)

* Fix for DSN Names with non-ASCII chars

Fixes: mkleehammer#948

Co-authored-by: bamboo <bamboo@galaxy.ad>
Co-authored-by: Gord Thompson <gordon.d.thompson@gmail.com>

* Added InterfaceError to pyodbc.pyi. (mkleehammer#1013)

Co-authored-by: Benjamin Holder <bholder@rpagency.com>

* Upgrade deprecated unicode encoding calls (mkleehammer#792)

* Do not include .pyc artifacts in source tarball mkleehammer#742

* Build wheels with cibuildwheels on GitHub Actions

Fixes mkleehammer#175
Ref mkleehammer#688
Closes mkleehammer#668
Closes mkleehammer#685
Fixes mkleehammer#441 and pretty much most issues that mention
` sql.h: No such file or directory`

This also need to setup some PyPI keys for automated uploads.

* Install unixodbc-dev for Linux wheels

* Enable GitHub Actions for pull requests

* Use Debian based `manylinux_2_24` image

* `apt-get` update before installing in wheel build

* Use PEP 440 version name required for wheels

* Skip building 32-bit wheels

* 4.0.dev0 for default version, because test_version() wants 3 parts here

Checked this won't shadow released minor version (credit goes to @hugovk)

    >>> from packaging.version import Version
    >>> Version("4.0.dev0") > Version("4.0.24")
    False

* Had to use Debian image for PyPy too

* Disable PyPy wheels

https://cibuildwheel.readthedocs.io/en/stable/options/#build-selection

PyPy is missing some C functions that `pyodbc` needs.

* Update README.md

* Avoid error when testing with DSN= connection

Fixes: mkleehammer#1000

* Disable setencoding/setdecoding in tests3/pgtests.py

Fixes: mkleehammer#1004

* Adjust test_columns() in tests3/pgtests.py for newer driver versions

Fixes: mkleehammer#1003

* Move driver version check out of function

* Add comment to _get_column_size()

* Fix memory leak with decimal parameters

Fixes: mkleehammer#1026

* Create codeql-analysis.yml

* Bugfix/sql param data memory leak (mkleehammer#703)

* Updated .gitignore

* * Created a test file for the specific scenario

* * Updated doc of test file for the specific SQLParamData scenario

* * Fixed the test file for the specific SQLParamData scenario by Py_XDECREF the PyObject with 1 reference.

* * Improved the test to close the cursor and set it to None, then forcing the gc

* * Changed the fix of the memory leak and updated the test.

* * Removed redundant empty line

* * Converted tabs to spaces

* * Moved variable out of conn's scope

* Update gitignore, remove duplicated

* Replace deprecated PyUnicode_FromUnicode(NULL, size) calls (mkleehammer#998)

Current versions of Python write a deprecation warning message to
stderr, which breaks CGI scripts running under web servers which
fold stderr into stdout. Likely breaks other software. This change
replaces the deprecated calls with PyUnicode_New(size, maxchar).
The accompanying code to populate the new objects has also been
rewritten to use the new PyUnicode APIs.

* Making pyodbc compatible with PostgreSQL infinity dates, returning MINYEAR and MAXYEAR to python, instead of values out of python's limits

* Removing autoformat from code

* Removing autoformat from code

* Add odbc_config support on mac and m1 homebrew dir

* Note EOL of 2.7 support in README (mkleehammer#945)

* Fix version of CI generated wheels

The CI system is checking out exact tags like "git checkout 4.0.33", which results in a
detached HEAD.  The version calculation was adding the commit hash.

* Fix for mkleehammer#1082 libraries in Linux wheels (mkleehammer#1084)

* use argparse instead of optparse (mkleehammer#1089)

Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
Co-authored-by: Alex Nelson <alexander.nelson@nist.gov>
Co-authored-by: Kian Meng, Ang <kianmeng.ang@gmail.com>
Co-authored-by: Gord Thompson <gord@gordthompson.com>
Co-authored-by: bamboo <bamboo@galaxy.ad>
Co-authored-by: Gord Thompson <gordon.d.thompson@gmail.com>
Co-authored-by: bdholder <benjamin.holder@rocketmail.com>
Co-authored-by: Benjamin Holder <bholder@rpagency.com>
Co-authored-by: Inada Naoki <songofacandy@gmail.com>
Co-authored-by: Michael Fladischer <FladischerMichael@fladi.at>
Co-authored-by: Anatoli Babenia <anatoli@rainforce.org>
Co-authored-by: Francisco Morales <51379487+jose598@users.noreply.github.com>
Co-authored-by: Gord Thompson <gordthompson@users.noreply.github.com>
Co-authored-by: Michael Kleehammer <michael@kleehammer.com>
Co-authored-by: Gilad Leifman <leifmangilad@gmail.com>
Co-authored-by: Bob Kline <bkline@rksystems.com>
Co-authored-by: Leandro Scott <lsrzj@yahoo.com>
Co-authored-by: Jordan Mendelson <jordan@zenzen.org>
Co-authored-by: Keith Erskine <toastie604@gmail.com>
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.

4.0.34 manylinux wheels include old libodbc.so and libltdl.so libraries
3 participants