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

Improve build script #81

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Improve build script #81

wants to merge 4 commits into from

Conversation

deviance
Copy link

@deviance deviance commented Jul 12, 2020

  • java/jni/client.c: add support for OpenSSL 1.1
  • Add --{enable,disable}-examples flag to toggle examples compilation
  • Add --with-system-libsafec flag to link against system libsafec
  • configure.ac: Fix AC_ARG_ENABLE/AC_ARG_WITH macros

@deviance
Copy link
Author

@rpb5bnc Hey Pete Beal, I'd like to have this reviewed. Thanks

@dieseldad60
Copy link

dieseldad60 commented Jul 13, 2020 via email

@deviance deviance force-pushed the master branch 2 times, most recently from 8458d95 to 9a76187 Compare July 15, 2020 20:06
Aleksandr Makarov added 4 commits July 17, 2020 18:51
This shall allow the java/jni to build with and link against OpenSSL 1.1.

Additionally, the configuration program will not attempt to process the
java/jni/ subdirectory if no --enable-jni has been specified.

Signed-off-by: Aleksandr Makarov <[email protected]>
Specifying the --with-system-libsafec flag shall allow the configuration
program to search for and, if found, to link against the libsafec library
that is installed in the system.

After configuring --with-system-libsafec, the compilation will may with
following error:

In file included from /usr/include/libsafec/safe_lib.h:43,
                  from est_server_http.c:39:
/usr/include/libsafec/safe_types.h:42:9: error: unknown type name 'size_t'
   42 | typedef size_t  rsize_t;
      |         ^~~~~~

The system libsafec lacks including stddef.h in its safe_types.h.

Fix that by moving libsafec include directives below the openssl; the latter apparently
does the inclusion of the necessary stddef.h at some point.

Signed-off-by: Aleksandr Makarov <[email protected]>
Multiple tests in configure.ac are flawed:

[--snip--]
    AC_ARG_ENABLE([pthreads],
            [AS_HELP_STRING([--disable-pthreads],
                            [Disable support for pthreads])],
            [pthreads_on=1],
            [pthreads_on=0])
[--snip--]

The third argument is "action-if-given" and the fourth argument
is "action-if-not-given" [0]. Which means that, whether you pass
--enable-pthreads or --disable-pthreads, the third argument will be
executed, that is "pthreads_on=1". And if you pass neither, the fourth
argument will be executed, i.e. "pthreads_on=0".

We want `--enable-pthreads` and `--disable-pthreads` flags to do their job.
The right way to do that will be to eliminate "action-if-given" and replace
the user-defined `FEATURE_on=0|1` shell variables with the `enable_FEATURE`
and `with_PACKAGE` shell variables provided by Autotools.

[0] https://www.gnu.org/savannah-checkouts/gnu/autoconf/manual/autoconf-2.69/autoconf.html#Package-Options

Signed-off-by: Aleksandr Makarov <[email protected]>
@deviance
Copy link
Author

Hey guys, @rpb5bnc @dieseldad60
Care to look at this?

@rpb5bnc
Copy link
Contributor

rpb5bnc commented Jul 27, 2020 via email

@deviance
Copy link
Author

The libEST repo is based upon an internal version of libEST, and the
internal version has different requirements then the external
implementation. In order to efficiently maintain the two versions
we work hard to keep them the same whenever possible. For example,
there has generally not been a widely used implementation of the
SafeC APIs in the industry, so the inclusion of the stub SafeC
folder and its use in the external version of liBEST is used to
allow the EST specific code to be identical in this regard in both
internal and external versions. In the case of these new
PRs/issues, the suggested changes understandably cannot take into
account and satisfy the internal requirements.

Sorry, but I do not see how introduction of an optional configuration flag, which, if not given, falls back to a well-known existing option, may be a problem. Appreciate if you clarify which requirements are needed to be satisfied in this case.

Our internal
version of SafeC is generally not installed as an RPM package so the
use of,

PKG_CHECK_MODULES([libsafec],
    [libsafec])

    will not work correctly for our internal uses.  Using the
        PKG_CHECK_MODULE() method to determine if a package is
        installed will not work when the SafeC library has been
        built specifically for a solution versus just installing the
        pre-built package.  Almost all of our product
    teams build our SafeC library for their product and/or platform
    and therefore the package manager will not know about the
    installation.  We use the standard method in configure to test
    for a package by calling an API into the package. 

The PKG_CHECK_MODULE call is not intended to be a solution suitable for both built-in/system libsafec CFLAGS acquisition.
The PKG_CHECK_MODULE is used as a method of CFLAGS acquisition only when --with-system-libsafec is given to ./configure, and if not, then the hard-coded CFLAGS for the stubbed SafeC are used.

However
again, the SafeC API we use for this retrieves the version of
the SafeC library, and this API is not part of the APIs in
Appendix K, so this won't work for outside or a strictly
standard implementation of SafeC. So, taking in these changes
will cause problems inside and just accepting and merging these
changes to the external implementation causes the two
implementations to again diverge further, causing higher
maintenance in the future.

For system libsafec, I've used a https://github.com/rurban/safeclib which you seem to reference as CentOS RPM.
I've not seen API incompatibilities when switching to it for linking.

rpb5bnc added a commit that referenced this pull request Aug 24, 2020
…n with internal version.

- Enable use of an external and non-Cisco implementation of SafeC, both locally and system/distro installed
- Change method for enable/disable options in configure.ac when possible
- Make example build optional. Default enabled.
- Get JNI building (OpenSSL 1.1.x and when client-only is enabled)
- Additional Coverity defect fix in /test util code.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants