Skip to content

bpo-31904 : Add support for VxWorks RTOS #4184

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

Closed
wants to merge 221 commits into from

Conversation

kuhlenough
Copy link

@kuhlenough kuhlenough commented Oct 31, 2017

This pull request enables cpython to cross-build on VxWorks, it is obsolete and only provided for reference of the scope of changes..
Smaller more manageable PRs against the current master as requested on python-dev will follow.

All doesn't include shared if static build
Hardwire asscii encoding for VxWorks
default to UTF-8 (like Android)
Semicolon as path separator
If build is configure'd for static, propigate to Makefile
Add detection for oddly named VxWorks OPENSSL and HASH libraries
Fix some warnings with unused functions and macro re-def
Extra include in faulthandler.c
Avoid duplicate macro definition
Setup.py complies all VxWorks compatible modules
Update some extension modules to compile on VxWorks
VxWorks does not have "timezone" and daylight "constants"
Limited signal fields in VxWorks
undef DATE TIME for VxWorks
@kuhlenough
Copy link
Author

Travis failures appear completely unrelated to this pull request?

setup.py Outdated
@@ -516,7 +524,7 @@ def detect_modules(self):
add_dir_to_list(self.compiler.library_dirs, '/usr/local/lib')
add_dir_to_list(self.compiler.include_dirs, '/usr/local/include')
# only change this for cross builds for 3.3, issues on Mageia
if cross_compiling:
if ( cross_compiling and not host_platform == 'vxworks'):
Copy link
Member

Choose a reason for hiding this comment

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

The extra space after the paren here is probably the source of the patchcheck warning on TravisCI.

@@ -1463,9 +1463,9 @@ PyInit_mmap(void)
#endif

setint(dict, "PAGESIZE", (long)my_getpagesize());

#ifndef __VXWORKS__
Copy link
Member

Choose a reason for hiding this comment

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

I suggest to define my_getallocationgranularity to my_getpagesize above like on Unix.

Copy link
Author

Choose a reason for hiding this comment

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

Your correct this is not required.

@@ -174,6 +174,7 @@ corresponding Unix manual entries for more information on calls.");
#define HAVE_FSYNC 1
#define fsync _commit
#else
#ifndef __VXWORKS__
Copy link
Member

Choose a reason for hiding this comment

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

Are all of this function absent? It looks suspicious that all of them are grouped alphabetically.

Copy link
Author

Choose a reason for hiding this comment

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

A couple are present in some configurations of VxWorks, but following comment is dated.
"Unix functions that the configure script doesn't check for"
The automake configure script finds all these without issue. So a better change would be to simply remove all these and let automake do it's job. I'm just not prepared to regression test the 1/2 dozen other OS referenced in this section.

@@ -181,6 +181,12 @@ if_indextoname(index) -- return the corresponding interface name\n\
# endif
#endif

#ifdef __VXWORKS__
# include <ipcom_sock2.h>
# define gethostbyaddr_r( a1, a2, a3, a4, a5, a6, a7 ) ipcom_gethostbyaddr_r( a1, a2, a3, a4, a5, a6, a7 )
Copy link
Member

Choose a reason for hiding this comment

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

Too long line. Why not just define gethostbyaddr_r to ipcom_gethostbyaddr_r?

# define gethostbyaddr_r ipcom_gethostbyaddr_r

@@ -532,7 +538,7 @@ set_error(void)
return PyErr_SetFromErrno(PyExc_OSError);
}


#ifndef __VXWORKS__
Copy link
Member

Choose a reason for hiding this comment

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

Is HAVE_HSTRERROR defined?

Copy link
Author

Choose a reason for hiding this comment

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

yes, but VxWorks has no h_errno also

setup.py Outdated
@@ -851,20 +871,31 @@ def detect_modules(self):
['/usr/kerberos/include'])
if krb5_h:
ssl_incs += krb5_h
ssl_libs = find_library_file(self.compiler, 'ssl',lib_dirs,
if host_platform == 'vxworks':
Copy link
Member

Choose a reason for hiding this comment

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

The code below is mostly duplicated. The only differences are few arguments. Wouldn't be easy to just set platform depending local variables for these arguments? The value for libraries also is used below.

@@ -525,7 +532,7 @@ set_error(void)
return PyErr_SetFromErrno(PyExc_OSError);
}


/*#ifndef __VXWORKS__ */
Copy link
Member

Choose a reason for hiding this comment

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

If this no longer needed, remove it.

@@ -1421,13 +1421,15 @@ PyInit_timezone(PyObject *m) {
#if defined(HAVE_TZNAME) && !defined(__GLIBC__) && !defined(__CYGWIN__)
PyObject *otz0, *otz1;
tzset();
#ifndef __VXWORKS__
PyModule_AddIntConstant(m, "timezone", timezone);
Copy link
Member

Choose a reason for hiding this comment

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

Does timezone not exist? What about _timezone?

Copy link
Author

Choose a reason for hiding this comment

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

No timezone, no _timezone, you have to use tzset/tzget

configure Outdated
@@ -2926,7 +2926,7 @@ $as_echo_n "checking for python interpreter for cross build... " >&6; }
if $interp -c "import sys;sys.exit(not '.'.join(str(n) for n in sys.version_info[:2]) == '$PACKAGE_VERSION')"; then
break
fi
interp=
Copy link
Member

Choose a reason for hiding this comment

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

Why this line is removed? Is configure generated from configure.ac? What version of autoconf you have used?

Copy link
Author

Choose a reason for hiding this comment

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

I haven't checked in a regenerated configure yet. What version do you use?

Copy link
Member

Choose a reason for hiding this comment

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

The last stable version is 2.69.

setup.py Outdated
@@ -503,7 +511,7 @@ def add_gcc_paths(self):

def detect_math_libs(self):
# Check for MacOS X, which doesn't need libm.a at all
if host_platform == 'darwin':
if (host_platform == 'darwin' or host_platform == 'vxworks'):
Copy link
Member

Choose a reason for hiding this comment

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

Parenthesis are redundant.

setup.py Outdated
@@ -516,7 +524,7 @@ def detect_modules(self):
add_dir_to_list(self.compiler.library_dirs, '/usr/local/lib')
add_dir_to_list(self.compiler.include_dirs, '/usr/local/include')
# only change this for cross builds for 3.3, issues on Mageia
if cross_compiling:
if (cross_compiling and not host_platform == 'vxworks'):
Copy link
Member

Choose a reason for hiding this comment

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

Parenthesis are redundant.

configure Outdated
@@ -2926,7 +2926,7 @@ $as_echo_n "checking for python interpreter for cross build... " >&6; }
if $interp -c "import sys;sys.exit(not '.'.join(str(n) for n in sys.version_info[:2]) == '$PACKAGE_VERSION')"; then
break
fi
interp=
Copy link
Member

Choose a reason for hiding this comment

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

The last stable version is 2.69.

configure.ac Outdated
*-*-cygwin*)
_host_cpu=
;;
*-*-cygwin*)
Copy link
Member

Choose a reason for hiding this comment

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

Seems this is not indented as above lines.

@gvanrossum
Copy link
Member

Yes, we'll clean this up the cruff and squash some commits.
I have no expectation that this pull will be merged until some discussion on long term support is agreed upon

Who is "we" here? Are you just a happy VxWorks user or are you working for the org or company that owns it (assume there is such an org or company)?

This reverts commit 7f38637.

# Conflicts:
#	Doc/tools/templates/indexsidebar.html
@terryjreedy terryjreedy removed their request for review April 16, 2018 21:55
@kuhlenough
Copy link
Author

