USNavalResearchLaboratory / norm

NACK-Oriented Reliable Multicast (NORM) implementation & tools (RFCs 5740, 5401)
https://www.nrl.navy.mil/Our-Work/Areas-of-Research/Information-Technology/NCS/NORM/
Other
98 stars 36 forks source link

Adds option to force a build of protokit and select a specific commit #15

Closed mjvankampen closed 3 years ago

mjvankampen commented 3 years ago

This way you can achieve a more reproducible build, which is good when you publish a library. Otherwise the protokit version will be different every time.

Could you check if the commit of protokit I selected is good?

PENDING https://github.com/USNavalResearchLaboratory/protolib/pull/31

bebopagogo commented 3 years ago

The current NORM code incorporates Protolib as a git submodule, so a new version of Protolib is only brought into play when we have committed an update to NORM and we have explicitly updated the NORM "protolib" submodule tag reference to roll up which Protolib version NORM uses. In your CMAKE changes you have either a specific tag or the protolib origin/master ... so it's not clear to me how that tag number embedded in the CMakelists.txt file would get updated.

Also note that the "protokit" code is statically linked into the NORM shared library built (at least for other builds) so it has its own private copy of the protolib code. I realize for some purposes where you may multiple apps or daemons using protolib that you may want to have it separated to keep the code footprint small. For the mainstream NORM purposes, I felt it was easier to have this approach since we don't yet carefully conduct protolib libary version control or have checks in our projects to validate that a compatible protolib library is installed/being used. So that's why I have tried to keep the shared NORM library "atomic" (having its protolib internal instead of linking to an external protolib) in this regards.

To summarize here, because protolib is incorporated as a git submodule, the same version of protolib is already used for the NORM build each time (regardless of the state of the protolib origin/master) until we explicitly roll norm up to use a new protolib version.

mjvankampen commented 3 years ago

I see that makes sense but this is not the setup in cmake at this time.

I can make protolib always build statically if you want. Which makes a lot of sense if you don't want to make it into an actual library for reuse.

bebopagogo commented 3 years ago

I wonder if there is a way to get the current protolib submodule tag and have the cmake_fetch_content grab that tag so we don't have to manually update the Cmakelists file?

It looks like "git submodule status protolib" executed in a freshly fetch NORM clone gives you the tag. Since I'm new to CMake, I'm not exactly sure if/how you can embed lines in the Cmakelists to do that?

bebopagogo commented 3 years ago

Here is a link to a web page that describes how to have CMake pull in the submodules in an automated fashion:

https://cliutils.gitlab.io/modern-cmake/chapters/projects/submodule.html

Another option is when the NORM code is pulled to use the --recursive option that automatically pulls in submodule source trees, too. So I think the ZMQ CMakeLists could do that to fetch NORM and the appropriate protolib tag in one fell swoop

mjvankampen commented 3 years ago

Done! I did not use git submodule status as it reflects the currently checked out version, not the target version.

bebopagogo commented 3 years ago

I see that you used git list-tree instead and that seems to do the trick. So, just to clarify my understanding, the basic workflow is:

1) It looks to see if a pre-existing protokit library is already present and, if it does, uses that (where do the header files come from in that case?), and if not 2) Will use a custom protolib tag if given as CMake option, or 3) Fetch the protolib tag indicated by "git list-tree HEAD" which should be the current submodule revision

In relation to my question above regarding the protolib header files if it finds a pre-existing protokit library, the other question is the possibility that the pre-existing installed protokit library may be an incompatible protolib version. It seems safer to pull and build the known compatible protolib version. Since we don't have version checks in the protolib or norm code to validate compatibility between the library and header files, the mismatch might cause a problem without clear indication of what the problem was. So that default use of the pre-existing protokit library worries me a little bit. What do you think?

Since I'm not yet spun up enough on CMake (although that's what I hope to transition to instead of 'waf'), I don't really have a stake in this and I think your approach should be fine and will accept your pull request here (assuming the question above regarding the protolib header files is not an issue). For the longer term, one thing comes to mind given my own use patterns, probably more as the NORM code developer than user of the code:

I usually pull in the protolib submodule code to work NORM updates (which sometimes entail adding protolib features) and would build/test using that local modified protolib submodule source tree. If I were using CMake to do this, would the above work for this case? I.e., would it find the existing local submodule tree and use that for the build? Or would I need to take special measures to do that. So, a slight preference on my part as an alternative to using a pre-installed protokit library found as the default protolib, if the CmakeLists.txt found an existing checked out "protolib" submodule tree in the "norm" code directory, if would use that by default (i.e., where a user has already pulled in the protolib submodule code). I think this would sort of give you the best of both worlds ... A default behavior of using the submodule tree for users who either use the "git pulll --recursive" option that grabs submodule code or your approach for users that don't know or want to invoke the submodule pulll ... Any thoughts on this?

mjvankampen commented 3 years ago

So option 1. was removed in this commit. This means the branch/commit specified by the user is used and otherwise the submodule target commit is used.

What I understand is that protolib and norm are so tightly linked you develop protolib inside norm instead of on its own. In that case it might make sense to add an option to use protolib from the /protolib sub directory.

I think this would be easiest if you can set the user option to specify which protolib version to use to something like subdir or local.

mjvankampen commented 3 years ago

Ok implemented as my message before, please have a look!

Just use cmake ../ -DNORM_CUSTOM_PROTOLIB_VERSION="./protolib" As the initial configure step.

bebopagogo commented 3 years ago

OK - I will merge this in. Thanks for your contribution here. It's greatly appreciated! NORM and Protolib are tightly coupled. I hope to improve how Protolib sets its build macros and option to reduce this coupling a little bit and reduce the number of failure modes for mismatching protolib headers, build configuration, and the built library so it can be managed more independently. I think CMake provides better means for managing that in a cross-platform way as I learn more about it.