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

Generate only one large library "libtoxcore". #442

Merged
merged 1 commit into from Dec 29, 2017

Conversation

Projects
None yet
8 participants
@iphydf
Member

iphydf commented Jan 16, 2017

This library contains all the code for the old libtoxcore, libtoxav,
libtoxdns, and libtoxencryptsave. The build for toxav is still optional,
and disabling it causes libtoxcore to simply not contain those symbols
and the pkg-config file to not include opus and vpx as dependencies.


This change is Reviewable

@iphydf iphydf added this to the v0.2.0 milestone Jan 16, 2017

@cebe

This comment has been minimized.

Show comment
Hide comment
@cebe

cebe Jan 17, 2017

Member

@iphydf can you explain what you are doing here and why?

Member

cebe commented Jan 17, 2017

@iphydf can you explain what you are doing here and why?

@iphydf

This comment has been minimized.

Show comment
Hide comment
@iphydf

iphydf Jan 22, 2017

Member

@cebe I've added a top level comment to the CMakeLists.txt explaining what it does. Why: avoiding the ABI mess that is toxcore.

Member

iphydf commented Jan 22, 2017

@cebe I've added a top level comment to the CMakeLists.txt explaining what it does. Why: avoiding the ABI mess that is toxcore.

@robinlinden

This comment has been minimized.

Show comment
Hide comment
@robinlinden

robinlinden Jan 27, 2017

Member

Reviewed 11 of 11 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

Member

robinlinden commented Jan 27, 2017

Reviewed 11 of 11 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@cebe cebe removed their assignment Jan 29, 2017

@SkyzohKey

This comment has been minimized.

Show comment
Hide comment
@SkyzohKey

SkyzohKey Feb 13, 2017

:lgtm_strong:


Reviewed 11 of 11 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

SkyzohKey commented Feb 13, 2017

:lgtm_strong:


Reviewed 11 of 11 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@SkyzohKey SkyzohKey added the PR:ready label Mar 21, 2017

@iphydf iphydf assigned nurupo and unassigned jin-eld and nurupo May 29, 2017

@iphydf

This comment has been minimized.

Show comment
Hide comment
@iphydf

iphydf Jul 9, 2017

Member

@cebe can you take a look at this?

Member

iphydf commented Jul 9, 2017

@cebe can you take a look at this?

@cebe

This comment has been minimized.

Show comment
Hide comment
@cebe

cebe Jul 10, 2017

Member

@iphydf on a coarse overview it looks okay to me. But I have no idea how pkgconfig works and also have not tested anything of this. Just looked at the changes in the diff.


Reviewed 6 of 11 files at r1, 2 of 3 files at r2.
Review status: 8 of 9 files reviewed at latest revision, 1 unresolved discussion.


CMakeLists.txt, line 386 at r2 (raw file):

find_program(SPECTEST NAMES
  tox-spectest
  ../.stack-work/install/x86_64-linux/lts-2.22/7.8.4/bin/tox-spectest)

what is this? looks super specific to a certain environment.


Comments from Reviewable

Member

cebe commented Jul 10, 2017

@iphydf on a coarse overview it looks okay to me. But I have no idea how pkgconfig works and also have not tested anything of this. Just looked at the changes in the diff.


Reviewed 6 of 11 files at r1, 2 of 3 files at r2.
Review status: 8 of 9 files reviewed at latest revision, 1 unresolved discussion.


CMakeLists.txt, line 386 at r2 (raw file):

find_program(SPECTEST NAMES
  tox-spectest
  ../.stack-work/install/x86_64-linux/lts-2.22/7.8.4/bin/tox-spectest)

what is this? looks super specific to a certain environment.


Comments from Reviewable

@cebe cebe removed their assignment Jul 10, 2017

@zoff99

This comment has been minimized.

Show comment
Hide comment
@zoff99

zoff99 Jul 11, 2017

how do you activate this? (i use configure on CircleCI)
CI with this PR still builds all the sperate tox libs

zoff99 commented Jul 11, 2017

how do you activate this? (i use configure on CircleCI)
CI with this PR still builds all the sperate tox libs

@iphydf

This comment has been minimized.

Show comment
Hide comment
@iphydf

iphydf Oct 4, 2017

Member

@zoff99 it's always activated when building with CMake.

Member

iphydf commented Oct 4, 2017

@zoff99 it's always activated when building with CMake.

@zoff99

This comment has been minimized.

Show comment
Hide comment
@zoff99

zoff99 Oct 4, 2017

@iphydf making cmake and autotools build even more different is a bad idea.
it will break builds :-(

zoff99 commented Oct 4, 2017

@iphydf making cmake and autotools build even more different is a bad idea.
it will break builds :-(

@Diadlo

This comment has been minimized.

Show comment
Hide comment
@Diadlo

Diadlo Oct 5, 2017

@iphydf What if someone wants to link his app without toxav, and simultaneously link with toxav for another app, for example?

Diadlo commented Oct 5, 2017

@iphydf What if someone wants to link his app without toxav, and simultaneously link with toxav for another app, for example?

@iphydf

This comment has been minimized.

Show comment
Hide comment
@iphydf

iphydf Nov 20, 2017

Member

@Diadlo do you or someone you know have that use case? Otherwise, YAGNI.

Member

iphydf commented Nov 20, 2017

@Diadlo do you or someone you know have that use case? Otherwise, YAGNI.

@iphydf

This comment has been minimized.

Show comment
Hide comment
@iphydf

iphydf Nov 20, 2017

Member

@zoff99 I agree. So after this we should make autotools do the same.

Member

iphydf commented Nov 20, 2017

@zoff99 I agree. So after this we should make autotools do the same.

@Diadlo

This comment has been minimized.

Show comment
Hide comment
@Diadlo

Diadlo Nov 20, 2017

@iphydf It's just like interface segregation. I just don't understand, why you want to merge all function of public API in one library, if they logically different

Diadlo commented Nov 20, 2017

@iphydf It's just like interface segregation. I just don't understand, why you want to merge all function of public API in one library, if they logically different

@iphydf

This comment has been minimized.

Show comment
Hide comment
@iphydf

iphydf Dec 28, 2017

Member

@Diadlo I'll happily split it out again if and when toxav does not depend on toxcore internals.

The alternative is to have libraries split along the layer boundaries, so we end up having the many-small-libs situation, which we had before. That has different pros and cons. I hope toxav will eventually become independent of toxcore internals (@mannol has plans for this). At that point, I'd like toxav to be a separate repository, developed independently. In the meantime, the split is headaches for developers for no reason.

Member

iphydf commented Dec 28, 2017

@Diadlo I'll happily split it out again if and when toxav does not depend on toxcore internals.

The alternative is to have libraries split along the layer boundaries, so we end up having the many-small-libs situation, which we had before. That has different pros and cons. I hope toxav will eventually become independent of toxcore internals (@mannol has plans for this). At that point, I'd like toxav to be a separate repository, developed independently. In the meantime, the split is headaches for developers for no reason.

@iphydf

This comment has been minimized.

Show comment
Hide comment
@iphydf

iphydf Dec 28, 2017

Member

Oh, and why that is an alternative: because then toxav can correctly depend on toxnetcrypto and other things it uses. Right now, toxav is lying because it says it only depend on toxcore, while actually it depends on its internals. If we expose the internals correctly (through libraries), we can have everything separated.

Member

iphydf commented Dec 28, 2017

Oh, and why that is an alternative: because then toxav can correctly depend on toxnetcrypto and other things it uses. Right now, toxav is lying because it says it only depend on toxcore, while actually it depends on its internals. If we expose the internals correctly (through libraries), we can have everything separated.

@robinlinden

This comment has been minimized.

Show comment
Hide comment
@robinlinden

robinlinden Dec 28, 2017

Member

Reviewed 1 of 11 files at r1, 2 of 3 files at r2, 3 of 3 files at r3.
Review status: all files reviewed at latest revision, 6 unresolved discussions, all commit checks successful.


CMakeLists.txt, line 323 at r3 (raw file):

    toxav/video.h)
  target_link_modules(toxav toxgroup ${OPUS_LIBRARIES} ${VPX_LIBRARIES})
  #message("*** toxav_LINK_MODULES: ${toxav_LINK_MODULES}")

This should probably be removed.


CMakeLists.txt, line 359 at r3 (raw file):

# Link it to all dependencies.
foreach(sublib ${toxcore_SUBLIBS})
  #message("*** toxcore_LINK_MODULES += ${${sublib}_LINK_MODULES}  # ${sublib}_LINK_MODULES")

This should probably be removed.


cmake/ModulePackage.cmake, line 154 at r3 (raw file):

  foreach(dep ${ARGN})
    #message("*** Processing dep ${dep} for ${target}")

This should probably be removed.


cmake/ModulePackage.cmake, line 165 at r3 (raw file):

        list(FIND LINK_MODULES ${dep} _index)
        if(_index EQUAL -1)
          #message("*** Adding ${dep} to ${target}_LINK_MODULES.")

This should probably be removed.


cmake/StrictAbi.cmake, line 36 at r3 (raw file):

      OUTPUT_STRIP_TRAILING_WHITESPACE)
    string(REPLACE "\n" ";" sublib_SYMS ${sublib_SYMS})
  

Stray whitespace.


Comments from Reviewable

Member

robinlinden commented Dec 28, 2017

Reviewed 1 of 11 files at r1, 2 of 3 files at r2, 3 of 3 files at r3.
Review status: all files reviewed at latest revision, 6 unresolved discussions, all commit checks successful.


