Allow crypto to be used with add_subdirectory()
#6
Conversation
I've updated the CMAKE_SOURCE_DIR references to CMAKE_CURRENT_SOURCE_DIR to allow easier use of this code as a Git submodule.
Update ParseBinaryDir to default to 64-bit Release to allow being built in a more consistent fashion as part of eg a add_subdirectory() setup.
Update ParseBinaryDir to fall back to the right module name, as well as change the cmakelist and includes to only reference the current project instead of the global project.
friedkiwi
requested review from
Fish-Git,
ivan-w,
Juergen-Git and
Peter-J-Jansen
as code owners
Use the EXTPKG_NAME constant instead of 'crypto'
Set the include path to CMAKE_CURRENT_BINARY_DIR as opposed to CMAKE_BINARY_DIR so it will always default to the curent project.
Rename uninstall target to ${EXTPKG_NAME}_uninstall to avoid overriding
the generic 'uninstall' target.
This commit matches the changes in SDL-Hercules-390/crypto#6
This commit mirrors SDL-Hercules-390/crypto#6 for this extpkg
|
Hi Yvan! (@friedkiwi) Your proposed changes look reasonable to me. Should the same set of changes be made to all of the other External Package (etxpkg) repositories as well? (i.e. SoftFloat, decNumber and telnet?) That is to say, should your changes be made to all of our extpks repositories? Or are you requesting that it be made to only the crypto repository? |
I have the changes made already to all repositories and can submit PR's, but I didn't want to overload you with three times the same PR to deal with when discussing this. In this way, it doesn't break compatibility with the existing setup, but ideally I would like to move to using the proper CMake settings to define 32/64b (toolchain files/configuration) and debug/release (-DCMAKE_BUILD_TYPE). This would also make it a lot easier to build the main source tree on eg. RISC-V. What's your primary environment to develop on? |
|
Yvan, I work to keep autohell working on as many systems as is reasonable, and have a considerable catalog of virtual machines, Raspberry Pi images, etc. that I test all this on (it doesn't yet include RISC-V -- dunno if that works or not). I will get you that list. Any change to the CMAKE stuff will need to be tested on them all, eventually (for values of "eventually" including "before the next official release"). Bill |
More than happy to help you deal with that. I have a spare Allwinner D1 board here with a RISC-V SoC; I could always send it to you, or make it accessible remotely if you're interested? |
|
If it's easy for you to put it on the net, I could give it a go. Bill |
Be aware, real world performance is Pi1 levels - iirc I've seen you on the Moshix discord? |
|
That's OK. I do Pi Zero as well. Yes, I'm on the Moshix discord. I do the Hercules-Helper builder thingy, and have been working to track down the random failures in the MVS-SYSGEN process. Bill |
Ah. Good. Understood. I agree discussing it here first would be best so we can then apply it to all of the others once your proposed changes here are accepted. Good idea.
If there's something our current CMake setup isn't doing right then we should probably fix that. I'm curious what it is about our current CMake build design that isn't right. What aren't we doing correctly? And how would fixing that break things?
Mine? Windows. But I have a KDE Neon 5.24 and a Kubuntu 21.10 virtual machine that Bill setup for me (as well as a very old CentOS 6.4 system that a different (former) developer setup for me a long time ago) that I use to test Hercules builds on. (I'm not much of a *nix person. I dabble, but am fairly inexperienced. I mostly let other team members (such as Bill) deal with the Linux side of Hercules build issues.) I see that Bill has already opened dialog with you regarding this PR, so I will let him take it from here. Please work with him while I watch. Thanks. |
This PR allows the crypto extpkg to be used with a CMake
add_subdirectory()statement so it can be used as a Git submodule.Summarised, the following changes are made:
replace
CMAKE_SOURCE_DIRwithCMAKE_CURRENT_SOURCE_DIRThis allows the use of the extpkg as a Git submodule
Change the ParseBinaryDir macro to not error out
Now, when the ParseBinaryDir does not detect the right directory format, it defaults to Release64. This way, existing build infrastructure and scripts keep on working, while the module can be included using an
add_subdirectory()statement in anotherCMakeLists.txtfile.The latter is not an ideal fix; in an ideal world these settings would be configured with CMake settings so that they can also be configured using eg the curses or QT GUI. Fixing this, however, would result in breaking all existing build scripts, which I'm personally not fond of.