rework project structure and seperate examples, test and python subdirectories from libsys-sage.so#74
rework project structure and seperate examples, test and python subdirectories from libsys-sage.so#74mozarif wants to merge 29 commits into
Conversation
…gainst libsys-sage.so instead
mozarif
changed the title
… new documentation for the examples
|
Ready for review |
|
examples don't compile according to readme (installing sys-sage locally) Please adapt the readme. |
|
tried building this sys-sage with DS_HWLOC ( Please have a look at it to make sure it compiles properly. Also, the PR is very complex with lots of changes, which makes it hard to review. If you have the chance, try splitting those into multiple smaller ones in the future, and/or list all changes in the PR description. |
| parsers/qdmi-parser.hpp | ||
| parsers/iqm-parser.hpp | ||
| ) | ||
| target_compile_features(sys-sage PUBLIC cxx_std_20) |
There was a problem hiding this comment.
please move these lines to the main cmake file, roughly where they were. you can use an interface library.
There was a problem hiding this comment.
Which lines do you mean? Can you give me the concrete line numbers
There was a problem hiding this comment.
58-60 -- the ones you removed from there
There was a problem hiding this comment.
They can not be moved directly to the root CMakeLists.txt, since they depend on the target sys-sage which is defined inside of src/CMakeLists.txt.
I have made it so that the root CMakeLists.txt only exposes the public API, e.g. the options (and now the dependencies), and the src/CMakeLists.txt handles the actual target build. So everything inside of src/CMakeLists.txt is target dependant. I could make the compile options public and decouple it from the target like it was before, but I don't know why this would be necessary.
I'm also not sure what you mean by interface library.
There was a problem hiding this comment.
https://cmake.org/cmake/help/latest/command/add_library.html#interface-libraries
as far as I understand, with this, you can pre-define the library in the top-level cmake, add the properties there, and then you build it here (with the second add_library)
There was a problem hiding this comment.
I have just tried to define an interface library in the root CMakeLists.txt and to move the compiler options there, but there are some problems with this and I wasn't able to find a good solution.
There are 3 compiler options that we want to set:
- set the language standard to C++20
- add the warning flag -Wall
- disable compiler extensions such as from GNU
The thing is we want to propegate the C++20 dependency to the user, so that the user needs a compiler that supports C++20. However, the warning flag -Wall should not be propegated and instead should only apply to sys-sage itself. This means we need to split this into two interfaces: compiler_options and compiler_warnings, which are public and private respectively. Also, disabling the compiler extensions is a target-specific property, meaning that it needs to be applied to the actual sys-sage target and not the interfaces. Otherwise it doesn't work (at least I didn't find a solution to this).
In the root CMakeLists.txt, I would then have
add_library(compiler_options INTERFACE)
target_compile_features(compiler_options INTERFACE cxx_std_20)
add_library(compiler_warnings INTERFACE)
target_compile_options(compiler_warnings INTERFACE -Wall)and in the src/CMakeLists.txt, I would have
add_library(sys-sage SHARED ${SOURCES})
set_target_properties(sys-sage PROPERTIES CXX_EXTENSIONS OFF)
target_link_libraries(sys-sage PUBLIC compiler_options)
target_link_libraries(sys-sage PRIVATE compiler_warnings)I would also need to slightly change the target install to also include the compiler_options target, since it is a public dependency.
In my opinion, this isn't a good approach with the interfaces, because we need to distinguish between public and private interfaces and some options need to be done on the actual target. It could also be that there is a way of fixing this which I'm not aware of. But I would suggest to keep the compiler flags in the src/CMakeLists.txt file, just as I have done so. These are internal commands. The only interesting thing for the user would be that sys-sage requires the C++20 languag standard, which can simply (and should actually) be mentioned in the README. I think the other flags are not worth exposing.
|
I have stopped with the review now; please look at the suggestions. Once you integrate them, the cmake files will hopefully be more similar, which will make it easier for me to review. |
The README in the root directory mentions that if someone has done a local install of sys-sage and wants to use it inside of CMake, then one needs to export the I can mention this again in the README of the examples (and probably the test) directory... |
On my system (ArchLinux), everything works fine. I think the problem is that you don't have PS: I think |
You can always look directly at the files in the branch of my fork for more clarity. Anyways, I have replied to your comments. Please continue with the review |
Yes, please, do so -- to make it as easy for the user as possible. |
I am on a Mac. sys-sage should work on macs as well, unless the functionalities build on linux-specific libraries and interfaces (e.g. /proc/cpuinfo) |
Yes I know. If you look closely, the error is generated from this line: cmake/FindHWLOC.cmake:69 (find_dependency) This file was already there since I have started as a HiWi. I haven't touched it, so I'm not the one who introduced this dependancy. I'm not sure why all of a sudden you can't compile it anymore. Can you please compile the master branch with the DS_HWLOC flag and tell me whether it works there? Even if you don't have pkg-config installed, the if statement on line 87 should handle this case without problems. I'm confused |
|
I think I've fixed the problem with the -DDS_HWLOC option. Could you please try again? |
| parsers/qdmi-parser.hpp | ||
| parsers/iqm-parser.hpp | ||
| ) | ||
| target_compile_features(sys-sage PUBLIC cxx_std_20) |
There was a problem hiding this comment.
https://cmake.org/cmake/help/latest/command/add_library.html#interface-libraries
as far as I understand, with this, you can pre-define the library in the top-level cmake, add the properties there, and then you build it here (with the second add_library)
| # -DDS_NUMA=ON -- builds the caps-numa-benchmark. If turned on, includes Linux-specific libraries. | ||
| # -DQDMI=ON -- builds with QDMI support. If turned on, includes QDMI library headers. | ||
| # -DPAPI=ON -- builds with PAPI support. If turned on, includes PAPI library headers. | ||
| # -DCMAKE_INSTALL_PREFIX=<prefix> -- to set the install destination (default on UNIX platforms: /usr/local) |
There was a problem hiding this comment.
leave the -DCMAKE_INSTALL_PREFIX=../inst-dir, so that users can copy-paste this, and mention that this is for local installation (in this repo/inst-dir)
There was a problem hiding this comment.
Actually, the CMAKE_INSTALL_PREFIX option isn't just for local installs. If the user wants to make a system-wide installation, one would need to do -DCMAKE_INSTALL_PREFIX=/usr for instance. But never mind...
|
Also by the number of comments, you probably see that PRs like this don't work. You mix so many things together in this one PR, so that it took me hours to review it. I probably still missed many things, so I will have to spend more hours to review it again. Moreover, orienting ourselves in the comments will be untrivial as well. Next time, please make sure your PRs are incremental, focussing on one problem. This could easily have been 5 PRs, and it would have saved both of us a lot of times -- when we could focus on one thing at a time. Finally, in an already very large PR, shuffling lines makes it even harder for me to conduct the review. I see the diff where lines randomly appear and disappear -- this must have costed you a lot of time to rearrange, and it has costed me a lot of time to understand what you are doing, if that is okay or not; and the added value is often zero. (comment for myself) I did not review the CMAke files fully, will have to revisit them; also have to test the examples and the build options. |
|
Just a general comment on your remarks. I get it, you want to make your work as easy as possible, but you don't consider my perspective. Writing the code and doing some changes in the codebase in a way so that it is easy for you to review isn't a top priority for me. Sometimes you have to break things to make it right. And if that means that the git diff viewer gets messy at times, then that's something I can not guarantee to avoid. I know the PR started with only one task: seperate the installation of the Python bindings from the main cmake installation procedure of the C++ library, as we have discussed in a meeting some time ago. To make this work, I had to change the src/CMakeLists.txt file, because I needed a way for the Python bindings to find sys-sage through CMake and to link against it. This also explains the changes in the pkg directory. In order for me to solve this problem, I had to take a look into which files get installed and where. And let me tell you, sys-sage's installation is confusing to say the least. Let me illustrate my point: I have already mentioned that the examples shouldn't be part of the installation and they should never be installed into the bin directory of the user. If I clone the develop branch of sys-sage and run the following commands mkdir build && cd build
cmake -DCMAKE_INSTALL_PREFIX=../inst-dir ..
make all installjust as shown in the README, the contents of the inst-dir are: [inst-dir]$ ls
bin inc libThe contents of the bin directory are the examples, which shouldn't be there. The name "inc" for the inc directory is wrong, since it doesn't match the standard include path. C and C++ compilers expect the include directories to be named "include" to have a correct path lookup in either /usr/include or /usr/local/include. Having /usr/inc or /usr/local/inc wouldn't work out-of-the-box and the user would still need to export some environment variables, even though he made a system-wide install. This isn't good. Also, the inc directory contains an empty directory named "python-bindings". I don't know why it is there, but that's a mistake, since the python-bindings directory doesn't have a .hpp file. Lastly, the lib directory contains the following: [inst-dir]$ ls lib
cmake libsys-sage.so sys-sage.pcThe sys-sage.pc file isn't placed correctly here. It needs to be put in a subdirectory named "pkgconfig", which is expected for standard path lookup. Otherwise, [inst-dir]$ ls lib/cmake
inc lib sys-sage syssageAs you can see, for some reason you have a second installation layer inside of the cmake dir which contains an exact copy of the inc directory and the lib directory again contains libsys-sage.so. So we have 2 nested installations of sys-sage. Why? Also, the contents of the sys-sage and syssage subdirectory are sys-sage-config.cmake and sys-sage-targets.cmake, sys-sage-targets-noconfig.cmake respectively. They should be merged together into one directory since they are both needed for the same task in CMake. I also think having them seperately prevents CMake from finding the files automatically, but I haven't tested it and I give you the benefit of the doubt. I think I have made my point clear. The cmake files contain errors, are messy and are hard to reason about. I have fixed this and it is now usable for the Python bindings. Complaining about lines not being where they used to be so that the git diff viewer is harder to read isn't really important in comparison. I think you understand. Regarding other changes to the cmake files, have a look at my answers to your comments above. Now, the reason why I have expanded the PR to what it is now (also seperating the test and examples directory and reworking the header includes) is because judging by the rate you review PRs, I thought it would take much longer to split this into other PRs which would depend on this one. So instead of waiting for you to first review and merge this singular PR (seperating the Python bindings and changing the project structure/cmake files), for me to then create the subsequent PRs (seperating tests, seperating examples, changing the respective cmake files, fixing headers) which would then again stall the review pipeline, I've put them together. If you remember, during this time you told me you were busy working on your doctoral thesis and you wouldn't be able to have a look at the PRs for like 3-4 weeks or so. Putting everything together in one PR was my way of doing some work instead of waiting for you and doing absolutely nothing for ca. 2 weeks. On the changes to the header includes specifically: Since I had to change the project structure within the src directory, as discussed in a meeting, I had to consequently change the header include paths inside of each .hpp and .cpp file. Meaning I had to rework the header includes anyways. Since I noticed that headers were sometimes included without actually needing them and many source files rely on transitive header includes of other files for correct compilation, I took the time to also fix this and to bring some order here. Within each file, the header includes are structured into the following sections (look at the gitlab issue I created regarding unifying the coding style):
This way, we ensure that every file contains all headers needed and is thus modular and makes development easier. The order of header includes go from special/project-specific includes to general/standard library includes. Now we can argue about the details such as having a space between each section or anything else, but the header includes need to be fixed. Sometimes, I have even removed unnecessary headers and replaced them with forwarding definitions. This reduces compilation time. In case you wonder why we use <> instead of "", for instance in <sys-sage.hpp> instead of "sys-sage.hpp" in the examples or the README, it's because "" wouldn't be correct when doing system-wide installs of sys-sage. If you surround a header include with "", this signals to the compiler to look for the header file in the current working directory. If it is not found there, then the compiler tries to look for standard paths like /usr/include. Since the user likely doesn't have the sys-sage header files in the working directory, we should use <> instead, which directly looks for the standard include paths. Likewise, I have also used them inside of the sys-sage source files, e.g. |
|
Look, what I offer you is to bring some order to the codebase so that sys-sage has a solid foundation. I want sys-sage to succeed as well, but for that to happen there are some radical changes needed in my opinion. Changes that both affect the outside API for the user and changes that affect the project's internals for the developer. sys-sage still is in an early stage and there's still an opportunity to solidify a strong foundation. A foundation that is clean, robust and easy to work with. I know that you as a project manager might not care as much about the internal structure, but I do as a developer. Having some order in the codebase makes the implementation easier for me. Do you know how hard it was for me to understand what was going on in the codebase when I first started? The codebase was a mess. It didn't even compile, documentations were incomplete and outdated, coding styles were all over the place, wrong installation procedure, confusing cmake scripts, erroneous memory management... Take for instance the delete vs. destructor topic. The reason why I was so invested in this is because sys-sage was broken at such a fundamental level that it quite honestly was baffling to me. Since I care about sys-sage, I wanted to make it right. If I wouldn't care, I would simply be a yes-man and just shut up and do as you say without pointing out some concerns I have. At the end of our discussions on this topic I gave in and we were able to come to an agreement. Part of why this took so long was due to miscommunication. I know that I can be hard to work with sometimes, and I'm sorry for that, but someone has to question and point out certain things. I just want it to be done correctly. I'm sure you have your own opinion and that we need to discuss about the details, but I hope through discourse we can arrive at a solution. For this PR specifically, I broadly told you about the changes that needed to be done, but I went ahead and made the decisions about the details myself, since you were busy at that time and we have canceled a bunch of meetings. I thought instead of asking you all the time at a time where you are not really available, I just implement it and this would lay the ground for future discussions/changes. I didn't expect this to be such a burden for you. I honestly don't know what the best approach would be in general. I know we have to discuss and agree on the direction we want to take when implementing something, but I can't ask you every time what to do when I encounter something when implementing. |
2 participants
No description provided.