Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Ubuntu backend with langpack support #18

Merged
merged 5 commits into from Sep 15, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -35,7 +35,7 @@ import utils : isRemote, downloadFile;

class DebPackage : Package
{
private:
protected:
string pkgname;
string pkgver;
string pkgarch;
@@ -61,7 +61,7 @@ public:
override
@property string filename () const {
if (debFname.isRemote) {
immutable path = buildPath (tmpDir, debFname.baseName);
immutable path = buildNormalizedPath (tmpDir, debFname.baseName);
downloadFile (debFname, path);
return path;
}
@@ -121,6 +121,19 @@ public:
return pa;
}

protected void extractPackage (const string dest = buildPath (tmpDir, name))
{
import std.file : exists;
import std.regex : ctRegex;

if (!dest.exists)
mkdirRecurse (dest);

auto pa = openPayloadArchive ();

pa.extractArchive (dest);
}

private auto openControlArchive ()
{
auto ca = new ArchiveDecompressor ();
@@ -39,7 +39,7 @@ import utils : escapeXml, getFileContents, isRemote;
class DebianPackageIndex : PackageIndex
{

private:
protected:
string rootDir;
Package[][string] pkgCache;
bool[string] indexChanged;
@@ -175,6 +175,11 @@ public:
return downloadIfNecessary (rootDir, tmpDir, buildPath (path, "Packages.%s"));
}

protected DebPackage newPackage (string name, string ver, string arch)
{
return new DebPackage (name, ver, arch);
}

private DebPackage[] loadPackages (string suite, string section, string arch)
{
auto indexFname = getIndexFile (suite, section, arch);
@@ -195,7 +200,7 @@ public:
if (!name)
continue;

auto pkg = new DebPackage (name, ver, arch);
auto pkg = newPackage (name, ver, arch);
Copy link
Collaborator Author

@iainlane iainlane Aug 26, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to either make a DebPackage or an UbuntuPackage, so I added a factory method here which UbuntuPackageIndex overrides. Let me know if you can think of a nicer way to do that.

Copy link
Owner

@ximion ximion Aug 26, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The nicest way I can think of would be making use of D's awesome templating system.
That would allow you to write one function for DebPackage and Ubuntu packages which gets compiled for the right type at build-time.

E.g.:

private T[] loadPackages(T) (string suite, string section, string arch)
    if (is(T == DebPackage) || is(T == UbuntuPackage))
{
    auto pkg = new T (name, ver, arch);
    static if (is(T == UbuntuPackage)) {
        // Ubuntu stuff
    }
}

(call with loadPackages!DebPackage ("xenial", "main", "amd64"))

The current solution isn't very unclean though, so I would be okay with that too (the templating stuff is probably the cleanest way though).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like having to name the classes in the code like that, so I prefer to leave it as is unless there's a way around that. It's nice that the Debian backend doesn't mention the Ubuntu one at all currently.

I don't think the static if bit would be required in this case, though. There's no Ubuntu specific stuff at this level.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay - I am fine with the newPackage stuff too, I don't think it's very ugly.

pkg.filename = buildPath (rootDir, fname);
pkg.maintainer = tagf.readField ("Maintainer");

@@ -19,6 +19,9 @@

module backends.interfaces;

import appstream.Component;
import glib.KeyFile;

import std.string;
import std.container;
public import datastore;
@@ -33,6 +36,7 @@ abstract class Package
@property string ver () const @safe pure;
@property string arch () const @safe pure;
@property string maintainer () const;
@property Package[] otherPackages;

/**
* A associative array containing package descriptions.
@@ -59,6 +63,11 @@ abstract class Package
*/
abstract const(ubyte)[] getFileData (string fname);

/**
* Retrieve backend-specific translations.
*/
string[string] getDesktopFileTranslations (KeyFile desktopFile, const string text) { return null; }

/**
* Close the package. This function is called when we will
* no longer request any file data from this package.
@@ -0,0 +1,23 @@
/*
* Copyright (C) 2016 Matthias Klumpp <matthias@tenstral.net>
*
* Licensed under the GNU Lesser General Public License Version 3
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU Lesser General Public License as published by
* the Free Software Foundation, either version 3 of the license, or
* (at your option) any later version.
*
* This software is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public License
* along with this software. If not, see <http://www.gnu.org/licenses/>.
*/

module backends.ubuntu;

public import backends.ubuntu.ubupkg;
public import backends.ubuntu.ubupkgindex;
@@ -0,0 +1,191 @@
/*
* Copyright (C) 2016 Canonical Ltd
* Author: Iain Lane <iain.lane@canonical.com>
*
* Licensed under the GNU Lesser General Public License Version 3
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU Lesser General Public License as published by
* the Free Software Foundation, either version 3 of the license, or
* (at your option) any later version.
*
* This software is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public License
* along with this software. If not, see <http://www.gnu.org/licenses/>.
*/

module backends.ubuntu.ubupkg;

import appstream.Component;

import backends.debian.debpkg;
import backends.interfaces;

import glib.Internationalization;
import glib.KeyFile;

import std.container : Array;

import logging;

import utils : DESKTOP_GROUP;

extern (C) char *bindtextdomain (const char *domainname, const char *dirName) nothrow @nogc;

class UbuntuPackage : DebPackage
{
this (string pname, string pver, string parch, string globalTmpDir, ref Array!Package allPackages)
{
this.globalTmpDir = globalTmpDir;
this.langpackDir = buildPath (globalTmpDir, "langpacks");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How big are the langpacks? Does it make sense to unpack them into an in-memory location for speed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Too big, as above, but a future optimisation is to extract the .mo files only

this.localeDir = buildPath (langpackDir, "locales");
this.allPackages = allPackages;
super (pname, pver, parch);
}

override string[string] getDesktopFileTranslations (KeyFile desktopFile, const string text)
{
string langpackdomain;

try {
langpackdomain = desktopFile.getString (DESKTOP_GROUP,
"X-Ubuntu-Gettext-Domain");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also came across X-GNOME-Gettext-Domain - is that still used / something worth supporting?

} catch {
try {
langpackdomain = desktopFile.getString (DESKTOP_GROUP,
"X-GNOME-Gettext-Domain");
} catch {
return null;
}
}

logDebug ("%s has langpack domain %s", name, langpackdomain);

synchronized {
extractLangpacks ();
return getTranslations (langpackdomain, text);
}
}

private:
string globalTmpDir;
string langpackDir;
string localeDir;
string[] langpackLocales;
Array!Package allPackages;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you plan to add more stuff to the array, use an Appender:
Appender!(Package[]) (initialize with allPackages = appender(Package[]))
Appenders work with memory pools so they don't need to reallocate for every added element.
See https://dlang.org/phobos/std_array.html#appender


private void extractLangpacks ()
{
import std.algorithm : filter, map;
import std.array : appender, array, split;
import std.file : dirEntries, exists, SpanMode, readText;
import std.parallelism : parallel;
import std.path : baseName;
import std.process : Pid, spawnProcess, wait;
import std.string : splitLines, startsWith;

auto path = buildPath (langpackDir, "usr", "share", "locale-langpack");

if (!langpackDir.exists) {
bool[string] extracted;

langpackDir.mkdirRecurse ();

foreach (pkg; allPackages) {
if (!pkg.name.startsWith ("language-pack") || pkg.name in extracted)
continue;

UbuntuPackage upkg = to!UbuntuPackage (pkg);

logDebug ("Extracting %s", pkg.name);
upkg.extractPackage (langpackDir);

extracted[pkg.name] = true;
}

auto supportedd = buildPath (langpackDir, "var", "lib", "locales", "supported.d");

localeDir.mkdirRecurse ();

foreach (locale; parallel (supportedd.dirEntries (SpanMode.shallow), 5))
{
foreach (ref line; locale.readText.splitLines) {
auto components = line.split (" ");
auto localecharset = components[0].split (".");

auto outdir = buildPath (localeDir, components[0]);
logDebug ("Generating locale in %s", outdir);

auto pid = spawnProcess (["/usr/bin/localedef",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugly... But I know you tried to avoid this...

"--no-archive",
"-i",
localecharset[0],
"-c",
"-f",
components[1],
outdir]);

scope (exit) wait (pid);
}
}

}

if (langpackLocales is null)
langpackLocales = localeDir.dirEntries (SpanMode.shallow)
.filter!(f => f.isDir)
.map!(f => f.name.baseName)
.array;
}

string[string] getTranslations (const string domain, const string text)
{
import core.stdc.locale;
import core.stdc.string : strdup;

import std.c.stdlib : getenv, setenv, unsetenv;
import std.string : fromStringz, toStringz;

char *[char *] env;

foreach (ref var; ["LC_ALL", "LANG", "LANGUAGE", "LC_MESSAGES"]) {
auto value = getenv (var.toStringz);
if (value !is null) {
env[var.toStringz] = getenv (var.toStringz).strdup;
unsetenv (var.toStringz);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I should in fact retrieve and restore these? Do we care about preserving the values, since asgen isn't translated?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, not sure... If we - for whatever reason - add translation stuff to asgen in future, we wouldn't need to chase down bugs which for some reason only occur with the Ubuntu backend.
I am a bit worried that this might have an impact on other parts of asgen, mainly the stuff handling fonts / drawing stuff (you never know...). So maybe unsetting the env vars again is a good idea, so we don't have any of the external libraries pick up some strange locale and do weird things because of that.

}
}

scope (exit) {
foreach (key, val; env)
setenv (key, val, false);
}

setenv ("LOCPATH", localeDir.toStringz, true);

auto initialLocale = setlocale (LC_ALL, "");
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is all very thread unsafe. It's only called from synchronized code, but is that enough / correct? I didn't notice any problems when testing but they might be well hidden.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, if it's called from synchronized, there will be nothing else running at the same time, so you should be fine.
(this will hurt performance though :( )

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll switch to the new libmo soon, so we can just read the files directly.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The slightly annoying result of that will be that we'll need to package libmo in Debian and Ubuntu first, or abandon the dub-based build system in favor of Meson, because dub is unable to check for the existence of libraries and is also unable to conditionally compile stuff based on flags (otherwise one could just disable the Ubuntu backend if they don't have libmo).

On the other hand, libmo will likely be super-low on maintenance, so getting it into Debian will be easy.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Tue, Aug 30, 2016 at 08:23:41AM -0700, Matthias Klumpp wrote:

The slightly annoying result of that will be that we'll need to package libmo in Debian and Ubuntu first, or abandon the dub-based build system in favor of Meson, because dub is unable to check for the existence of libraries and is also unable to conditionally compile stuff based on flags (otherwise one could just disable the Ubuntu backend if they don't have libmo).

On the other hand, libmo will likely be super-low on maintenance, so getting it into Debian will be easy.

I would kill dub if it can't do what we want any more. Would ideally use
libmo as a submodule for a while until we're sure it is good enough to
stand alone.

Iain Lane [ iain@orangesquash.org.uk ]
Debian Developer [ laney@debian.org ]
Ubuntu Developer [ laney@ubuntu.com ]

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably also relatively easy to have a D implementation - last time, I just needed to copy the C and massage it slightly ^^

But yeah, long-term I want to get rid of dub anyway, the things that are preventing me from doing so right now are that Meson still needs to do a release with D support (but that should happen very soon, a release is overdue), and that this needs to be updated in Debian.
After that, I think it would be okay to drop dub (other distros will highly likely love that change rather than it causing issues for them - most will have meson already).

scope(exit) setlocale (LC_ALL, initialLocale);

auto dir = buildPath (langpackDir,
"usr",
"share",
"locale-langpack");

string[string] ret;

foreach (ref locale; langpackLocales) {
setlocale (LC_ALL, locale.toStringz);
bindtextdomain (domain.toStringz, dir.toStringz);
auto translatedtext = Internationalization.dgettext (domain, text);

if (text != translatedtext)
ret[locale] = translatedtext;
}

return ret;
}
}
@@ -0,0 +1,62 @@
/*
* Copyright (C) 2016 Canonical Ltd
* Author: Iain Lane <iain.lane@canonical.com>
*
* Licensed under the GNU Lesser General Public License Version 3
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU Lesser General Public License as published by
* the Free Software Foundation, either version 3 of the license, or
* (at your option) any later version.
*
* This software is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public License
* along with this software. If not, see <http://www.gnu.org/licenses/>.
*/

module backends.ubuntu.ubupkgindex;

import backends.debian;
import backends.interfaces;
import backends.ubuntu.ubupkg;

import std.container : Array, make;

class UbuntuPackageIndex : DebianPackageIndex
{

public:
this (string dir)
{
/*
* UbuntuPackage needs to extract the langpacks, so we give it an array
* of all packages. We don't do this here, as you migh think makes
* sense, because it is a very expensive operation and we want to avoid
* doing it if it's not necessary (when no packages being processed are
* using langpacks).
*/
allPackages = make!(Array!Package);
super (dir);
}

override DebPackage newPackage (string name, string ver, string arch)
{
return new UbuntuPackage (name, ver, arch, tmpDir, allPackages);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's sort of ugly to push allPackages down into each package, even as a reference. But it's also right that packages don't know about the index, I think?

Copy link
Collaborator Author

@iainlane iainlane Aug 26, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also we push tmpDir in so that all Packages have a single place to look at for the extracted langpacks. But what about using something inside the XDG_RUNTIME_DIR? That should be a tmpfs, so quite a lot faster. Then again, why doesn't asgen use that for all of its temporary directories? Actually it might not be big enough.

laney@xenial (langpacks|✚1)> du -hs ../asgen-workdir/cache/tmp/asgen-BMQVsyz0/ubuntu/langpacks
921M    ../asgen-workdir/cache/tmp/asgen-BMQVsyz0/ubuntu/langpacks

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right on both points: We don't use XDG_RUNTIME_DIR unconditionally, because we might extract a lot of large objects, like images, which we don't clean up immediately (but all at once at the end of a generator run).
I'd rather like to explicitly use a tmpfs location when we know that the result will be small.

Pushing tmpDir in is fine, I think (if it isn't there, the package will use $workspacedir/cache/tmp/$random/$name-$version, where $workspacedir/cache/tmp/$random is always the same for each session. So in theory, I think, a package could also find out the locale location on its own).

Also: 921M!!! What is in there? That's an insane size for translations!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tons of translations, yelp files, translated images, KDE stuff. The big size is why langpacks exist in the first place. :)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Crazy!

}

override Package[] packagesFor (string suite, string section, string arch)
{
auto pkgs = super.packagesFor (suite, section, arch);

allPackages ~= pkgs;

return pkgs;
}
}

private:
Array!Package allPackages;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It needs to be an Array and not a Package[] because Arrays are passed by reference properly, whereas dynamic arrays aren't (even with ref).

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha! So, ignore my appender commend from above, or check if RefAppender does what I think it does ^^

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't make RefAppender work. I don't think this is too bad though, since we add the packages a suite/section/arch at a time, not individually - so there are actually very few individual append operations.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, did you see that this private attribute is not part of the class but on the global (thread-local) scope?
I am pretty sure that wasn't intended, so I moved this piece of code in master.

@@ -65,6 +65,7 @@ enum Backend
Unknown,
Dummy,
Debian,
Ubuntu,
Archlinux,
RpmMd
}
@@ -276,6 +277,10 @@ class Config
this.backend = Backend.Debian;
this.metadataType = DataType.YAML;
break;
case "ubuntu":
this.backend = Backend.Ubuntu;
this.metadataType = DataType.YAML;
break;
case "arch":
case "archlinux":
this.backend = Backend.Archlinux;