Bug 875021

Summary: publican build ignores --brand_dir, and doesn't accept a relative path for brand_dir in publican.cfg
Product: [Community] Publican Reporter: Raphaël Hertzog <raphael>
Component: publicanAssignee: Jeff Fearn <jfearn>
Status: CLOSED CURRENTRELEASE QA Contact: tools-bugs <tools-bugs>
Severity: unspecified Docs Contact:
Priority: unspecified    
Version: 3.0CC: rglasz, rlandman, xma
Target Milestone: 3.1   
Target Release: ---   
Hardware: Unspecified   
OS: Unspecified   
Fixed In Version: 3.1.0 Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2013-02-05 22:40:41 EST Type: Bug
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---

Description Raphaël Hertzog 2012-11-09 05:42:14 EST
1/ I wanted to use the "brand_dir" parameter to point to a "brand" sub-directory of my project. So I have put "brand_dir: brand" in publican.cfg. This did not work as it requires an absolute path to work otherwise you get this:

	Using XML::LibXSLT on brand/xsl/html.xsl
Could not create file parser context for file "brand/xsl/html.xsl": No such file or directory at /usr/share/perl5/Publican/Builder.pm line 1259

This should be fixed so that publican auto-expands the relative path.

2/ Then since I can't hardcode the path name (each contributor can clone the git repository in a separate location), I wanted to use the --brand_dir=$PWD/brand command line option. But unfortunately that parameter is mostly ignored (this can be witnessed in my case since the line "Using XML::LibXSLT on" points to the default XSL file and not the one provided by my brand).

Looking at the code, bin/publican correctly retrieves it and injects it in the Publican object that it creates... unfortunately Publican::Builder create its own Publican object that doesn't get those command line parameters (it only gets those from the configuration file).

It looks like that some refactoring is in order to share the same Publican object (create a singleton?).
Comment 1 Jeff Fearn 2013-01-07 22:04:55 EST
So there a couple of issues here.

1/ Not all command line options are supposed to be override-able via the cfg file. This is one such example. I will ensure that all unexpected options in the cfg file are ignored (and burp a message).

2/ Publican is a singleton, so if parts of the process aren't finding things then it is probably a bug in the code. Can you provide a copy of the "Using" lines and the file list in your brand's xsl directory?

Note: if your brand doesn't override the specific xsl then it will use the default. i.e. if your brand has html.xsl but not html-single.xsl then html-single will use the default brand XSL.

It might be worth setting the brand in your books cfg file to a brand that isn't installed, that way it might trigger a more revealing error message.
Comment 2 Jeff Fearn 2013-01-08 00:50:14 EST
1/ This fix has been committed to the devel branch for inclusion in Publican 3.1.

Example output.

WARNING: Unknow config key release, ignoring.

Will leave this in NEW to handle 2/.
Comment 3 Raphaël Hertzog 2013-01-08 06:29:05 EST
Here's the requested output.

$ find brand/
$ publican build --brand_dir=$(pwd)/brand --langs=en-US --formats=html
Setting up en-US
Beginning work on en-US
DTD Validation OK
	Starting html
	Using XML::LibXSLT on /usr/share/publican/xsl/html.xsl
	Finished html

Now if I put $(pwd)/brand in publican.cfg I get the expected:

	Using XML::LibXSLT on /home/rhertzog/x/tdah/brand/xsl/html.xsl

Now I put a wrong value in publican.cfg:
$ grep brand_dir publican.cfg 
brand_dir: /does-not/exist
$ publican build --langs=en-US --formats=html
Failed to load brand file: /does-not/exist/publican.cfg at /usr/bin/publican line 692

Which is exactly the same as if I give the wrong brand on the command line:
$ publican build --brand_dir=/home/rhertzog/nauitse --langs=en-US --formats=html
Failed to load brand file: /home/rhertzog/nauitse/publican.cfg at /usr/bin/publican line 692

So the command line option is not entirely ignored, it's just not correctly forwarded where appropriate...
Comment 4 Raphaël Hertzog 2013-01-08 09:42:48 EST
Hum, the problem is more complicated than it seems. I added some debug statements to the code... $publican->param('brand_dir') is always set but it's set to "/home/rhertzog/écriture/debian-handbook/git/brand" in my case because it resolves some symlinks in the path (due to abs_path() usage I believe) and the following test in Publican::Builder fails:

    my $xsl_file = $common_config . "/xsl/$format.xsl";
    $xsl_file = "$brand_path/xsl/$format.xsl"
        if ( -f "$brand_path/xsl/$format.xsl" );

I added this to verify it:

    carp "ERROR: $brand_path/xsl/$format.xsl doesn't exist" unless -f "$brand_path/xsl/$format.xsl";

I get this in the output:
ERROR: /home/rhertzog/écriture/debian-handbook/git/brand/xsl/html.xsl doesn't exist at /usr/bin/publican line 887

Yet this file exists:
$ ls -al /home/rhertzog/écriture/debian-handbook/git/brand/xsl/html.xsl
-rw-r--r-- 1 rhertzog rhertzog 431 nov.  12 17:26 /home/rhertzog/écriture/debian-handbook/git/brand/xsl/html.xsl
$ perl -e 'print "OK\n" if -f "/home/rhertzog/écriture/debian-handbook/git/brand/xsl/html.xsl"'

And the test works fine in a trivial perl script as this shows. Somehow I suspect some encoding problem. My filenames are in UTF-8 and my locale is fr_FR.UTF-8.

