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

unix: fix segmentation fault on linking FreeDOS build with DJGPP #2644

Closed

Conversation

@pohmelie
Copy link
Contributor

@pohmelie pohmelie commented Nov 17, 2016

  • add FreeDOS build files to gitignore
  • make fatfs module optional
  • use internal ERRNO
  • add --no-gc-sections to LDFLAGS_EXTRA

Original commit by @prusnak (prusnak@a97e088)

unix/mpconfigport_freedos.h Outdated
@@ -32,13 +32,9 @@
#define MICROPY_STREAMS_NON_BLOCK (0)

#undef MICROPY_PY_SYS_PLATFORM
#define MICROPY_PY_SYS_PLATFORM "freedos"
#define MICROPY_PY_SYS_PLATFORM "FreeDOS"

This comment has been minimized.

@dpgeorge

dpgeorge Dec 2, 2016
Member

The sys.platform string is traditionally all lower case.

unix/mpconfigport.mk Outdated
@@ -34,6 +34,9 @@ MICROPY_SSL_MBEDTLS = 0
# jni module requires JVM/JNI
MICROPY_PY_JNI = 0

# FatFS VFS support
MICROPY_PY_FATFS = 1

This comment has been minimized.

@dpgeorge

dpgeorge Dec 2, 2016
Member

There is already MICROPY_FATFS variable for this.

unix/Makefile Outdated
@@ -103,6 +103,9 @@ ifeq ($(MICROPY_PY_THREAD),1)
CFLAGS_MOD += -DMICROPY_PY_THREAD=1 -DMICROPY_PY_THREAD_GIL=0
LDFLAGS_MOD += -lpthread
endif
ifeq ($(MICROPY_PY_FATFS),1)

This comment has been minimized.

@dpgeorge

dpgeorge Dec 2, 2016
Member

Please use MICROPY_FATFS.

@pohmelie pohmelie force-pushed the pohmelie:fix-freedos-segfault-on-linking branch Dec 2, 2016
@pfalcon
Copy link
Contributor

@pfalcon pfalcon commented Jan 25, 2017

The commit message makes it look like using non-internal errno what caused segfaults, or something.

Building MicroPython with --gc-sections is now actually a requirement, i.e. building without it may break at any time (even if it works for a particular configuration now). So, please add a comment straight at the Makefile, explaining why it's needed, and referencing an upstream bug report on that, so everyone could check when this workaround can be removed.

@pfalcon pfalcon force-pushed the micropython:master branch to 56e7ebf Jan 28, 2017
@pohmelie pohmelie force-pushed the pohmelie:fix-freedos-segfault-on-linking branch Feb 1, 2017
@pohmelie
Copy link
Contributor Author

@pohmelie pohmelie commented Feb 1, 2017

Ok, the problem was with djgpp. New version of binutils (2.27) solve the problem. Discussion:
andrewwutw/build-djgpp#12
https://groups.google.com/forum/#!topic/comp.os.msdos.djgpp/AwKemzSvn6A

Internal errors are used since there is one more error (EOPNOTSUPP) djgpp don't know about. So, in this case using internal errors are good enough I think.

unix/Makefile Outdated
@@ -156,11 +156,13 @@ LIB_SRC_C = $(addprefix lib/,\
timeutils/timeutils.c \
)

ifeq ($(MICROPY_VFS_FAT), 1)

This comment has been minimized.

@dpgeorge

dpgeorge Feb 2, 2017
Member

Are you sure this ifeq is needed? The VFS stuff is disabled by default (eg not included in standard unix build).

unix/Makefile Outdated
MICROPY_PY_BTREE=0 \
MICROPY_VFS_FAT=0 \
MICROPY_FSUSERMOUNT=0 \
MICROPY_FATFS=0

This comment has been minimized.

@dpgeorge

dpgeorge Feb 2, 2017
Member

FSUSERMOUNT and FATFS options are no longer needed. VFS_FAT also shouldn't be needed (it's disabled by default).

@pohmelie pohmelie force-pushed the pohmelie:fix-freedos-segfault-on-linking branch to b36820b Feb 2, 2017
@pohmelie
Copy link
Contributor Author

@pohmelie pohmelie commented Feb 2, 2017

@dpgeorge, thanks. Now it looks much cleaner. 👍

@dpgeorge
Copy link
Member

@dpgeorge dpgeorge commented Feb 6, 2017

Thanks, merged in 5bea6ea.

@dpgeorge dpgeorge closed this Feb 6, 2017
tannewt pushed a commit to tannewt/circuitpython that referenced this pull request Feb 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants