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

refactor: Install headers into repo-equivalent directories. #2599

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

iphydf
Copy link
Member

@iphydf iphydf commented Jan 25, 2024

This allows code to be built from an un-installed toxcore repo the same way it's built from an installed toxcore. I.e. all #include directives will work the same inside the toxcore repo and on a system which has toxcore installed.

Previously, "toxcore/tox.h" was installed as "tox/tox.h", making make install a requirement for building any client code. Now, clients can be built directly from the repo source, which is especially nice for simple clients like echobots.

Output:

-- Installing: /usr/local/include/tox/tox.h
-- Installing: /usr/local/include/toxcore/tox.h
-- Installing: /usr/local/include/toxcore/tox_events.h
-- Installing: /usr/local/include/toxcore/tox_dispatch.h
-- Installing: /usr/local/include/toxcore/tox_private.h
-- Installing: /usr/local/include/tox/toxav.h
-- Installing: /usr/local/include/toxav/toxav.h
-- Installing: /usr/local/include/tox/toxencryptsave.h
-- Installing: /usr/local/include/toxencryptsave/toxencryptsave.h

This change is Reviewable

@iphydf iphydf added this to the v0.2.19 milestone Jan 25, 2024
@iphydf iphydf marked this pull request as ready for review January 25, 2024 14:52
@iphydf iphydf force-pushed the include-install branch 3 times, most recently from 0c2dcc9 to 81dcc95 Compare January 25, 2024 15:00
Copy link

codecov bot commented Jan 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.13%. Comparing base (ed2b60c) to head (1a98760).
Report is 53 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2599      +/-   ##
==========================================
+ Coverage   73.06%   73.13%   +0.07%     
==========================================
  Files         149      149              
  Lines       30517    30517              
==========================================
+ Hits        22297    22319      +22     
+ Misses       8220     8198      -22     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nurupo
Copy link
Member

nurupo commented Jan 25, 2024

Previously, "toxcore/tox.h" was installed as "tox/tox.h", making make install a requirement for building any client code. Now, clients can be built directly from the repo source, which is especially nice for simple clients like echobots.

  1. Why exactly do we want to avoid calling make install on toxcore to build clients? Is it that troublesome to have toxcore installed before building a client?
    1. Even simple clients like echobot would still require that libsodium is installed, no? (or can libsodium be included as source too?)
  2. Do we really want to commit to supporting clients building directly off the toxcore source code in addition to the installed prefix (binaries + include files)?
    1. Doesn't that restrict us from adding some logic to our build system that modifies the code at build time, for example template substitution?
  3. Isn't it better to change the repository's file structure around instead of changing the /usr/local/include structure, which will affect (break) all downstream projects?
    1. I see that in this PR you have created tox/{tox.h, toxav.h, toxencryptsave.h} in the repository, which contain just the #include {toxcore/tox.h, toxav/toxav.h, toxencryptsave/toxencryptsave.h} line -- isn't that already enough for what you are trying to achieve, without having to change the /usr/local/include structure?
    2. Some projects, e.g. VLC, have include/ in their repository for files that define the public API. This is just one of the possible ways to re-arrange the files within the repository, but what do you think about it?

Feels a bit weird that a single libtoxcore library / the c-toxcore project, uses three separate /usr/local/include/{toxcore,toxav,toxencryptsave} namespaces, instead of using just one and subdirectoring it. This doesn't seem to be a common thing to do. Are there some precedents of other projects doing this?

@nurupo nurupo closed this Jan 25, 2024
@nurupo
Copy link
Member

nurupo commented Jan 25, 2024

Sorry, misclicked, didn't mean to close.

@nurupo nurupo reopened this Jan 25, 2024
@iphydf iphydf force-pushed the include-install branch 3 times, most recently from 74eaa35 to 877f118 Compare January 31, 2024 17:13
@iphydf
Copy link
Member Author

iphydf commented Jan 31, 2024

Previously, "toxcore/tox.h" was installed as "tox/tox.h", making make install a requirement for building any client code. Now, clients can be built directly from the repo source, which is especially nice for simple clients like echobots.

  1. Why exactly do we want to avoid calling make install on toxcore to build clients? Is it that troublesome to have toxcore installed before building a client?

Yes, it's quite troublesome when developing toxcore together with a client. Iterating on both client and toxcore development is slowed down quite a bit by having to make install each time. I don't suffer from this myself, because I develop mostly using bazel.

  1. Even simple clients like echobot would still require that libsodium is installed, no? (or can libsodium be included as source too?)

libsodium isn't co-developed with clients, so the above is less of an issue. libsodium also generally changes less, so installing it once is less overhead than installing toxcore many times as it's in active development.

  1. Do we really want to commit to supporting clients building directly off the toxcore source code in addition to the installed prefix (binaries + include files)?

