Header files should be grouped to two kinds: the one for library users and another one is for internal use. Just like libgit2, not all header files need to be included by users. Thus, the install target could help install the proper header files to the system include directories, while current structure, which is qgit2.h include everything, does not allow this. Reproducible: Always
For the git2.h file, I personally don't use it anywhere (well, hopefully :). We can remove the includes except the version constants. Maybe, these should be put in the CmakeLists.txt file (like the qmake VERSION variable). What do you think? LibGit2 has the structure include/git2/HEADER.h. Currently I really don't see anything, that shouldn't be published except the data() and constData() methods, which are used internally to access the "low-level" pointers. Additionally this will definitely imply compatibility issues. So, if we do that, maybe better for a future release. Definitely in a separate branch.
(In reply to comment #1) > For the git2.h file, I personally don't use it anywhere (well, hopefully :). > We can remove the includes except the version constants. Maybe, these should > be put in the CmakeLists.txt file (like the qmake VERSION variable). What do > you think? Yes, and there should be a config.h containing version generated by cmake. > > LibGit2 has the structure include/git2/HEADER.h. Currently I really don't > see anything, that shouldn't be published except the data() and constData() > methods, which are used internally to access the "low-level" pointers. > Additionally this will definitely imply compatibility issues. So, if we do > that, maybe better for a future release. Definitely in a separate branch. What do you mean by backward compatibility issue? I don't see any.
By compatibility issues I mean, the change in the include paths. Projects currently use something like #include <src/qgitrepository.h>. This has to change to, let's say #include <qgit2/qgitrepository.h>. For me personally it is not much of a problem - I simply change it. But other projects probably won't compile anymore. I propose to do the change in a topic branch for this reason. I'd like to use the "develop" branch for integration. Creating a "topic/move-headers" branch would be good from there. I don't know, if that's possible using patches. I'll give it a try. So send it and we'll see how that works.
But do you think the structure is reasonable? This structure makes almost impossible a system library that can be installed by user. And is the compatibility issue very important currently?
(In reply to comment #4) > But do you think the structure is reasonable? This structure makes almost > impossible a system library that can be installed by user. And is the > compatibility issue very important currently? The include structure as you proposed is ok. Maybe we could use some forward includes to keep source compatibility? For example in src/qgitrepository.h: qWarn("You are referencing a deprecated header file. This will be removed in next major release. Please use <qgit2/qgitrepository.h> instead"); #include <qgit2/qgitrepository.h> ... Thus users would just have to add the include path, but not forcing to change their includes directly. If that doesn't work for you, just change it.
I agree. And not all of the symbols in the header files need to be exported, right?
(In reply to comment #6) > I agree. And not all of the symbols in the header files need to be exported, > right? Yep.
Will you do this?
(In reply to comment #8) > Will you do this? Ok, thought you would do it in order to the cmake conversion. I'll do it. Maybe you could do the needed changes in CMakeLists.txt afterwards?
(In reply to comment #9) > (In reply to comment #8) > > Will you do this? > > Ok, thought you would do it in order to the cmake conversion. I'll do it. > Maybe you could do the needed changes in CMakeLists.txt afterwards? Started with it and it quickly gets "uncomfortable" causing lots of changes in the library itself. So I thought again and took a look at the Qt sources. I like their solution to it. Example: include/QtCore: QFile -> #include "qfile.h" qfile.h -> #include "../../src/corelib/io/qfile.h" ../../src/corelib/io/qfile.h -> The actual header implementation. According to our lib it would look like this: include/QGit: QGitRepository: #include "qgitrepository.h" qgitrepository.h: #include "../../src/qgitrepository.h" So, I hope you understand it. This is exactly the opposite direction from the idea before. Advantages: * Makes it more easy to add new classes. (cause .h and .cpp is by default created in same dir) * Internal classes can easily be excluded by not linking them in the include dir. * No (forced) change for existing implementations. * To use it, one could use the "Qt way" by writing: #include <QGit/QGitRepository> The only question is: What about the installation/deployment? Are you ok with it, or seeing big troubles on that end? I will open a separate topic branch, so you can see yourself and change it, until we are good to go.
You are talking about header files that are used internally, right? Maybe you can check `/usr/include/qt4/Qt`. You will find that they would not have `src` dir appear.
You are talking about header files that are used internally, right? I am talking about header files that should be used externally by library users. Maybe you can check `/usr/include/qt4/Qt`. You will find that they would not have `src` dir appear.
(In reply to comment #12) > You are talking about header files that are used internally, right? I am > talking about header files that should be used externally by library users. > Maybe you can check `/usr/include/qt4/Qt`. You will find that they would not > have `src` dir appear. Currently there are no internal headers in libqgit2. To encapsulate the data() and constData() methods, we could simply not export them. So, every method itself has to be exported to be publicly available. If you look on different platforms, the Qt headers are actually copied to different places. The originals though, always live in the "src" location (this is, when you install Qt sources). It seems, like on install the headers are copied to the appropriate place for the platform. I thought of taking a similar approach? Or is it too much?
(In reply to comment #13) > (In reply to comment #12) > > You are talking about header files that are used internally, right? I am > > talking about header files that should be used externally by library users. > > Maybe you can check `/usr/include/qt4/Qt`. You will find that they would not > > have `src` dir appear. > > Currently there are no internal headers in libqgit2. To encapsulate the > data() and constData() methods, we could simply not export them. So, every > method itself has to be exported to be publicly available. > > If you look on different platforms, the Qt headers are actually copied to > different places. The originals though, always live in the "src" location > (this is, when you install Qt sources). It seems, like on install the > headers are copied to the appropriate place for the platform. I thought of > taking a similar approach? Or is it too much? You can also do this, since it's quite simple. You said all header files need to be exported, right? Then, copy all the header files to a directory called `include/qgit2`, and do a little modification of the build system: add `include/git2` as a include path, or `include` as include path and modify the `#include` lines in the source file (this is recommended). When installing, just copy the `include/git2/*` to `PREFIX/include/git2`, and everything is done.
And add a `qgit2.h` in `include/qgit2` to have all the other file included. Simple.
(In reply to comment #15) > And add a `qgit2.h` in `include/qgit2` to have all the other file included. > Simple. Ok, sounds good. Even if it does not match exactly the idea of using the qt approach. Anyways, I'll do it and commit to topic/new-include-structure.
(In reply to comment #16) > (In reply to comment #15) > > And add a `qgit2.h` in `include/qgit2` to have all the other file included. > > Simple. > > Ok, sounds good. Even if it does not match exactly the idea of using the qt > approach. Anyways, I'll do it and commit to topic/new-include-structure. Done!
This looks good. I'll close this.
Would merge this into develop branch?
(In reply to comment #19) > Would merge this into develop branch? Yes, would you bring the cmake changes in. I still compile on qmake.
This change still contains some fetal errors: libqgit2/src/qgitref.cpp:28:23: fatal error: git2/refs.h: No such file or directory Could you fix this? Thanks.
(In reply to comment #21) > This change still contains some fetal errors: > > libqgit2/src/qgitref.cpp:28:23: fatal error: git2/refs.h: No such file > or directory > > Could you fix this? Thanks. Hu? Are you referencing to the cmake build? Sorry, I don't think I can help you here. Somehow you're not referencing the correct include path (libgit2/include) I assume? BTW: Tried to open CMakeLists.txt in QtCreator. The Import fails: CMake Error at /opt/local/share/cmake-2.8/Modules/FindPkgConfig.cmake:275 (message): A required package was not found Call Stack (most recent call first): /opt/local/share/cmake-2.8/Modules/FindPkgConfig.cmake:329 (_pkg_check_modules_internal) CMakeLists.txt:78 (pkg_check_modules) Am I missing something? Or do I have to remove a file?
Created attachment 72142 [details] Make cmake work for the new include structure
The patch is attached here. Please test it.
Sorry that I missed something. I thought git2/refs.h is some file in your repo. You remind me that it is from libgit2.
(In reply to comment #25) > Sorry that I missed something. I thought git2/refs.h is some file in your > repo. You remind me that it is from libgit2. No Problem. We are all human :). Well, I cannot really test it as of the error message described in comment #22 above. I really don't like the fact, that libgit2 is referenced as an installed library. I think it should really compile as a subproject, if needed. The reason is, that I partly have to use development versions of libgit2 to implement new features. So, final versions should install, referencing the correct libgit2 version. For example, libgit2 does it similar with zlib.
Libgit2 is used as a shared library. If we use it as a submodule, how should we install libqgit2 then? There has to be conflicts.
(In reply to comment #27) > Libgit2 is used as a shared library. If we use it as a submodule, how should > we install libqgit2 then? There has to be conflicts. I'm not sure if we're talking about the same point. So let me explain a little more. The (now obsolete) qmake version works a little different. I'm currently referencing the source files of libgit2 directly. So libqgit2 is compiled as a single static library without need of libgit2 installed. The reason for that is a "workaround", not having to put an additional .pro file in the libgit2 directory. The point whith switching is: It will become two shared libraries (libqgit2 and libgit2), right? This is ok, but they have stay in sync (maybe it is a current development debug version). When talking about subprojects, I mean referencing the CMakeLists.txt of libgit2 instead of referencing the (in my case not existing) precompiled binary, which would be wrong. Hope you understand it. Actually this is one of the reasons why development became possible again after it stopped for approx. 1 year :).
(In reply to comment #28) > (In reply to comment #27) > > Libgit2 is used as a shared library. If we use it as a submodule, how should > > we install libqgit2 then? There has to be conflicts. > > I'm not sure if we're talking about the same point. So let me explain a > little more. The (now obsolete) qmake version works a little different. I'm > currently referencing the source files of libgit2 directly. So libqgit2 is > compiled as a single static library without need of libgit2 installed. The > reason for that is a "workaround", not having to put an additional .pro file > in the libgit2 directory. > > The point whith switching is: It will become two shared libraries (libqgit2 > and libgit2), right? This is ok, but they have stay in sync (maybe it is a > current development debug version). When talking about subprojects, I mean > referencing the CMakeLists.txt of libgit2 instead of referencing the (in my > case not existing) precompiled binary, which would be wrong. Hope you > understand it. Actually this is one of the reasons why development became > possible again after it stopped for approx. 1 year :). Could you explain what is your "sync"? Does your sync mean, the ligqgit2 library always uses the submodule version of libgit2? Again, if we use them as two separate shared library, which is the best way I think, we can NOT build a specific version of libgit2 by ourselves, since there would be installation conflicts. Did I miss something? Could you explain more?
You can add a note to the build instruction, "the source code is only supported with the latest version of libgit2", to help users understand.
Could you explain what is your "sync"? Does your sync mean, the ligqgit2 library always uses the submodule version of libgit2? Again, if we use them as two separate shared library, which is the best way I think, we can NOT build a specific version of libgit2 by ourselves, since there would be installation conflicts. Yes, we can :) - and we have to. We have to differ between development and deployment. I am actually developing on all three projects (app, qt-wrapperlib and libgit2). Sorry, "sync" was not the right word (written in a hurry). Assume all three projects are on the current working copy, referencing some development commits (let's say develop branch). So to test it out, I have to compile (maybe debug) all three projects. This happens all the time and was the case some days ago when implementing the QGitSubmodule. The "latest" binary is the one I (or any other developer) compile. This, of course, is not deployed as an official version, but the current development state. If it would reference a versioned binary, development was not really possible. I looked a bit around and cmake has the ability to use subprojects (just like qmake). To sum it up, somehow we have to differ between a "make" and "make install", ok? (In reply to comment #30) > You can add a note to the build instruction, "the source code is only > supported with the latest version of libgit2", to help users understand. Nope, not if one is really compiling the lib. It should compile with the checked out submodule (no need of a ready version). If a project just links the lib without need of source code, this is an option. Both should be made possible.
Yes, make is different from "make install". What if some one build against the submodule and then "make install"? And also which option should be the default one, build against submodule or system lib?
If the library is built against the submodule, how should the app use it? It needs the two shared libraries, with specific versions, while an installed libgit2 is possible. Modify "LD_LIBRARY_PATH"? Is the submodule for development usage only?
(In reply to comment #32) > Yes, make is different from "make install". What if some one build against > the submodule and then "make install"? And also which option should be the > default one, build against submodule or system lib? Just to be clear: I am just talking about the project structure. Nothing to do with git (cause you're using the term submodule). So I'll use subproject (referencing the sources). Subproject is always prior to System lib and should have its own build dir, and yes, for development usage only. Look at the projects separately. It is three Projects, that are independently compilable: * The Application * The libqgit2 shared library * The libgit2 shared library If the application references the sources depends on, if the developer needs it. When he's not in the need of debugging or changing the libraries, he simply develops the app, using pre-installed binaries, not referencing the sources in his project structure. We should separate the deploy step completely from development by using a packaging script instead of cmake. The packaging script can copy the release build (e.g. libgit2.a, libgit2.dll, ...) to some install package. For the different platforms,I would advice to use one of these options: Mac: macports / DMG / pkg Linux: apt-get / slapt-get / yast, ... Windows: innosetup So, when building the app, the libraries go to the build dir, which is used prior to the system library. When deploying the app. The deployment scripts copy maybe the release lib to some package. I know it is a somewhat challenging task. But it needs to be done. Maybe, looking at other projects as a reference would be good idea? Modifying LD_LIBRARY_PATH is not an option (it wouldn't work for zlib anymore).
(In reply to comment #34) > (In reply to comment #32) > > Yes, make is different from "make install". What if some one build against > > the submodule and then "make install"? And also which option should be the > > default one, build against submodule or system lib? > > Just to be clear: I am just talking about the project structure. Nothing to > do with git (cause you're using the term submodule). So I'll use subproject > (referencing the sources). OK. > > Subproject is always prior to System lib and should have its own build dir, > and yes, for development usage only. Look at the projects separately. It is > three Projects, that are independently compilable: > * The Application > * The libqgit2 shared library > * The libgit2 shared library > > If the application references the sources depends on, if the developer needs > it. When he's not in the need of debugging or changing the libraries, he > simply develops the app, using pre-installed binaries, not referencing the > sources in his project structure. If the application references the sources depends on, and the project is built, then how does the developer test it? The library path is determined by LD_LIBRARY_PATH (or something else), thus a starting script is required. > > We should separate the deploy step completely from development by using a > packaging script instead of cmake. The packaging script can copy the release > build (e.g. libgit2.a, libgit2.dll, ...) to some install package. > > For the different platforms,I would advice to use one of these options: > Mac: macports / DMG / pkg > Linux: apt-get / slapt-get / yast, ... > Windows: innosetup > > So, when building the app, the libraries go to the build dir, which is used > prior to the system library. When deploying the app. The deployment scripts > copy maybe the release lib to some package. > > I know it is a somewhat challenging task. But it needs to be done. Maybe, > looking at other projects as a reference would be good idea? CPack can do this. http://www.cmake.org/Wiki/CMake:Packaging_With_CPack I have already done some projects with CPack, and the packaging works perfect. But not with innosetup on Windows, but NSIS. > > Modifying LD_LIBRARY_PATH is not an option (it wouldn't work for zlib > anymore).
Thanks! I didn't know CPack so far. Took a quick look at it and it looks neat. Unfortunately it does not seem to support macports (works similar to apt-get, just for mac)? But maybe we could use a separate script for that ... whatever. So, everything looks great. Just the project dependency to libgit2 is missing. Concerning your question how a developer "tests" the library. How would a Testserver do it: 1. git clone --recursive .... && git checkout some_branch && git submodule update --init --recursive 2. build project and subprojects 3. run tests 4. cleanup everything for the next run 5. done! So, hope it makes it clear enough. Especially the Testserver does not really install the program. If you look at other projects, using a lot more shared libraries (like e.g. qt-creator), they do a similar thing. BTW. I'm running a second project on the libqgit2 plus a standalone libqgit2 library project and it works really good! It does not install the binary yet. This probably could be done by CPack?
(In reply to comment #36) > Thanks! I didn't know CPack so far. Took a quick look at it and it looks > neat. Unfortunately it does not seem to support macports (works similar to > apt-get, just for mac)? But maybe we could use a separate script for that > ... whatever. > > So, everything looks great. Just the project dependency to libgit2 is > missing. Concerning your question how a developer "tests" the library. How > would a Testserver do it: > > 1. git clone --recursive .... && git checkout some_branch && git submodule > update --init --recursive > 2. build project and subprojects > 3. run tests > 4. cleanup everything for the next run > 5. done! This is not an issue on Windows, but on UNIX if you do not have a root privilege, and you do not change LD_LIBRARY_PATH, how do you make the app executable use the shared library you have just built? Mozilla also bundle shared libraries, but they have a startup script, which sets LD_LIBRARY_PATH for the process. > > So, hope it makes it clear enough. Especially the Testserver does not really > install the program. If you look at other projects, using a lot more shared > libraries (like e.g. qt-creator), they do a similar thing. > > BTW. I'm running a second project on the libqgit2 plus a standalone libqgit2 > library project and it works really good! It does not install the binary > yet. This probably could be done by CPack? Probably. But you need to have details to decide whether it is possible.
BTW, you should remove `src/CMakeLists.txt`. It's really irrelevant to this project. It might be included by accident.
(In reply to comment #38) > BTW, you should remove `src/CMakeLists.txt`. It's really irrelevant to this > project. It might be included by accident. Yep, it is. I don't know why it exists, but it surely had a reason :). i'll remove it.
(In reply to comment #39) > (In reply to comment #38) > > BTW, you should remove `src/CMakeLists.txt`. It's really irrelevant to this > > project. It might be included by accident. > > Yep, it is. I don't know why it exists, but it surely had a reason :). i'll > remove it. Done in develop branch.
What is the final solution anyway?
(In reply to comment #41) > What is the final solution anyway? Thanks for asking. I made up a cmake file for mspiggit and removed qmake yesterday (unfinished, but at least loadable). The libraries are included by using "add_subdirectory" to support development, when downloading the sources. For projects, using the versioned binary, this should be made public by using CPack or such. So the libs can be used either as binary or together as source. When building from source, the libraries go to the cmake "build" directory and are linked from there. When using the installed binary, the libraries should be used from the installation path. I am not a cmake expert, but it should work that way. I'll put it online in separate branch ("topic/cmake") and am happy when you could help finalizing.
(In reply to comment #42) > (In reply to comment #41) > > What is the final solution anyway? > > Thanks for asking. I made up a cmake file for mspiggit and removed qmake > yesterday (unfinished, but at least loadable). The libraries are included by > using "add_subdirectory" to support development, when downloading the > sources. For projects, using the versioned binary, this should be made > public by using CPack or such. So the libs can be used either as binary or > together as source. When building from source, the libraries go to the cmake > "build" directory and are linked from there. When using the installed > binary, the libraries should be used from the installation path. I am not a > cmake expert, but it should work that way. > > I'll put it online in separate branch ("topic/cmake") and am happy when you > could help finalizing. FYI: Branch is online at github as it is temporary and deleted when port is finished.
I believe the MsPigget libqgit2 submodule should be updated, or the build won't succeed.
Dear Bug Submitter, This bug has been stagnant for a long time. Could you help us out and re-test if the bug is valid in the latest version? I am setting the status to NEEDSINFO pending your response, please change the Status back to REPORTED when you respond. Thank you for helping us make KDE software even better for everyone!
Dear Bug Submitter, This is a reminder that this bug has been stagnant for a long time. Could you help us out and re-test if the bug is valid in the latest version? This bug will be moved back to REPORTED Status for manual review later, which may take a while. If you are able to, please lend us a hand. Thank you for helping us make KDE software even better for everyone!
Thank you for reporting this issue in KDE software. As it has been a while since this issue was reported, can we please ask you to see if you can reproduce the issue with a recent software version? If you can reproduce the issue, please change the status to "REPORTED" when replying. Thank you!
Dear Bug Submitter, This bug has been in NEEDSINFO status with no change for at least 15 days. Please provide the requested information as soon as possible and set the bug status as REPORTED. Due to regular bug tracker maintenance, if the bug is still in NEEDSINFO status with no change in 30 days the bug will be closed as RESOLVED > WORKSFORME due to lack of needed information. For more information about our bug triaging procedures please read the wiki located here: https://community.kde.org/Guidelines_and_HOWTOs/Bug_triaging If you have already provided the requested information, please mark the bug as REPORTED so that the KDE team knows that the bug is ready to be confirmed. Thank you for helping us make KDE software even better for everyone!
This bug has been in NEEDSINFO status with no change for at least 30 days. The bug is now closed as RESOLVED > WORKSFORME due to lack of needed information. For more information about our bug triaging procedures please read the wiki located here: https://community.kde.org/Guidelines_and_HOWTOs/Bug_triaging Thank you for helping us make KDE software even better for everyone!