OS400 fails to build 7.52.1 #1237

Open
jonrumsey opened this Issue Feb 1, 2017 · 8 comments

Projects

None yet

3 participants

@jonrumsey

[ There are collections of known issues to be aware of:
https://curl.haxx.se/docs/knownbugs.html https://curl.haxx.se/docs/todo.html ]

I did this

Build 7.52.1 release on OS/400 V7R1M0

I expected the following

Clean build

curl/libcurl version

7.52.1
[curl -V output perhaps?]

operating system

OS/400 V7R1M0

The build is failing because the definition of CURLOPT_SOCKS_PROXY isn't defined (appears to have been renamed CURLOPT_PRE_PROXY) is still in the OS/400 specific build files (curl.inc.in, ccsidcurl.c and README.OS400).

Secondly, there are a number of assert() calls in http2.c, memdebug.c, mprintf.c and rand.c that should be behind a #ifdef HAVE_ASSERT_H condition, these result in an unresolved external when linking the service program.

@jay jay added the build label Feb 1, 2017
@jay
Member
jay commented Feb 1, 2017

assert is defined in the C standard, you don't have it in OS/400?

@jay jay added a commit to jay/curl that referenced this issue Feb 1, 2017
@jay jay OS400: Fix symbols
- s/CURLOPT_SOCKS_PROXY/CURLOPT_PRE_PROXY
  Follow-up to 7907a2b and 845522c.

- Fix incorrect id for CURLOPT_PROXY_PINNEDPUBLICKEY.

- Add id for CURLOPT_ABSTRACT_UNIX_SOCKET.

Bug: curl#1237
Reported-by: jonrumsey@users.noreply.github.com
a49d2d0
@jay
Member
jay commented Feb 1, 2017

I just made a symbols fix, can you try it?
https://github.com/jay/curl/compare/fix_os400_symbols
I don't know what to do about the assert calls yet until you get back to me.

@jonrumsey

Thanks jay, the symbols fix works great.

OS/400 does have an assert.h, I think the problem here is that the code only "#include <assert.h>" if HAVE_ASSERT_H is defined (but it is not defined by packages/OS400/initscript.sh).

Even so, would it not be appropriate to be able to disable asserts in a build ? Looks like in memdebug.c for platforms that don't have this, assert is defined as a macro to Curl_nop_stmt.

@jay
Member
jay commented Feb 3, 2017 edited

assert.h is a standard header since C89 so I don't know why HAVE_ASSERT_H. Who doesn't have it? I suppose we could define it for OS400 but I'd rather get rid of it altogether.

would it not be appropriate to be able to disable asserts in a build

Another thing I don't know. Why are regular asserts used in those places instead of DEBUGASSERT? Some of the curl build systems define NDEBUG (makes assert a no-op) and some don't so there is some inconsistency in using assert.

@jonrumsey

OS/400 doesn't use configure like UNIX platforms so the "HAVE_xxx_H" macro needs to be defined in the script. Defining it in initscript.sh fixes the build, but you are right, if we expect every platform to have assert.h then its debatable whether it should be behind a conditional directive in the first place.

@bagder
Member
bagder commented Feb 3, 2017

Why are regular asserts used in those places instead of DEBUGASSERT?

Just sloppiness I think. We should add a check for that to checksrc to make sure we don't let them slip through...

@jay jay added a commit to jay/curl that referenced this issue Feb 5, 2017
@jay jay build: Assume assert.h and limits.h are always available. draft1
- Get rid of HAVE_ASSERT_H and HAVE_LIMITS_H tests and ifdef guards.
  assert.h and limits.h are both standard headers in the C standard.

- Ban assert macro everywhere but tests/, use DEBUGASSERT instead.

Ref: curl#1237 (comment)
beaa66c
@jay
Member
jay commented Feb 5, 2017

Apparently assert.h can be missing on WinCE sometimes, but I think this is a problem of WinCE. assert.h should always be available unless freestanding (ie we aren't using standard libraries and OS functions).

First draft removes guards around assert.h and limits.h, both standard headers. In addition assert is banned everywhere except tests/. Could assert be useful in tests or elsewhere where an assert is needed if not a DEBUGBUILD? For example all the asserts in memdebug I changed to DEBUGASSERT, but is that a good idea? For example someone is debugging without DEBUGBUILD? I'm having second thoughts, maybe there is reason I am not aware of it is useful to have both assert and DEBUGASSERT?

https://github.com/curl/curl/compare/master...jay:build_ban_assert?expand=1

@bagder
Member
bagder commented Feb 7, 2017

I'm not sure I see the value in removing the #ifdef protections around the headers. They may be standard, but I'm sure there are lots of platforms and compilers out there that don't totally adhere to standards. Now I tried to research why we added the configure checks for them but the commits were light on details and very old so I couldn't figure that out.

I would suggest that we leave the checks and #ifdefs for them, but change to DEBUGASSERT() all over. With DEBUGASSERT it seems more up to us to control if they're present in a non-debug build or not. I'm not sure users will actually set NDEBUG.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment