cisco / libsrtp

Library for SRTP (Secure Realtime Transport Protocol)
Other
1.21k stars 474 forks source link

CMake installs cipher_types.h instead of crypto_types.h #477

Closed bwalden closed 4 years ago

bwalden commented 4 years ago

Running CMake install on 2.3.0 installs the following headers:

Assuming this is incorrect because:

  1. The Alphine Linux libsrtp-dev package deploys:
    • auth.h
    • cipher.h
    • crypto_types.h
    • srtp.h
  2. cipher_types.h includes cipher.h, which includes crypto_types.h. Causing a compiler error because crypto_types.h does not exist.
  3. The makefile install target installs crypto_types.h, not cipher_types.h
paulej commented 4 years ago

I think it's risky to install an older dev package along with the latest code from GitHub.

bwalden commented 4 years ago

Agreed. The CMake install was done on Windows. I only mentioned the Alpine Linux package for the purpose of comparison.

paulej commented 4 years ago

Looking back at the older 1.6 code, if I did a make install, it would install all of these in the include directory:

aes_cbc.h       config.h         err.h            null_cipher.h  srtp.h
aes_gcm_ossl.h  cryptoalg.h      getopt_s.h       prng.h         srtp_priv.h
aes.h           crypto.h         gf2_8.h          rand_source.h  stat.h
aes_icm.h       crypto_kernel.h  hmac.h           rdb.h          ut_sim.h
aes_icm_ossl.h  crypto_math.h    integers.h       rdbx.h         xfm.h
alloc.h         crypto_types.h   kernel_compat.h  rtp.h
auth.h          datatypes.h      key.h            rtp_priv.h
cipher.h        ekt.h            null_auth.h      sha1.h

That was definitely overkill. The current code (using make rather than Cmake) installs:

auth.h  cipher.h  crypto_types.h  srtp.h

So, I think the change to install crypto_types.h is at least internally consistent. :)

I don't know if there are other headers people are using that need to be exposed.

pabuhler commented 4 years ago

I this was just a mistake when creating cmake project, we have no install tests. I guess this is grounds for a 2.3.1 release in the near future.

pabuhler commented 4 years ago

This is fixed in master and no one has complained about it not being in the release so close for now.