search

kvetak / RINA

RINA Simulator
https://rinasim.omnetpp.org/
Other
29 stars 12 forks source link

Add OMNeT++ 6 preview support. #25

Open karlhto opened 4 years ago

karlhto commented 4 years ago

As of OMNeT++ 5.3, a new message compiler was introduced. The legacy message compiler is still used by default in OMNeT++ 5, but for compatibility with future versions I have explicitly set the msg4 option, which enables the legacy message compiler. Also updated to C++17. Literally no reason to use C++11.

Implicit casts are now preferred over the use of .longValue() for cPar. You can see why under features introduced in 5.3: https://doc.omnetpp.org/omnetpp/api/APIChanges.html .longValue() is replaced by .intValue() in OMNeT++ 6, so casting is the better option anyway in regards to compatibility.

SubmoduleIterator uses * instead of () as an access operator now. https://doc.omnetpp.org/omnetpp/api/classomnetpp_1_1cModule_1_1SubmoduleIterator.html

From https://github.com/karlhto/RINA-thesis originally, which was a private bare clone of this repo. Check it for original commit dates if wanted.

karlhto commented 4 years ago

Oh yeah, I also added -Wno-inconsistent-missing-override because files generated by the message compiler will spam this warning. Ths warning is unlikely to be encountered anyway.

Also, I added -Wextra because I think it's a good idea to write good code. :)

kvetak commented 3 years ago

Hi there!

So what is the OMNeT++ version that you recommend to test your implementation?

So far 5.2.1 does not work when I clone master of https://github.com/karlhto/RINA-thesis.

karlhto commented 3 years ago

I recommend the latest version @kvetak Either 5.6.2 or 6 preview 8. Should work out of the box as long as you have inet4 in the same directory as rina. If you go with OMNeT++ preview 8 this should work regardless, but 5.6.2 only requires you to add the inet project as a project reference in the OMNeT++ IDE if you use that. I used functionality from newer versions (where a lot of stuff has been improved).

So, if you use the IDE:

If you do this from outside the IDE, using the Makefile that is supplied:

Using the simulate.sh script, you need to define nedpath for INET4 NED-files, which can be done like:

./simulate.sh examples/Shim/SwitchShim -G -c PingAll -n ../inet4/src

...where:

Please notify me if any of this does not work.

karlhto commented 3 years ago

I don't really know why there's a discrepancy between the names of the included INET4 in OMNeT++ 6 and OMNeT++ 5.6.2, but official tools and the docker images use inet4 as the path. Note that OMNeT++ 5.6.2 and OMNeT++ 6 both include version 4.2.0 of INET, but OMNeT++ 5 for some reason uses the wrong name.

IMO they should have kept the name as inet for both releases to not mess up the already fragile setups that OMNeT++ facilitates in regards to dependencies.

karlhto commented 3 years ago

Also note that I have not tested this on Windows, but I don't see any reason why it shouldn't work if you use the IDE. The code should be completely agnostic, so if there's a problem it's probably in the Makefile or something.

kvetak commented 3 years ago

I have spent a decent amount of time trying to run your RINASim version from master of your repo. Unfortunately, I was unable to compile it in neither version following any path (CLI vs GUI) in your guideline out-of-the-box. The problem always lies in cross-reference between inet and rinasim. Currently, I do not have any option how to reproduce your results and check your scenarios.

karlhto commented 3 years ago

Weird. Maybe you need to use a new workspace for OMNeT++?

kvetak commented 3 years ago

Following occurs when trying to compile your master out-of-the-box. inet and rina are in the same directory samples\ and rina references inet project.

OMNeT++ 5.6.2 image

OMNeT++ 6prev8 image

karlhto commented 3 years ago

Interesting! I'll look into it. :)

kvetak commented 3 years ago

@screw was able to compile it, so I will inform at him.

However, let's try to coordinate work on pull-requests aiming to integrate your thesis. Generally, I am okay with your changes in Flow Allocator module as long as they will not break fingerprints on previous non-ShimEthernet scenarios.

screw commented 3 years ago

I had the same error as @kvetak so I manualy added the inet/src as another include location and also comment out one line using some random function from .

I wonder if perhaps you have some settings in .cproject, .project, etc. that didn't get pushed to the repo due to gitignore.

karlhto commented 3 years ago

@kvetak Regarding breaking fingerprints:

Since my changes trigger flow allocation of management flows more times, it follows that fingerprints won't match for most examples where that's relevant since it's computed by the set of events that happen. I am open to discussion on it, but we should probably take that in the relevant PR (the two last commits are relevant). Also, IIRC most of the recorded fingerprints were already incorrect from before I started developing the projects.

karlhto commented 3 years ago

@screw is from the C++ standard library (I assume you're pointing to find_if), so I sincerely doubt that it should create an issue, even on Windows. Maybe it was included through nested includes on Linux somehow?

Also this PR in and of itself should compile alright. I think that it's compatible with every version of OMNeT++ after and including 5.3 at least, but maybe before as well. Note that INET 4.2.0 claims that they only support OMNeT++ 5.5.1 or later, so including my other changes it's probably a good idea to support the newest versions regardless.

I tested both with earlier and later versions of OMNeT++, and no examples were broken that weren't already broken from before. I have some additional changes that I haven't put in a PR yet that fixes several examples, like SimpleLS which has random results every time because of uninitialised values.

screw commented 3 years ago

I tested it on macOS and it didn't compile so I was just trying things. I didn't have a chance to test it on Linux. The problem is random_shuffle(). Per google search it has been removed from in C++17.

Also, I am against -Wno-inconsistent-missing-override.

I agree we should discuss other points in the corresponding PR.

karlhto commented 3 years ago

I tested it on macOS and it didn't compile so I was just trying things. I didn't have a chance to test it on Linux. The problem is random_shuffle(). Per google search it has been removed from in C++17.

I see, I assumed wrong, sorry. :^) I'll look into that too.

Also, I am against -Wno-inconsistent-missing-override.

I forgot to specify why I added the change: this warning appears in every single auto-generated _m.h file, so it creates a lot of noise when compiling. I do agree that it's a good idea to include more warnings to prevent bad code, but there are plenty of more serious warnings in the codebase already that should be brought more attention to.