CMakeLists.txt, line 323 at r3 (raw file):

    toxav/video.h)
  target_link_modules(toxav toxgroup ${OPUS_LIBRARIES} ${VPX_LIBRARIES})
  #message("*** toxav_LINK_MODULES: ${toxav_LINK_MODULES}")

This should probably be removed.


CMakeLists.txt, line 359 at r3 (raw file):

# Link it to all dependencies.
foreach(sublib ${toxcore_SUBLIBS})
  #message("*** toxcore_LINK_MODULES += ${${sublib}_LINK_MODULES}  # ${sublib}_LINK_MODULES")

This should probably be removed.


cmake/ModulePackage.cmake, line 154 at r3 (raw file):

  foreach(dep ${ARGN})
    #message("*** Processing dep ${dep} for ${target}")

This should probably be removed.


cmake/ModulePackage.cmake, line 165 at r3 (raw file):

        list(FIND LINK_MODULES ${dep} _index)
        if(_index EQUAL -1)
          #message("*** Adding ${dep} to ${target}_LINK_MODULES.")

This should probably be removed.


cmake/StrictAbi.cmake, line 36 at r3 (raw file):

      OUTPUT_STRIP_TRAILING_WHITESPACE)
    string(REPLACE "\n" ";" sublib_SYMS ${sublib_SYMS})
  

Stray whitespace.


Comments from Reviewable

Generate only one large library "libtoxcore".
This library contains all the code for the old libtoxcore, libtoxav,
libtoxdns, and libtoxencryptsave. The build for toxav is still optional,
and disabling it causes libtoxcore to simply not contain those symbols
and the pkg-config file to not include opus and vpx as dependencies.
@iphydf

This comment has been minimized.

Show comment
Hide comment
@iphydf

iphydf Dec 28, 2017

Member

Review status: all files reviewed at latest revision, 6 unresolved discussions, all commit checks successful.


CMakeLists.txt, line 386 at r2 (raw file):

Previously, cebe (Carsten Brandt) wrote…

what is this? looks super specific to a certain environment.

It's specific to toktok-stack, which is available to everyone (it's a github repo).


CMakeLists.txt, line 323 at r3 (raw file):

Previously, robinlinden (Robin Lindén) wrote…

This should probably be removed.

Done.


CMakeLists.txt, line 359 at r3 (raw file):

Previously, robinlinden (Robin Lindén) wrote…

This should probably be removed.

Done.


cmake/ModulePackage.cmake, line 154 at r3 (raw file):

Previously, robinlinden (Robin Lindén) wrote…

This should probably be removed.

Done.


cmake/ModulePackage.cmake, line 165 at r3 (raw file):

Previously, robinlinden (Robin Lindén) wrote…

This should probably be removed.

Done.


cmake/StrictAbi.cmake, line 36 at r3 (raw file):

Previously, robinlinden (Robin Lindén) wrote…

Stray whitespace.

Done.


Comments from Reviewable

Member

iphydf commented Dec 28, 2017

Review status: all files reviewed at latest revision, 6 unresolved discussions, all commit checks successful.


CMakeLists.txt, line 386 at r2 (raw file):

Previously, cebe (Carsten Brandt) wrote…

what is this? looks super specific to a certain environment.

It's specific to toktok-stack, which is available to everyone (it's a github repo).


CMakeLists.txt, line 323 at r3 (raw file):

Previously, robinlinden (Robin Lindén) wrote…

This should probably be removed.

Done.


CMakeLists.txt, line 359 at r3 (raw file):

Previously, robinlinden (Robin Lindén) wrote…

This should probably be removed.

Done.


cmake/ModulePackage.cmake, line 154 at r3 (raw file):

Previously, robinlinden (Robin Lindén) wrote…

This should probably be removed.

Done.


cmake/ModulePackage.cmake, line 165 at r3 (raw file):

Previously, robinlinden (Robin Lindén) wrote…

This should probably be removed.

Done.


cmake/StrictAbi.cmake, line 36 at r3 (raw file):

Previously, robinlinden (Robin Lindén) wrote…

Stray whitespace.

Done.


Comments from Reviewable

@robinlinden

This comment has been minimized.

Show comment
Hide comment
@robinlinden

robinlinden Dec 28, 2017

Member

:lgtm_strong:


Reviewed 3 of 3 files at r4.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

Member

robinlinden commented Dec 28, 2017

:lgtm_strong:


Reviewed 3 of 3 files at r4.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@iphydf iphydf merged commit f2b6090 into TokTok:master Dec 29, 2017

5 checks passed

ci/circleci Your tests passed on CircleCI!
Details
code-review/reviewable 1/1 LGTMs
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@iphydf iphydf deleted the iphydf:one-big-lib branch Dec 29, 2017

@iphydf iphydf modified the milestones: v0.2.0-RC1, v0.2.0 Jan 14, 2018

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