Project

General

Profile

Actions

Task #3084

closed

Rename "src" folder to "ndn-cxx"

Added by Alex Afanasyev over 8 years ago. Updated over 5 years ago.

Status:
Closed
Priority:
Normal
Category:
Build
Target version:
Start date:
Due date:
% Done:

100%

Estimated time:

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.


Related issues 2 (0 open2 closed)

Blocks ndn-cxx - Bug #4041: Doxygen generates incomplete include pathsClosedDavide Pesavento

Actions
Blocks ndn-cxx - Task #4782: Install implementation detail headersClosedJunxiao Shi12/06/2018

Actions
Actions #1

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.

Actions #2

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).

Actions #3

Updated by Junxiao Shi over 8 years ago

Answer to note-2:

Makefile can invoke any shell command, including find ... -execdir cp ...

Actions #4

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.

Actions #5

Updated by Junxiao Shi over 8 years ago

  • Category set to Build

Is there any negative impact on ndn-cxx itself?

Actions #6

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.

Actions #7

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.

Actions #8

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.

Actions #9

Updated by Davide Pesavento over 5 years ago

  • Blocks Bug #4041: Doxygen generates incomplete include paths added
Actions #10

Updated by Davide Pesavento over 5 years ago

  • Status changed from In Progress to Code review
  • % Done changed from 0 to 50
Actions #11

Updated by Ashlesh Gawande over 5 years ago

Should other libraries (ChronoSync and PSync) follow suit?

Actions #12

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.

Actions #13

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.

Actions #14

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 tests are 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.

Actions #15

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.

Actions #16

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.

Actions #17

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.

Actions #18

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.
Actions #19

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.
Actions #20

Updated by Junxiao Shi over 5 years ago

  • Blocks Task #4782: Install implementation detail headers added
Actions #21

Updated by Davide Pesavento over 5 years ago

  • Status changed from Code review to Closed
  • % Done changed from 50 to 100
Actions

Also available in: Atom PDF