Yes. Until it's too much of a burden.

  1. Doesn't that restrict us from adding some logic to our build system that modifies the code at build time, for example template substitution?

Yes, and that's quite intentional. We should stay away from build system integrated codegen as much as possible. Generating files and committing them to git is ok, we already do that for events, and we may do more of that as part of semfmt, but we should always in principle own all our code.

  1. Isn't it better to change the repository's file structure around instead of changing the /usr/local/include structure, which will affect (break) all downstream projects?

Why is it better? This PR does not break any downstream projects.

  1. I see that in this PR you have created tox/{tox.h, toxav.h, toxencryptsave.h} in the repository, which contain just the #include {toxcore/tox.h, toxav/toxav.h, toxencryptsave/toxencryptsave.h} line -- isn't that already enough for what you are trying to achieve, without having to change the /usr/local/include structure?

It's not quite enough, because one other things that this change enables is that toxav could include "../toxcore/tox.h" instead of re-declaring struct Tox with the ifndef thing around it (which is trouble for many reasons, not the least C++ modules and apigen).

  1. Some projects, e.g. VLC, have include/ in their repository for files that define the public API. This is just one of the possible ways to re-arrange the files within the repository, but what do you think about it?

That's possible, but right now you can simply do tcc my_code.c $(find tox* -name "*.c") third_party/cmp/cmp.c) and have a working binary. That's really quite nice, and something that @irungentoo has always been doing and has made quite sure to always continue supporting (this is also why we committed apidsl outputs instead of build-system-ing them).

Feels a bit weird that a single libtoxcore library / the c-toxcore project, uses three separate /usr/local/include/{toxcore,toxav,toxencryptsave} namespaces, instead of using just one and subdirectoring it.

The 3 libraries are really quite separate libraries and should become even more separate. We're building a single .so for convenience (used to build multiple), but I can see a future in which toxav is maintained in a separate repo, build, install, etc (not necessarily in practice, but we should make it theoretically possible, so design decisions should move us towards that being possible).

This doesn't seem to be a common thing to do. Are there some precedents of other projects doing this?

Probably. I can't think of any right now, but I'm also not very interested in following precedents, and rather make logical decisions based on goals and requirements.

@iphydf iphydf force-pushed the include-install branch 5 times, most recently from d3ef35c to 2b2ffba Compare February 2, 2024 23:52
@iphydf iphydf modified the milestones: v0.2.19, v0.2.20 Feb 11, 2024
iphydf added a commit to iphydf/c-toxcore that referenced this pull request Feb 18, 2024
Unfortunately, events and dispatch are already installed in .18, so we
can't stop, but for .19, let's not add this private header if there's
the possibility of moving it to a different location
(TokTok#2599). Installing it makes it
a chore for package maintainers (debian, pkgsrc).
iphydf added a commit to iphydf/c-toxcore that referenced this pull request Feb 18, 2024
Unfortunately, events and dispatch are already installed in .18, so we
can't stop, but for .19, let's not add this private header if there's
the possibility of moving it to a different location
(TokTok#2599). Installing it makes it
a chore for package maintainers (debian, pkgsrc).
@zoff99
Copy link

zoff99 commented Feb 25, 2024

@iphydf i think the autotools config was not updated to reflect the changes

@iphydf
Copy link
Member Author

iphydf commented Feb 25, 2024

@iphydf i think the autotools config was not updated to reflect the changes

Yes that was intentional. Do you use the autotools build? I was keeping it around for unmaintained clients, so I didn't update it so there are fewer changes to those builds.

@iphydf iphydf modified the milestones: v0.2.20, v0.2.21 Nov 6, 2024
This allows code to be built from an un-installed toxcore repo the same
way it's built from an installed toxcore. I.e. all `#include` directives
will work the same inside the toxcore repo and on a system which has
toxcore installed.

Previously, "toxcore/tox.h" was installed as "tox/tox.h", making `make
install` a requirement for building any client code. Now, clients can be
built directly from the repo source, which is especially nice for simple
clients like echobots.

Output:
```
-- Installing: /usr/local/include/tox/tox.h
-- Installing: /usr/local/include/toxcore/tox.h
-- Installing: /usr/local/include/toxcore/tox_events.h
-- Installing: /usr/local/include/toxcore/tox_dispatch.h
-- Installing: /usr/local/include/toxcore/tox_private.h
-- Installing: /usr/local/include/tox/toxav.h
-- Installing: /usr/local/include/toxav/toxav.h
-- Installing: /usr/local/include/tox/toxencryptsave.h
-- Installing: /usr/local/include/toxencryptsave/toxencryptsave.h
```
@github-actions github-actions bot added the refactor Refactoring production code, eg. renaming a variable, not affecting semantics label Nov 8, 2024
@iphydf iphydf modified the milestones: v0.2.21, v0.2.x Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Refactoring production code, eg. renaming a variable, not affecting semantics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants