Task #3084
closed
Rename "src" folder to "ndn-cxx"
Added by Alex Afanasyev over 8 years ago. Updated over 5 years ago.
100%
Description
There are two use cases (NFD-android and ndnSIM) when I would like to embed ndn-cxx library as a submodule to the project.
To make everything work without installing the library anywhere and without requiring to change include directives, there should ndn-cxx folder as applications use #include <ndn-cxx/...> statements.
The easiest way out is to simply rename src folder to ndn-cxx. Alternative is to move header files to ndn-cxx or include/ndn-cxx, but I really don't like this, as it spread out code into different/sparse locations, complicating maintenance and debugging.
With NFD-android the current workaround is to use a symlink. This works great on *nix-based platforms, but fails miserably when compiling (cross-compiling) on windows.
Updated by Junxiao Shi over 8 years ago
This is unnecessary with the two use cases.
A solution is: tweak wscript of ndnSIM and NFD-Android to copy ndn-cxx/src/**/*.hpp to build/ndn-cxx/**/*.hpp.
Updated by Alex Afanasyev over 8 years ago
This can work for ndnSIM, but it doesn't immediately work for NFD-android. NFD-android uses a makefile to do the job (my attempts to do something like this failed in the past, though I'm not an expert in makefiles).
Updated by Junxiao Shi over 8 years ago
Answer to note-2:
Makefile can invoke any shell command, including find ... -execdir cp ...
Updated by Alex Afanasyev over 8 years ago
The issue with android'd makefile is that there are no explicit targets for which I can define dependency. You can check Android.mk script for example.
Updated by Junxiao Shi over 8 years ago
- Category set to Build
Is there any negative impact on ndn-cxx itself?
Updated by Alex Afanasyev over 8 years ago
I cannot think of any negative impact, except potential (initial) confusion about the folder name where the source code is located.
Updated by Alex Afanasyev over 8 years ago
- Status changed from New to Abandoned
In any case. I will abandon this issue for now, as the issue was resolved within ndnSIM. I will try to find a workaround for NFD-anrdoid (only windows platform has problem right now, as it lacks support of symlinks**).
** Not really true, as windows/ntfs has some support for links, but it is different from UNIX symlinks and require admin privileges.
Updated by Davide Pesavento over 5 years ago
- Tracker changed from Feature to Task
- Status changed from Abandoned to In Progress
- Assignee set to Davide Pesavento
- Target version set to v0.7
Reopening. This is a common best-practice for C/C++ libraries and we should definitely follow it. In addition to making things simpler for ndnSIM and NFD-Android as mentioned above, renaming the directory to "ndn-cxx" also solves #4041 and fixes a long-standing weirdness in the examples where we currently have to use different #include paths from what users would use if they installed ndn-cxx on their system.
Updated by Davide Pesavento over 5 years ago
- Blocks Bug #4041: Doxygen generates incomplete include paths added
Updated by Davide Pesavento over 5 years ago
- Status changed from In Progress to Code review
- % Done changed from 0 to 50
Updated by Ashlesh Gawande over 5 years ago
Should other libraries (ChronoSync and PSync) follow suit?
Updated by Davide Pesavento over 5 years ago
It's up to their respective maintainers. I recommend doing the same in PSync, on the other hand ChronoSync can probably be left alone since it's somewhat "legacy". At the end of the day it all depends on whether someone wants to do the work.
Updated by Junxiao Shi over 5 years ago
https://gerrit.named-data.net/5046 is changing #include style.
I feel it’s going too far. <ndn-cxx/header.hpp> is fine, because that’s how a dependent project would include ndn-cxx header.
<tests/header.hpp> shouldn’t use angle-brackets. Traditionally angle-brackets are only for headers in another project, and quotes are for headers in same project. Headers under tests are not installed so it’s unreasonable to use angle-brackets.
Updated by Davide Pesavento over 5 years ago
Junxiao Shi wrote:
Traditionally
define "traditionally".
angle-brackets are only for headers in another project, and quotes are for headers in same project.
says who?
Headers under
testsare not installed so it’s unreasonable to use angle-brackets.
Explain why you think it's "unreasonable". You haven't really given any concrete reason.
Updated by Junxiao Shi over 5 years ago
Having almost everything in angle brackets makes it difficult to tell which headers are internal and which are external. I strongly disagree with this change.
<ndn-cxx/header.hpp> is acceptable. Others are not.
Updated by Davide Pesavento over 5 years ago
How do you distinguish external vs. internal headers? Are you saying that we should use quotes for detail headers, since they are not installed? Are those considered "internal"?
FTR, the vast majority of #include directives in Boost use angle brackets (~64700 using brackets, of which ~14000 for detail headers, vs. ~900 using quotes, of which ~100 for detail headers).
This is not to say that I agree or disagree with you, it's just to point out how arbitrary this distinction is.
Updated by Junxiao Shi over 5 years ago
External: everything not from the repository.
Internal: everything in the same repository.
<ndn-cxx/header.hpp> for installed headers is internal, but I could accept this as an exception.
Others are unacceptable.
Updated by Junxiao Shi over 5 years ago
20181126 call discussed note-13 issue. Conclusion:
#include "ndn-cxx/header.hpp"and#include "tests/header.hpp"for internal headers;#include <boost/header.hpp>for external headers.- internal means everything in the same repository, including submodules.
Also:
#include "../header.hpp"is forbidden. To go up the directory tree, the include line must start from project root.#include "header.hpp"(for header in current directory) and#include "dir/header.hpp"(for header in subdirectory) are permitted.
Updated by Ernest McCracken over 5 years ago
Junxiao Shi wrote:
20181126 call discussed note-13 issue. Conclusion:
#include "ndn-cxx/header.hpp"and#include "tests/header.hpp"for internal headers;#include <boost/header.hpp>for external headers.- internal means everything in the same repository, including submodules.
Also:
#include "../header.hpp"is forbidden. To go up the directory tree, the include line must start from project root.#include "header.hpp"(for header in current directory) and#include "dir/header.hpp"(for header in subdirectory) are permitted.
As per the call I agree with these rules. Both the root level relative path requirement and project external brackets will help new contributors navigate the project as well as determine which headers are project external dependencies.
Can https://gerrit.named-data.net/5046 proceed forward as is? Else a patch to change any patterns for <tests/header.hpp> to quotation syntax.
Updated by Junxiao Shi over 5 years ago
- Blocks Task #4782: Install implementation detail headers added
Updated by Davide Pesavento over 5 years ago
- Status changed from Code review to Closed
- % Done changed from 50 to 100