Win32 merge #33
Win32 merge #33
Conversation
…, resulting in build errors Replaced with use of xcopy for windows - a better approach could be found, I think. A DLL imported as a Python module needs to have the extension .pyd - overrode SHRLIB_SUFFIX_BASE to create _dbapi.pyd. Also added Windows library dependencies.
… make them conditional on epics version number.
…nt to keep in main branch.
mdavidsaver
left a comment
mdavidsaver
left a comment
There was a problem hiding this comment.
Looks good aside from some minor cleanup.
configure/RULES_PY
Outdated
| install -m 644 $< $@ | ||
| else | ||
| xcopy /S /Q /Y /F $(subst /,\,$<) $(subst /,\,$@)* | ||
| endif |
There was a problem hiding this comment.
You (and I...) should use $(INSTALL) as is done by the install rules in Base.
devsupApp/src/Makefile
Outdated
|
|
||
| LOADABLE_LIBRARY_HOST += _dbapi | ||
| ifeq ($(OS),Windows_NT) | ||
| SHRLIB_SUFFIX_BASE = .pyd |
There was a problem hiding this comment.
I think this should be in configure/CONFIG_PY, where the equivalent for Darwin/OSX is set.
ifeq ($(OS_CLASS),WIN32)
LOADABLE_SHRLIB_SUFFIX = .pyd
endifThere was a problem hiding this comment.
OK, but I get a build error - which I can resolve by setting:
LOADABLE_LIBRARY_HOST_DEFAULT += _dbapi
LOADABLE_LIBRARY_HOST_WIN32 += _dbapi.pyd
There was a problem hiding this comment.
It does need to be SHRLIB_SUFFIX_BASE (setting LOADABLE_SHRLIB_SUFFIX does not have the desired effect).
But this can be, as you suggest, in CONFIG.PY.
devsupApp/src/devsup/__init__.py
Outdated
| _dbapi.dbReadDatabase(dbd_name) | ||
| # e.g. 3.15.0.2 -> 30000 + 1500 + 0 + 2 = 31502 | ||
| epics_version_int = EPICS_VERSION * 10000 + EPICS_REVISION * 100 + EPICS_MODIFICATION * 10 + EPICS_PATCH_LEVEL | ||
| if epics_version_int >= 31502: |
There was a problem hiding this comment.
While this will certainly work, fyi. tuple comparison is shorter and easier to understand. (cf. sys.version_info)
epics_version_int = (EPICS_VERSION, EPICS_REVISION, EPICS_MODIFICATION, EPICS_PATCH_LEVEL)
if epics_version_int >= (3, 15, 0, 2):
makehelper.py
Outdated
| from distutils.sysconfig import get_python_inc, get_python_lib | ||
| except ImportError: | ||
| def get_python_inc(): | ||
| return get_config_var('INCLUDEPY') or '' |
There was a problem hiding this comment.
Is a fallback get_python_lib() needed here as well?
pyIocApp/setup.c
Outdated
| #include "pydevsup.h" | ||
| #ifdef _WIN32 | ||
| #define PATH_MAX _MAX_PATH | ||
| #include <direct.h> |
There was a problem hiding this comment.
Does the order of these two lines change the behavior of direct.h?
There was a problem hiding this comment.
No, I don't think so. It's more appropriate the other way round.
And _MAX_PATH is actually defined in stdlib.h.
pyIocApp/setup.c
Outdated
|
|
||
| if(PyRun_SimpleString("import devsup\n" | ||
| "devsup._init(iocMain=True)\n" | ||
| "devsup._init(iocMain=False)\n" |
There was a problem hiding this comment.
Is this change necessary now that _init() does not use NamedTemporaryFile?
There was a problem hiding this comment.
The two are separate in _init(). I'm not too sure what was intended here, but I do not see any existing call of _init() that caused base to load, and this is still the case.
There was a problem hiding this comment.
base would of course have been loaded by the IOC start-up.
So I'm not sure whether loading it from _dbapi is actually required anyway.
requirements-deb10.txt
Outdated
| @@ -1,2 +1,3 @@ | |||
| numpy==1.16.2 | |||
| nose==1.3.7 | |||
| #nose==1.3.7 | |||
There was a problem hiding this comment.
I think references to nose v1 can be entirely removed.
|
Some files which I don't think should be changed/added by this PR.
|
|
I am not sure why the github actions jobs have not started. Maybe the conflict in |
…CONFIG_BASE for the definition of the INSTALL macro. epicsThreadExitMain is deprecated by recent EPICS base versions (and resolves to cantProceed). Being unsure as to what best to do about this, I commented it out in devSupMain.cpp. Python 2.7 has been obsoleted and won't install on the runner without taking special measures. Being unsure what to do about this, I have commented out builds that invoke 2.7. Github Windows-latest (i.e. Windows server 2022) is installed with VS2022 (not VS2019). Use of the debug build configuration will seek to link with the Python debug libraries that are not normally installed. I have a solution to this problem but we should debate how to proceed. Github Windows server 2019 is, I think, installed with VS2019, this should build OK with default configuration.
|
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 In *nix convention, In this context, my goal was that |
|
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 += I've tested this on Red Hat ('-L. -lpython3.6m') and Ubuntu ('-lpython3.10'). The BLDLIBRARY variable is not emitted from Python on Windows. Is this your preferred approach? |
|
Where I'm coming from with this is that the Windows build failed because it couldn't locate the .LIB file. I have no idea at this time why the Python 3.7 .. 3.9 builds failed - where the 3.10 build succeeded. |
On Windows, the library isn't in a system folder so the fully-qualified path is required.
|
@mdavidsaver: Where are we on this? 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... |
|
There will be a potential conflict in |
|
The conflict has been addressed. |
|
I actually do not think your fix to 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 |
|
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
<#33 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AD6SZN3UOLWHOUK3TUAHJTTZQSU5JAVCNFSM6AAAAABLOTLSZKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENZXG43TSNZWGI>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
I confirm that my version does not work with Python 3.12, due to distutils being removed. @eddybl, I saw your pull request #39 . I confirm that it works with Linux to 3.12. It does not work (as it stands) with Windows because: I have created a merged version from yours, and tested it in my 'devel' branch, with the necessary changes for Windows use. I have not added this to my pull request, to date. |
|
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. |
|
Done, and thank you for your excellent input to this. |
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...) |
|
Thank you @mdavidsaver for being so open. I am willing to be a maintainer. My thoughts: |
Excluded int64 and long string for that reason. Base 3.14 doesn't work because RuntimeError: Requires Base >=3.15.
Updated the readme file with changed repository location.
4 participants
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.