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

How is header file name with sub-folder name supposed to work together? #14

Open
ghost opened this issue May 24, 2023 · 14 comments
Open

Comments

@ghost
Copy link

ghost commented May 24, 2023

Dear team,

I have the following folder structure:

.
└── dir
    ├── dir1
    │   └── file1.cpp
    └── dir2
        └── collision.hpp

the content of file1.cpp is:

#include <dir3/collision.hpp>

dir3/collision.hpp does not exist in the above demo folder
dir2/collision.hpp is empty.

source: https://github.com/fracting/cpp_dependency_graph_minimal

When I run cpp_dependency_graph visualise_project --root_dir ./dir I got the following chart:

deps

This is a surprise to me because I expect dir1 not to depend on dir2.

Could you kindly clarify if this is indeed a bug, or if there is a specific parameter adjustment that would ensure dir1 does not appear to depend on dir2 in the resulting dependency chart?

My actual use case is more complex than the provided example, which I've simplified for clarity and ease of discussion.

Any advice is appreciated, thanks!

@ghost
Copy link
Author

ghost commented Jun 5, 2023

Hi @shreyasbharath, any comments?

@shreyasbharath
Copy link
Owner

Hey @fracting , apologies for the late reply.

I haven't looked at this in a couple of years, I'll need to dig in a bit to figure out what the issue is. Let me look into it tomorrow if possible.

@ghost
Copy link
Author

ghost commented Jun 6, 2023

Hey @fracting , apologies for the late reply.

I haven't looked at this in a couple of years, I'll need to dig in a bit to figure out what the issue is. Let me look into it tomorrow if possible.

Thanks a lot for response! If you have a crypto wallet, I would be glad to send a one time gift unconditionally to express my appreciation on your past work on this project!

@shreyasbharath
Copy link
Owner

Ah no, I didn't start this for money. I'll take a look tomorrow, I have some spare time this weekend.

@shreyasbharath
Copy link
Owner

So I've had a quick look and this is not a bug per se, the gem assumes that your project enforces unique filenames.

Is this not the case for your project?

@ghost
Copy link
Author

ghost commented Jun 12, 2023

So I've had a quick look and this is not a bug per se, the gem assumes that your project enforces unique filenames.

Is this not the case for your project?

Hi, thanks a lot for the help! No in my project there is no guarantee of uniqueness of file names, and I have no way to change that because it’s actually a third party open source project that I want to analyse and contribute to. Do you think it’s hard to modify cpp-dependency-graph to take folders into consideration? I haven’t written anything in ruby but it you can give me some hints I might try. Alternatively, if you know any similar tools that take folders into consideration could you please recommend?

Thanks!

@shreyasbharath
Copy link
Owner

Okay let me have a go at fixing this. My coding skills have gotten rusty over the past few months so it's a good opportunity for me to flex my coding muscle 😁

@shreyasbharath
Copy link
Owner

Can you give this #15 a go and let me know if it fixes your issue?

It's a bit hacky but I can tidy it up before merging.

@ghost
Copy link
Author

ghost commented Jun 12, 2023

Can you give this #15 a go and let me know if it fixes your issue?

It's a bit hacky but I can tidy it up before merging.

Thanks a lot! I'm testing, will feedback in 10 minutes.

@ghost
Copy link
Author

ghost commented Jun 12, 2023

I might miss something, I rerun cpp_dependency_graph visualise_project --root_dir ./dir but still got the same dependency chart where dir1 points to dir2. I'm double checking if I did anything wrong (wrong branch, wrong version, wrong environment, etc) and will report back soon.

Sorry, my environment was broken, I'm fixing it, first time building a ruby project from source, will update soon!

@ghost
Copy link
Author

ghost commented Jun 12, 2023

Update: after fixing the build environment, I can confirm the WIP branch works for my minimal example project (https://github.com/fracting/cpp_dependency_graph_minimal)

I'm now going to test it with the real project ( https://github.com/oxen-io/lokinet ) and will report back later.

Thanks!

@ghost
Copy link
Author

ghost commented Jun 12, 2023

@shreyasbharath

I tested the WIP cpp_dependency_graph against https://github.com/oxen-io/lokinet, previous there were a lot of cyclic dependencies

Now with the WIP fix there are no cyclic dependencies at all.

I'm not sure if it is really the case that lokinet does not have cyclic dependencies at all, or cpp_dependency_graph from the WIP branch fails to detect any cyclic dependencies, I'll need to manually check some files and report back later.

@ghost
Copy link
Author

ghost commented Jun 12, 2023

It seems there are some cyclic dependencies missed by the WIP cpp_dependency_graph. For example:

From https://github.com/oxen-io/lokinet/blob/e11f5018c241ce988104d539cb4c0e962c3d44ed/llarp/service/context.cpp#L4
We have:

#include <llarp/handlers/tun.hpp>

At the same time, from https://github.com/oxen-io/lokinet/blob/e11f5018c241ce988104d539cb4c0e962c3d44ed/llarp/handlers/tun.cpp#L16
We also have:

#include <llarp/service/context.hpp>

However, the WIP cpp_dependency_graph doesn't report any dependency between handlers and service when I run the following command line from the root directory of the project:

cpp_dependency_graph visualise_cyclic_deps --root_dir ./

(
Alternative, I also tried

cpp_dependency_graph visualise_cyclic_deps --root_dir lokinet

from the parent directory of the lokinet source repository and got the same result.
)

@shreyasbharath
Copy link
Owner

Hey @fracting apologies again, been a crazy few weeks! I can take a look next weekend 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant