search

mdavidsaver / pyDevSup

EPICS Device Support in Python
GNU General Public License v2.0
13 stars 16 forks source link

Win32 merge #33

Open pheest opened 1 year ago

pheest commented 1 year ago

In Makehelper.py, replaced use of Linux-specific library path configuration variables with get_python_lib() call.

In Rules.py, added use of xcopy to copy Python files to the install path for Windows use.

In devsupapp/source/dbfield.c, added support capability for lsi record type by: replacing fixed 40-charachter string limit with variable length. Calling the 'special' method of lsiRecord.c to set the record string length.

In devsupapp/source/makefile: A Windows Python loadable module has the file extenstion '.pyd' (not .dll). Overrode the SHRLIB_SUFFIX_BASE variable to apply this. Added new files _dbapi.dbd, _int64.dbd and _lsilso.dbd. Set these to copy to the same folder as Python files (although they aren't).

In devsupapp/source/devsup: added _dbapi.dbd, _int64.dbd and _lsilso.dbd in init.py, replaced use of temporary file with the above static files. _int64 and _lsilso are loaded conditionally on the EPICS base version.

In devsupapp/source/devsup/disect.py: InstanceType is imported at line 6 for Python 2 usage, and will raise an exception with Python 3. This is tested at line 84 with elif InstanceType is not None. But when the excpetion is raised, InstanceType is not None, it is undefined - causing the application to fail. Added InstanceType = None to ensure this doesn't happen.

In devsuppapp/source added changes to support alarm handling on output PVs, according to the mail sent on 06/07. To control the stat and sevr record fields.

In pyiocapp/setup.c added the definition PATH_MAX (as _MAX_PATH) for Windows use. In devsupapp/source/devsup/init.py _init() EPICS base.dbd is loaded on condition 'if not iocMain'. But In pyiocapp/setup.c passed 'iocMain = True' in pySetupReg() ... so where is EPICS base.dbd loaded in _dbapi? Changed to iocMain = False.

mdavidsaver commented 1 year ago

Some files which I don't think should be changed/added by this PR.

mdavidsaver commented 1 year ago

I am not sure why the github actions jobs have not started. Maybe the conflict in makehelper.py. Maybe something to do with an ongoing issue. https://www.githubstatus.com/

pheest commented 1 year ago

Please may I query the intended purpose of the PY_LDLIBS variable that is output from makehelper.py?

mdavidsaver commented 1 year ago

Please may I query the intended purpose of the PY_LDLIBS variable that is output from makehelper.py?

from sysconfig import get_config_var
print(get_config_var('BLDLIBRARY'))

On linux/debian 12 this prints -lpython3.11.

In nix convention, LDLIBS contains `-lentries mentioning library names, whileLDFLAGShas-L*` setting the search path.

In this context, my goal was that PY_LDLIBS contain -lpython* as well as any additional dependencies. Possibly libraries like -ldl, -lz, or maybe winsock.

pheest commented 1 year ago

Thank you, Michael,

My point is that the PY_LDLIBS variable does not appear to have been applied within the build process (unless I'm missing something...).

The variable can be applied with e.g. pyDevSup$(PY_LD_VER)_LDFLAGS += $(PY_LDLIBS), in replacement of e.g. pyDevSup$(PY_LD_VER)_SYS_LIBS += python$(PY_LD_VER).

I've tested this on Red Hat ('-L. -lpython3.6m') and Ubuntu ('-lpython3.10').

The BLDLIBRARY variable is not emitted from Python on Windows. I've created it in makehelper.py for that case.

Is this your preferred approach?

pheest commented 1 year ago

Where I'm coming from with this is that the Windows build failed because it couldn't locate the .LIB file. It worked on my PC because I had set LIB=%LIB%;C:\Python310\libs in my win32.bat file. That's not a portable approach, and I removed it.

I have no idea at this time why the Python 3.7 .. 3.9 builds failed - where the 3.10 build succeeded.

mdavidsaver commented 1 year ago

(I am not sure why GHA is making me approve each run?)

Sorry, I had forgotten how few of the sysconfig variables appear in windows builds... which will likely need to remain a special case.

fyi. python -m sysconfig on Windows shows.

    BINDIR = "C:\hostedtoolcache\windows\Python\3.10.11\x64"
    BINLIBDEST = "C:\hostedtoolcache\windows\Python\3.10.11\x64\Lib"
    EXE = ".exe"
    EXT_SUFFIX = ".cp310-win_amd64.pyd"
    INCLUDEPY = "C:\hostedtoolcache\windows\Python\3.10.11\x64\Include"
    LIBDEST = "C:\hostedtoolcache\windows\Python\3.10.11\x64\Lib"
    SO = ".cp310-win_amd64.pyd"
    TZPATH = ""
    VERSION = "310"
    abiflags = ""
    base = "C:\hostedtoolcache\windows\Python\3.10.11\x64"
    exec_prefix = "C:\hostedtoolcache\windows\Python\3.10.11\x64"
    installed_base = "C:\hostedtoolcache\windows\Python\3.10.11\x64"
    installed_platbase = "C:\hostedtoolcache\windows\Python\3.10.11\x64"
    platbase = "C:\hostedtoolcache\windows\Python\3.10.11\x64"
    platlibdir = "lib"
    prefix = "C:\hostedtoolcache\windows\Python\3.10.11\x64"
    projectbase = "C:\hostedtoolcache\windows\Python\3.10.11\x64"
    py_version = "3.10.11"
    py_version_nodot = "310"
    py_version_nodot_plat = "310"
    py_version_short = "3.10"
    srcdir = "C:\hostedtoolcache\windows\Python\3.10.11\x64"
    userbase = "C:\Users\runneradmin\AppData\Roaming\Python"
pheest commented 1 year ago

So the order of libraries passed to the linker matters. See https://stackoverflow.com/questions/11893996/why-does-the-order-of-l-option-in-gcc-matter. LDFLAGS go before LIBS and o files. Passing the library by means of LDFLAGS may not be such a good idea, after all.

Building with debug configuration means dependency on on the debug python library (python310_d.lib), which isn't installed.

There are some specific instructions regarding macOS at https://github.com/actions/setup-python/blob/main/docs/advanced-usage.md.

pheest commented 1 year ago

I am not sure why the github actions jobs have not started. Maybe the conflict in makehelper.py. Maybe something to do with an ongoing issue. https://www.githubstatus.com/

https://docs.github.com/en/actions/managing-workflow-runs/approving-workflow-runs-from-public-forks

ralphlange commented 2 months ago

@mdavidsaver: Where are we on this? To me, it looks as if there are only minor issues remaining. (If any.)

Background: Peter uses this on ITER-related work, so I need to include it in our CODAC packaging of pyDevSup. Obviously, I would prefer an upstream merge, as opposed to something I do only for our CODAC packaging = that I might need to clean up later...

eddybl commented 1 month ago

There will be a potential conflict in makehelper.py if #39 is merged

pheest commented 1 month ago

The conflict has been addressed. This also required to address the numpy 2.0 issue noted by another user.

eddybl commented 1 month ago

I actually do not think your fix to makehelper.py will work, as distutils.sysconfig and sysconfig are not replaceable and get_python_inc is not available from sysconfig

But maybe we should just wait for the other PR to be merged (or maybe modified). I just wanted to note down that there might be a potential change required here down the line, depending on the PR resolve order

pheest commented 1 month ago

It is working. It is set up to build on GitHub with a selection of platforms and Python versions; these show green. I understand that distutils is deprecated, but I hadn't been aware of your second point. The exception block does appropriately implement get_python_inc if this isn't available.

On Fri, Aug 9, 2024 at 12:56 PM eddybl @.***> wrote:

I actually do not think your fix to makehelper.py will work, as distutils.sysconfig and sysconfig are not replaceable and get_python_inc is not available from sysconfig

But maybe we should just wait for the other PR to be merged (or maybe modified). I just wanted to note down that there might be a potential change required here down the line, depending on the PR resolve order

— Reply to this email directly, view it on GitHub https://github.com/mdavidsaver/pyDevSup/pull/33#issuecomment-2277779762, or unsubscribe https://github.com/notifications/unsubscribe-auth/AD6SZN3UOLWHOUK3TUAHJTTZQSU5JAVCNFSM6AAAAABLOTLSZKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENZXG43TSNZWGI . You are receiving this because you authored the thread.Message ID: @.***>

pheest commented 1 month ago

I confirm that my version does not work with Python 3.12, due to distutils being removed.

@eddybl, I saw your pull request https://github.com/mdavidsaver/pyDevSup/pull/39 . That is considerably cleaner, thank you.

I confirm that it works with Linux to 3.12.

It does not work (as it stands) with Windows because:

The library directory is not contained within 'LIBDIR' but within 'LIBDEST'.

'BASECFLAGS' does not exist, so the compiler tries to build a non-existent file called 'Null'.

get_python_version() returns e.g. 3.11 (both Linux and Windows), so the linker tries to use python3.11.lib - but the file is actually python311.lib.
'VERSION' contains '3.11' on Linux but '311' on Windows, so that works for both.

The Windows Python library path is not a system path, so that needs to be explicitly specified.

I have created a merged version from yours, and tested it in my 'devel' branch, with the necessary changes for Windows use. I have verified that it works for all Python versions to 3.12 on both Linux and Windows.

I have not added this to my pull request, to date. Should I?

eddybl commented 1 month ago

Yes, so i have not considered windows during my changes.

I think it makes sense to push your changes from devel also to this PR.

How the PRs will be resolved is then a question for @mdavidsaver: For example if this PR might get merged soonish, there is no need to merge #39. But if this PR might still be open for a while it might make sense to already merge #39 now, because it is a rather small fix and in that case we most likely have to resolve a small merge conflict for this PR, but that should be doable.

pheest commented 1 month ago

Done, and thank you for your excellent input to this.

mdavidsaver commented 1 month ago

How the PRs will be resolved is then a question for @mdavidsaver ...

At this point, I think it is apparent to everyone that I don't have sufficient time to act as maintainer of pyDevSup, and it isn't clear when I will. I also don't want my lack of time to be a barrier to other who are willing to contribute.

So what to do?

Are others willing to take over review? Full maintainer? Transfer this repository? A pyDevSup2 fork? (in keeping with my unimaginative naming...)

pheest commented 1 month ago

Thank you @mdavidsaver for being so open.

I am willing to be a maintainer.

My thoughts: I think there should ideally be more than one maintainer. I take the view that the module should be hosted in an organizational site, rather than an individual person's. I believe maintainers should not approve their own pull requests. I don't see a need to change the module name.