GNOME Bugzilla – Bug 527137
[PATCH] Add glade catalog files
Last modified: 2010-03-16 10:05:43 UTC
This makes is easier to embed gtksourceview with glade (and doesn't corrupt the glade files...)
Created attachment 108926 [details] [review] Add glade catalog With this patch an optional glade catalog file and library are build and installed. If glade-3 is not installed - nothing happens.
Note that you don't need a dummy library, "libgtksourceview-2.0" works too. So I would simply install the catalog file unconditionally, or better bribe Tristan to include this into glade.
I have no strong opinion about the fact that this should be shipped with gtksourceview or glade... In fact as a former glade3 developer the idea was that it was up to libraries to install their catalogs, so I am fine with having this in gtksourceview: we only have to check with tristan which are the stability guarantees of the format, if it's going to change then it's better that glade3 ships the file so that it can be kept up to date. With regard to the patch, I'd put the catalog in a toplevel "data" dir so that is not mixed up with the gtksourceview sources (we should probably move the .pc file there too). Notpick: please use 2-spaces-indent for the xml file.
By the way, we should also hear distro packagers opinion: I do not want gtksourceview package to depend on glade-3, so they'd need to make a separate package for the catalog... no idea how hard that is, I guess that from their point of view putting it in glade itself is less work
First, thanks everybody for having a look at it. I don't care much where this is integrated but glade3 screwed up my files which was annoying. Anyway, I saw that the patch needs some work in the glade library to do the same hacks glade3 uses to make GtkTreeView work, so I guess we cannot remove this library. Tristan, what do you think about having this in glade?
Created attachment 109466 [details] [review] gtksourceview_glade_catalog.patch I had some difficulty applying that patch - it applied some changes to files in the top-level directory instead of the sub-directory. This patch should apply without problems - it has no other changes.
Created attachment 109467 [details] [review] gtksourceview_glade_catalog2.patch With the new files this time.
Fixed in git master with commit d4925db84f46d1597a18e1dc1f95104bccb4fa02 and cherry-picked to glom-1-12 branch as commit ec0842e917ea3a2c4856dfe0d90d267070388ae8.
Whoops, wrong bug.
Created attachment 156201 [details] [review] updated patch for latest git I updated the patch for git master. As per comment 2, there is no reason to build a dummy library, so this was removed from the patch. Two-spaces indent was used for the XML catalog, as per comment 3. OK to commit, or is the intention still to ship this with Glade?
well, other comments of comment 3 and 4 still stand (e.g. putting it in a data subdir, getting feedback from tristan about format stability guarantees, etc) Most important is comment 4: I really need to get packagers opinion to make a decision here: I think having gtksourceview depend on glade is not acceptable. By the way, in the mean time did other libs start shipping catalog files?
The catalog format has not significantly changed for years excepting some additions to the format. It makes no sense to have GtkSourceView itself depend on Glade, since you noted that your catalog doesnt provide a plugin for Glade, you dont need Glade headers to build, or distribute anything that really requires Glade (if you did, it would be best to package it separately). It would be interesting if the sourceview-dev package including the headers also included the catalog. That could be done when packaging sourceview for any target distribution where the packager can assume that Glade catalog files are to be found in a predetermined path: ${system_prefix}/share/glade3/catalogs. Oh, and to answer your last question, I have a /usr/share/glade3/catalogs/vte.xml here installed by libvte-common ;-)
(In reply to comment #11) > well, other comments of comment 3 and 4 still stand (e.g. putting it in a data > subdir, getting feedback from tristan about format stability guarantees, etc) Regarding the data subdir, sorry that I missed that, I will update the patch. Would you like me to move the pkg-config file in a separate commit? > Most important is comment 4: I really need to get packagers opinion to make a > decision here: I think having gtksourceview depend on glade is not acceptable. (In reply to comment #12) > It makes no sense to have GtkSourceView itself depend on Glade, since > you noted that your catalog doesnt provide a plugin for Glade, you > dont need Glade headers to build, or distribute anything that really > requires Glade (if you did, it would be best to package it separately). The patch depends on the libgladeui pkg-config file for the catalog install location. In Ubuntu, for example, this is packaged in libgladeui-1-dev. I guess that with a catalog installed by gtksourceview, the catalog would go into the gtksourceview dev package, which would then depend on libgladeui-1-dev. The default for the configure option that is added by the patch is to not install the catalog, so the dependency only exists if it is explicitly requested. I do not see a good way to avoid the dependency, but am open to suggestions. > It would be interesting if the sourceview-dev package including > the headers also included the catalog. That would be my expectation, or a package just for the catalog, that would depend on libgladeui-1-dev, but this seems like overkill. > That could be done when packaging sourceview for any target distribution > where the packager can assume that Glade catalog files are to be found > in a predetermined path: ${system_prefix}/share/glade3/catalogs. Ouch. There is already a pkg-config file that includes this location. I do not think that it is a good idea to duplicate it to avoid a dependency.
[...] > > That could be done when packaging sourceview for any target distribution > > where the packager can assume that Glade catalog files are to be found > > in a predetermined path: ${system_prefix}/share/glade3/catalogs. > > Ouch. There is already a pkg-config file that includes this location. I do not > think that it is a good idea to duplicate it to avoid a dependency. Maybe Glade can be an optional build time dependency, needed if you want to build/install the catalog from a tarball (or run a distcheck). From the packager POV, I suppose they could use the pkg-config var in some prerun script to decide the install location of the catalog on the target host, but (correct me if I'm wrong) I doubt thats how things are done. Generally when you assemble a distribution you dictate the paths to be used, and you already know that the glade-3 build you installed matches the gtksourceview build, which passed the path resolution at distcheck time, etc. Does that make any sense ?
(In reply to comment #14) > [...] > > > That could be done when packaging sourceview for any target distribution > > > where the packager can assume that Glade catalog files are to be found > > > in a predetermined path: ${system_prefix}/share/glade3/catalogs. > > > > Ouch. There is already a pkg-config file that includes this location. I do not > > think that it is a good idea to duplicate it to avoid a dependency. > > Maybe Glade can be an optional build time dependency, needed if you want > to build/install the catalog from a tarball (or run a distcheck). Yes, that sounds reasonable, and is what the patch does. The gladeui pkg-config file is only required if --enable-glade-catalog is passed to configure. > From the packager POV, I suppose they could use the pkg-config var > in some prerun script to decide the install location of the catalog on > the target host, but (correct me if I'm wrong) I doubt thats how things > are done. Generally when you assemble a distribution you dictate the > paths to be used, and you already know that the glade-3 build you installed > matches the gtksourceview build, which passed the path resolution at > distcheck time, etc. > > Does that make any sense ? I quickly checked the Ubuntu packages for Glade, and the location of the catalogs is hardcoded to /usr/share/glade3/catalogs. However, the situation you describe is orthogonal to configure checking for the correct location via pkg-config, as a distribution package could simply copy the catalog to the correct (possibly hardcoded) location and ignore the configure option, bypassing the gladeui dependency. The patch could be altered to use /usr/share/glade3/catalogs as a fallback location if the gladeui pkg-config file is not present, but if Glade was not installed, what use would the catalog file be? Altough the catalog could be installed to a hardcoded location and then used by Glade if it is installed at a later time, this seems like the wrong approach if the intent is to avoid a dependency.
(In reply to comment #15) [...] > I quickly checked the Ubuntu packages for Glade, and the location of the > catalogs is hardcoded to /usr/share/glade3/catalogs. However, the situation you > describe is orthogonal to configure checking for the correct location via > pkg-config, as a distribution package could simply copy the catalog to the > correct (possibly hardcoded) location and ignore the configure option, > bypassing the gladeui dependency. Yes precisely it is orthogonal to configure checking, my understanding is that requiring Glade as an optional dependency at that time is fine. From my understanding, this is whats okay and not okay: - You need Glade to be installed in order to install the sourceview catalog from the tarball: OK (as your patch does, its optional anyway). - You need Glade installed to do a proper all inclusive dist check: OK. - You need Glade installed when ./configuring && make installing the sourceview into a target DESTDIR which will be used to package the sourceview dev package: OK. - You need to have Glade installed for any application that links in GtkSourceView: NOT OK. - You need Glade installed in order to install the headers and dev package of GtkSourceView: QUESTIONABLE (for compilation purposes you dont really need Glade, so I would rather say NOT OK here). Unless I am misunderstanding the dependency which you want to avoid, I don't see why Glade should be encoded as a dependency for any of the _disted packages_. For instance, I'm not sure that installing the html that comes with a standard tarball pulls in DevHelp as a dependency just because of the .devhelp index file it happens to install.
I agree with the points listed by Tristan. OK, let's do this :) About the subidr, I see that vte used a "glade" subdir, let's do the same.
(In reply to comment #16) > (In reply to comment #15) > [...] > > I quickly checked the Ubuntu packages for Glade, and the location of the > > catalogs is hardcoded to /usr/share/glade3/catalogs. However, the situation you > > describe is orthogonal to configure checking for the correct location via > > pkg-config, as a distribution package could simply copy the catalog to the > > correct (possibly hardcoded) location and ignore the configure option, > > bypassing the gladeui dependency. > > Yes precisely it is orthogonal to configure checking, my understanding > is that requiring Glade as an optional dependency at that time is fine. > > From my understanding, this is whats okay and not okay: > > - You need Glade to be installed in order to install the sourceview > catalog from the tarball: OK (as your patch does, its optional anyway). > - You need Glade installed to do a proper all inclusive dist check: OK. > - You need Glade installed when ./configuring && make installing > the sourceview into a target DESTDIR which will be used to package > the sourceview dev package: OK. > > - You need to have Glade installed for any application that links > in GtkSourceView: NOT OK. > - You need Glade installed in order to install the headers and > dev package of GtkSourceView: QUESTIONABLE (for compilation purposes > you dont really need Glade, so I would rather say NOT OK here). > > Unless I am misunderstanding the dependency which you want to avoid, > I don't see why Glade should be encoded as a dependency for any of the > _disted packages_. > > For instance, I'm not sure that installing the html that comes with > a standard tarball pulls in DevHelp as a dependency just because of > the .devhelp index file it happens to install. Glade is only a build-time dependency with the patch, not a run-time dependency. Sorry, I should have made this more clear. I think that this covers your last two NOT OK points now :) (In reply to comment #17) > I agree with the points listed by Tristan. > > OK, let's do this :) > > About the subidr, I see that vte used a "glade" subdir, let's do the same. OK, I will update the patch.
Created attachment 156255 [details] [review] patch updated according to discussion
Slightly modified patch (now passes make distcheck by distributing the catalog in the tarball) pushed as commit 3e63dbb715561650d35215be16fc17f46c3a7fea.