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

Migrate LightPcapNg to the forked repo #1348

Open
tigercosmos opened this issue Mar 26, 2024 · 9 comments
Open

Migrate LightPcapNg to the forked repo #1348

tigercosmos opened this issue Mar 26, 2024 · 9 comments

Comments

@tigercosmos
Copy link
Collaborator

tigercosmos commented Mar 26, 2024

Currently we copy LightPcapNg code directly in seladb/PcapPlusPlus

The code lives at here: PcapPlusPlus/3rdParty/LightPcapNg

We now use the current_commit.git file to record the upstream git commit, but this way is difficult to sync the code from the upstream (basically, manual picking).

Therefore we created a forked LightPcapNg for esiast maintenance.

Here are the steps:

  1. Migrate the current code to https://github.com/PcapPlusPlus/LightPcapNg (Manually picking and copying)
  2. Remove the current copied code of LightPcapNg.
  3. Use CMake ExternalProject_Add feature to download the source code from PcapPlusPlus/LightPcapNg when doing the first run of cmake.
  4. Try to push our change in PcapPlusPlus/LightPcapNg back to the upstream
@seladb
Copy link
Owner

seladb commented Mar 26, 2024

@tigercosmos I don't think we should use git submodule. Unfortunately submodules are not very convenient because the user needs to remember to fetch them (a simple git clone won't fetch them by default).
I think we can keep what we do now, just sync the git commit against our fork instead of the upstream repo

@tigercosmos
Copy link
Collaborator Author

@seladb I think we can just write in the document to use git clone --recurse-submodules?

I think we can keep what we do now, just sync the git commit against our fork instead of the upstream repo

Do you mean to copy the code from the forked repo? I think it doesn't make the maintaining sync issue easier.

@seladb
Copy link
Owner

seladb commented Mar 26, 2024

@seladb I think we can just write in the document to use git clone --recurse-submodules?

I think we can keep what we do now, just sync the git commit against our fork instead of the upstream repo

Do you mean to copy the code from the forked repo? I think it doesn't make the maintaining sync issue easier.

yes... that's the reason I copied the code in the first place. Enforcing doing git clone --recurse-submodules is pretty annoying. Please notice we do that for all 3rd party libs. Since it's a forked repo that we don't change a lot, it shouldn't be very hard to maintain our copied code the same as the fork...

@tigercosmos
Copy link
Collaborator Author

I disagree with duplicating the code. If we opt to increment the version in the forked repository, it would result in copying all the code into PCPP. This approach seems inefficient. Instead, why not simply increment the commit in the git submodule?

Kindly permit me to invite @clementperon and @egecetin to share their opinions on this matter as well.

@egecetin
Copy link
Collaborator

egecetin commented Mar 26, 2024

To be honest submodule is not a problem for experienced developers they can easily figure out when they even configuring project with CMake and get path does not exist or similar error. But for most inexperienced people it is quite hard to figure out they missed the submodule init (Why does the git clone default do not fetch submodules, they are part of code, so I think also which is a bit annoying). If the concern is only troubles caused by missing the submodule initialization maybe we can add a warning line to CMake to remind people to call git submodule update --init --recursive (since they already cloned this command is better) if the path/file does not exist?

And a side note, source code zip downloads from GitHub also do not contain submodules

@seladb
Copy link
Owner

seladb commented Mar 26, 2024

PcapPlusPlus is often used by inexperienced developers and one of the things I always cared about is making it as easy to use as possible. I'm a strong believer in ease of use because that's what I expect when getting into a new library or project.

So even if it makes our lives (maintainers) a bit harder, it's worth the ease of use for the library's users.

If I didn't care about that I'd make it a submodule in the first place, that's the option that makes the most sense from a library maintainer point-of-view.

@tigercosmos
Copy link
Collaborator Author

tigercosmos commented Mar 26, 2024

@seladb how about this way, we don't copy the forked code into PCPP nor use the git submodule.

When the user runs cmake for the first time in PCPP, cmake will fetch and get the code of LightPcapNg from the forked repo.

For example:

include(ExternalProject)

# Define the external project
ExternalProject_Add(
    MyProjectExternal    # Arbitrary name for the project
    PREFIX ${CMAKE_BINARY_DIR}/MyProject    # Directory where the project will be downloaded and built
    GIT_REPOSITORY https://github.com/username/repo.git    # URL of the Git repository
    GIT_TAG master    # Branch, tag, or commit hash to checkout
    UPDATE_DISCONNECTED 1    # Don't update the project during the build
    # You can also specify other options such as CONFIGURE_COMMAND, BUILD_COMMAND, INSTALL_COMMAND, etc.
)

# Add dependencies of your main project to the external project
add_dependencies(MyMainProject MyProjectExternal)

@seladb
Copy link
Owner

seladb commented Mar 27, 2024

ExternalProject_Add

Hmm I'm open to this idea! Can you please create a POC of this to see how it works?

@tigercosmos
Copy link
Collaborator Author

@seladb sure, revised the issue content.

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

No branches or pull requests

3 participants