Wind River owns VxWorks. ( https://www.windriver.com/products/vxworks/ It really is the most pervasive OS you've never heard of. ) I'm a manager at Wind River, as side project I contribute VxWorks support to open source projects. I work with the VxWorks Product Manager. The product manager, Michel, wants "official" python support. In reply to bpo-31904 bug/enhancement Victor (vstinner) mentioned we should have a buildbot to run against pull requests as part of any official support. So I hired an intern, Oscar @ooosssososos, to work on that. He's also been working on getting the test suite to pass with VxWorks. ( His work term ends soon, so we just merged his work into my branch and it seems we got some unwanted commits with that :( )

On my side I'd like to get some additional POSIX support into VxWorks, e.g. proper handling of fcntl( ,F_SETFD, FD_CLOEXEC) and that effort may take awhile.

This reverts commit 18463dd.

# Conflicts:
#	Include/pystate.h
This reverts commit 0ad214f.
This reverts commit dd55e39.

# Conflicts:
#	Lib/test/test_support.py
@skrah skrah removed their request for review April 17, 2018 14:44
@gpshead gpshead removed their request for review September 10, 2018 23:33
import subprocess
try:
return subprocess.Popen(args).pid
except:
Copy link
Member

Choose a reason for hiding this comment

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

This is not good. First, 'try' is bare 'except' is bad news, hides bugs. Second, you are doing the 'vxworks' test on every function call, for every platform. Instead you could have something like:

def spawnv_passfds(...):
    ...

if 'vxworks' in sys.platform:
  # a comment explaining why this is needed
  def spawnv_passfds(...):
    # implementation of vxworks special version

@@ -753,7 +753,7 @@ def _syscmd_uname(option, default=''):
return default
try:
f = os.popen('uname %s 2> %s' % (option, DEV_NULL))
except (AttributeError, OSError):
except (AttributeError, OSError, ValueError):
Copy link
Member

Choose a reason for hiding this comment

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

Looks suspicious. Does os.popen() now return different exceptions? I suspect you should fix os.popen() to trap ValueError and return OSError instead. Otherwise everyone who uses os.popen() has to deal with the new exception case.

# way to find if a path is absolute. V7COR-3074, F7233
if not isinstance(s, str):
s = s.decode(sys.getdefaultencoding())
return bool(_vxwapi.isAbs(s))
Copy link
Member

Choose a reason for hiding this comment

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

Again, you are doing the _vxworks test on every function call for every platform. Not a huge deal but isabs() might be called a lot. Instead, define a new isabs() on vxworks.

ind = rest.find(':')
ind = 0 if ind < 0 else ind
path = rest[:ind]
rest = rest[ind:]
Copy link
Member

Choose a reason for hiding this comment

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

Again, runtime testing of _vxworks. Better to define a special _joinrealpath() function at module import time.

@@ -138,7 +138,7 @@ def test_file_not_found_in_home(self):
def test_file_not_found_explicit(self):
self.assertRaises(FileNotFoundError, netrc.netrc,
file='unlikely_netrc')

@unittest.skipIf('vxworks' in sys.platform, 'VxWorks does not require or have a HOME directory')
def test_home_not_set(self):
Copy link
Member

Choose a reason for hiding this comment

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

Sprinkling verbose tests like this throughout the code is not the way. Instead, you should define a global flag at the top of the module, named based on the feature that is not on VxWorks. E.g.

_HAVE_HOME = 'vxworks' not in sys.platform

Then, for the test functions:

@unittest.skipIf(not _HAVE_HOME)

Better yet perhaps is to add those flags to a central module, e.g. platform. Doing it this way makes it more clear what is going on. Also, if a new platform is added, the tests for platform specific features becomes easier to understand.

# endif
# ifndef FALSE
# define FALSE 0
# endif
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't look correct to me. multiprocessing requires TRUE = 1. I think you should 'undef' the existing TRUE/FALSE and define what multiprocessing expects.

@nascheme
Copy link
Member

The code quality in this PR is pretty far from the standard needed to get merged into cpython. The number of changes needed to support VxWorks doesn't look too extreme. So, if we do want to support it, I think it can be done without too much pain. Probably we should decide if we are able to support the new platform before putting a lot of effort into polishing the code and getting it to the quality sufficient for merging.

@vstinner
Copy link
Member

This PR is just too big, it's not possible to review it. I close it.

You should continue the effort that you already started to split such giant PR into "atomic" changes for fix one issue per PR.

@vstinner vstinner closed this Mar 27, 2019
@kuhlenough
Copy link
Author

NP. thanks fro your support Victor

@kuhlenough kuhlenough mannequin mentioned this pull request Jan 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.