Format all files to current ns-3 .clang-format rules

[non-det-alle]

Apply ns-3 .clang-format rules to all C++ files of the module. No additional changes beyond formatting.

This merge is meant to break up a previous huge pull request into more digestible pieces.

[pagmatt]

Ideally, I would opt for merging this last, and first merge the commits that introduce the support for the latest version of ns-3. In this way, I could at least check that no tests are broken and that compilation works, while now my review would be almost worthless (15K + lines of changes are obviously too many to manually go through)

[non-det-alle]

Ok, I'll do that first.

I started with this because, technically, the codestyle is an integral part of ns-3, so it would have been nice to bring it up to date before bumping the ns-3 version.

On the topic of tests, I have a future MR planned that also adds "running the examples" to the tasks executed by ./test.py for this module. This improves the test coverage, that is currently not ideal.

[albertogara]

As previously mentioned in the mailing list. I suggest doing this first. Otherwise, it will be a nightmare to set this at the very end, especially since this "last" merge can be very well in a few months, and coding style rules are constantly evolving in ns-3-dev.

If you set a remote repository forked from ns-3-dev gitlab master you can make use of the ns-3-dev gitlab pipeline (I don't know if there is such option fo ther github repository, maybe there is). Using this pipeline will check for any clang style mismatches and ensure that your code is up to rule not only with this MR but with any subsequent merges. Furthermore, you can constantly update your repository with the latest ns-3 changes. That is how we do it in ns-3 and makes keeping the code "updated" a painless process.

If using the ci pipeline is not an option, then yes, this MR should be the last.

[pagmatt]

Sorry all, I meant to keep this last as in simply merging the support for the latest ns-3 version first, so that we can at least start testing compilation and tests. In my opinion, this has the utmost priority, as it allows reviewers to setup at least some basic tests, and check that the coding style changes do not break those. As soon as we do that, I agree that aligning the coding style and setting up the full testing coverage of the main ns-3 would be a good move, before introducing any further changes such as improvements and extensions to the model.

[non-det-alle]

Note: already before our intervention (e8f7a21), the module was compiling and the existing tests were passing.

@pagmatt, do you suggest we add more tests before applying code formatting? Otherwise I can proceed to open a new MR with code formatting on the current repo state.

@albertogara in this repo there are 2 github ci pipelines for tests and documentation. Unfortunately they are outdated so they constantly fail. I agree that being able to use the ones already in place for ns-3 would be a nice time-saver. Ideally, Signet could open a fork of ns-3 on gitlab and then we could start working there. But for now I understand that Signet wants to keep the code here, so fixing the existing github pipelines could be a good priority as well.

[pagmatt]

What about opening a fork of ns-3-dev on Gitlab, with this repo as a submodule?

In my opinion as a next step we could update the formatting and setup an updated CI, ideally using the same tests performed in the mainline ns-3 repo. Then, we can start adding more lorawan-specific tests.

[albertogara]

@non-det-alle Yes, I asked around in ns-3 to Gabriel Ferreira who is one of the persons who handle the ci pipeline. Apparently, there is a minimalistic ci pipeline for the github repository but it only checks for compilation in some OS. It does not make the full checks the ci pipeline does in gitlab. So yes I recommend using the forked repo from gitlab.

@pagmatt Yes, in my opinion, the best option would be for you to fork from gitlab and we can start adding the submodule. I say "you" because ultimately you approve all the merges while we just participate on the sidelines :D. Also, I recommend you to maintain this new gitlab repository updated(once or twice per week) with the ns-3-dev changes. In this way, Alessandro will be always doing its MR with the bleeding edge version of ns-3-dev, and no coding style issues will arise in the future or they will be detected on each MR.

These are just suggestions, but I am ok with any way you both think is more comfortable. If you decide to go with the other repo, please share with us the link on the mailing list.

[non-det-alle]

Hello @albertogara, me and @pagmatt continued the discussion on CI here: https://github.com/signetlabdei/lorawan/pull/138 Check this comment where I raise some issues I see from my side with the ns-3 fork + submodule approach.

In the past days I've started working on creating a Github CI pipeline (also based on the one from Gabriel Ferreira already present in ns-3). I'm aiming to at least have a subset of the checks present in the Gitlab pipeline that cover building, testing, formatting and documentation in ubuntu with gcc.

You can check my current progress here.

My point is that having both option would be better (a reduced Gitlab pipeline for contributors' MRs + the full Gitlab pipeline that can be run by maintainers by committing to the ns-3 fork)

[pagmatt]

@albertogara Regarding the timing of the updates, I believe that following the mmwave and 5G NR approach makes more sense, i.e., update the module whenever a new version of ns-3 is released, or in case there are significant changes that are particularly relevant for the lorawan submodule. In the meantime, we point to the latest ns-3 release as the version of ns-3 to use the module with. Testing and fixing changes every week seems like an unnecessary overhead to me, and in fact, an approach which few (if any) external modules follow. A compromise could be to automatically test each week, and then whenever tests pass, mention that the module is compatible with the latest ns-3 commit as well. Otherwise, simiply point to the latest tested release

[albertogara]

I think updating with every ns-3 release like you mentioned is very reasonable. I just mentioned updating often because that is the approach I usually take with my external module and personally I find it less of a hassle because the changes are never that many when doing it this way. But of course, I have no issues with updating on every release like you propose. Are we using a gitlab fork (to use the pipeline) or continue using the github? (I know there's already been some merge to the gihub repo , but I mean moving forward)

[pagmatt]

@non-det-alle raised some valid points against the Gitlab fork, so I believe that keeping the repository here in Github might be the way forward, at least for the time being.