After having added some utf8::is_utf8() checks on various variables, I see that param('brand_dir') and thus $brand_path are not stored in UTF8. The original parameter submitted is properly recorded as UTF8 but abs_path("$brand_dir") in Publican.pm is dropping that flag somehow...

$ perl -Mutf8 -MCwd -e 'print "OK\n" if utf8::is_utf8(Cwd::abs_path("/home/rhertzog/écriture/debian-handbook/git/brand/xsl/html.xsl"))'

But there's something else at play because this alone still works as expected:
$ perl -Mutf8 -MCwd -e 'print "OK" if -f Cwd::abs_path("/home/rhertzog/écriture/debian-handbook/git/brand/xsl/html.xsl")'

That said, I found a simple work around: just replace the abs_path($brand_dir) call in Publican.pm with File::Spec->rel2abs($brand_dir). It will still fail with a relative path because File::Spec relies on Cwd to retrieve the path but at least if you provide a valid absolute path, it doesn't munge it...

$ publican build --brand_dir=brand --langs=en-US --formats=html
Failed to load brand file: /home/rhertzog/écriture/debian-handbook/git/brand/publican.cfg at /usr/bin/publican line 694

(Note the "é" sign of a double UTF-8 encoding...)

I found this entry which confirms the issue and the fact that rel2abs works better: http://stackoverflow.com/questions/9530397/cwd-unicode-support

Maybe it's better to just decode the string returned by abs_path() I don't know. Or maybe only if the current locale uses UTF8...
Comment 5 Raphaël Hertzog 2013-01-08 10:00:06 EST
Following the recommendation on StackOverflow, it seems that the best fix is this:

$ diff -u /tmp/Publican.pm /usr/share/perl5/Publican.pm
--- /tmp/Publican.pm	2013-01-08 15:57:38.005829914 +0100
+++ /usr/share/perl5/Publican.pm	2013-01-08 15:55:27.385813476 +0100
@@ -12,6 +12,7 @@
 use XML::LibXML;
 use Publican::Localise;
 use Cwd qw(abs_path);
+use Encode;
@@ -486,7 +487,7 @@
     $config->param( 'common_config',  $common_config )         if $common_config;
     $config->param( 'common_content', $common_content )        if $common_content;
-    $config->param( 'brand_dir',      abs_path("$brand_dir") ) if $brand_dir;
+    $config->param( 'brand_dir',      decode_utf8(abs_path(encode_utf8($brand_dir))) ) if $brand_dir;
     $self->{configfile} = $configfile;
     $self->{config}     = $config;
Comment 6 Jeff Fearn 2013-01-08 20:59:16 EST
I like rel2abs more than all the encoding so I tested with that. My shell is en_US.utf8.

$ publican build --brand_dir ../../publican-zkouška --formats html

Worked fine.

	Using XML::LibXSLT on /home/jfearn/Source/fedora/publican-git/publican/Users_Guide/../../publican-zkouška/xsl/html.xsl

Creating a symlink to publican-zkouška and using that link as a brand directory worked fine.

Using the full path also worked.

	Using XML::LibXSLT on /home/jfearn/Source/fedora/publican-git/publican-zkouška/xsl/html.xsl

I got the exact same results using the approach in #5, but I think rel2abs is tidier so I went with that.

This fix has been committed to the devel branch for inclusion in Publican 3.1.
Comment 7 Raphaël Hertzog 2013-01-09 02:46:17 EST
(In reply to comment #6)
> I like rel2abs more than all the encoding so I tested with that. My shell is
> en_US.utf8.
> $ publican build --brand_dir ../../publican-zkouška --formats html
> Worked fine.

The issue I saw with rel2abs is when $(pwd) contains a UTF-8 char. So that's not exactly the same case.
Comment 8 Jeff Fearn 2013-01-09 18:43:16 EST
(In reply to comment #7)
> The issue I saw with rel2abs is when $(pwd) contains a UTF-8 char. So that's
> not exactly the same case.

And testing that way does make it blow up, so ugly wins again!

This fix has been committed to the devel branch for inclusion in Publican 3.1.
Comment 9 xuezhi ma 2013-01-30 03:30:09 EST
Verify version:pubilcan-3.1.0-0.el6eng.noarch  -> PASS

Verify steps:
1. Insall publican-redhat.
2. Create a book, and remove brand config in publican.cfg file.
3. Build the book with "publican build --brand_dir=/usr/share/publican/Common_Content/test --langs=en-US --formats=html"
4. Build the book with "publican build --brand_dir=/usr/share/publican/Common_Content/RedHat --langs=en-US --formats=html"
5. Edit publican.cfg, add brand_dir:/usr/share/publican/Common_Content/RedHat
6. Build the book "publican build --formats=html --langs=en-US"

Actual results:
1. Use "publican build --brand_dir=/usr/share/publican/Common_Content/test --langs=en-US --formats=html" 
we got ERROR "Failed to load brand file:/usr/share/publican/Common_Content/test/publican.cfg at /usr/bin/publican line 713"

2. Use "publican build --brand_dir=/usr/share/publican/Common_Content/RedHat --langs=en-US --formats=html"
we got
Starting html
Using XML::LibXSLT on /usr/share/publican/Common_Content/RedHat/xsl/html.xsl

3. If add brand_dir into publican.cfg, then build the book with "publican build --formats=html --langs=en-US"
got "WARNING: Unknow config key brand_dir, ignoring."
then the the book still can be built.