Re: [Libreoffice] [PATCH] compiling libreoffice-3-4 sal module with dbglevel=2: init test fails

On Fri, Aug 05, 2011 at 04:19:49PM +0100, Caolán McNamara wrote:
> On Thu, 2011-08-04 at 12:07 +0200, Lionel Elie Mamane wrote:

> > A two-day-old clone of branch libreoffice-3-4 fails to build module
> > sal with dblevel=2, unittest fails: osl_old_test_file.cxx assertion
> > "#failure 1.1": the failure is that blah/a/.. is not transformed to
> > "blah", but stays "blah/a/.."

> > That's because in my setup symlinks are allowed, and in that setup
> > osl_getAbsoluteFileURL does not touch the last component (the part
> > after the last slash).

> Presumably this is with environmental variable SAL_ALLOW_LINKOO_SYMLINKS
> set to true, i.e. building in an environment where . ./ooenv was
> previously done to set the hacky SAL_ALLOW_LINKOO_SYMLINKS to true

"grep -i symlink LinuxX86-64Env.Set.sh" does not find anything. OTOH,
the C++ source code looks like

oslFileError osl_getAbsoluteFileURL(rtl_uString*  ustrBaseDirURL, rtl_uString* ustrRelativeURL, 
rtl_uString** pustrAbsoluteURL)
{
    static char *allow_symlinks = getenv( "SAL_ALLOW_LINKOO_SYMLINKS");

    if (!allow_symlinks)
    {
      (...)
    }
    else
    {
      // THIS BRANCH IS TAKEN
      (...)
    }
solenv/bin/linkoo:export SAL_ALLOW_LINKOO_SYMLINKS=1}

so SAL_ALLOW_LINKOO_SYMLINKS must be set in some way somewhere.

Ah, I found:
solenv/bin/linkoo:export SAL_ALLOW_LINKOO_SYMLINKS=1
sc/source/ui/vba/testvba/runTests.pl: $ENV{"SAL_ALLOW_LINKOO_SYMLINKS"} = "1";
toolkit/workben/layout/TEST:export SAL_ALLOW_LINKOO_SYMLINKS=1


> I worked around this at
> http://cgit.freedesktop.org/libreoffice/ure/commit/?id=784641a19811c22208e7db8efa150e744348d8bf
> in master.

> Don't know, in the light of that, if we still need this proposed patch
> then ?

Your workaround unsets that variable for the test, effectively saying
that the "else" branch above should not be tested at all. From a
general point of view, this looks fishy; if a specific subtest does
not apply to this branch, I'd rather disable just that specific
subtest; I mean the file sal/qa/osl/file/osl_old_test_file.cxx tests
for the SAL_ALLOW_LINKOO_SYMLINKS environment variable, and if it is
set, runs only the subtests that should pass. For example, in

const char * const aSource1[] =
{
    "a"    , "file:///" TEST_VOLUME "bla/a",
    ///TODO: check if last slash must be omitted in resolved path.
//    "a/"   , "file:///" TEST_VOLUME "bla/a",
    "../a" , "file:///" TEST_VOLUME "a" ,
    "a/.." , "file:///" TEST_VOLUME "bla/",
    "a/../b" , "file:///" TEST_VOLUME "bla/b",
    ".."   , "file:///" TEST_VOLUME "",
    "a/b/c/d"   , "file:///" TEST_VOLUME "bla/a/b/c/d",
    "a/./c"   , "file:///" TEST_VOLUME "bla/a/c",
    "file:///bla/blub", "file:///"  "bla/blub",
    0 , 0
}

it would not run two of them, the ones where the first part of the
pair ends with "..".

To get into this specific case, my guess of the semantics that one
wants from osl_getAbsoluteFileURL tells me that these subtests should
pass, too. I mean, if "/flob/foo/bar" is a symlink to qux/bar, do we
want osl_getAbsoluteFileURL("/flob/foo/bar/..") to:

 - CASE 1: effectively refer to the directory "/flob/foo/qux", and
   thus it should remain the string "/flob/foo/bar/.."

 - CASE 2: effectively refer to the directory "/flob/foo", and thus it
   should become the string "/flob/foo"

My guess is for CASE 2. For example, case 2 maintains the invariant
that these two refer the same file or directory:

 - result of osl_getAbsoluteFileURL("/flob/foo/bar/../bral")

 - concatenation of:
   1) result of osl_getAbsoluteFileURL("/flob/foo/bar/..)
   2) the string "/bral"

and that file is /flob/foo/bral. Which is then not the same file as
"/flob/foo/bar/../bral", which is the file "/flob/foo/qux/bral".

On the other hand, CASE 1 maintains the invariant that these two refer
to the same file or directory:

 - the string "/flob/foo/bar/../bral"

 - concatenation of:
   1) result of osl_getAbsoluteFileURL("/flob/foo/bar/..)
   2) the string "/bral"

and that file is "/flob/foo/qux/bral", which is then _not_ the same
file as referred to by the result of
osl_getAbsoluteFileURL("/flob/foo/bar/../bral"), namely
"/flob/foo/bral".


If I'm right in my guess and CASE 2 is true, then we should:

1) revert your patch
2) apply my patch


But if from your better understanding of the codebase you tell me CASE
1 is true, then IMHO we should:

1) revert your patch
2) disable only the subtests that don't apply in the case they don't,
   but still run the others. I'll gladly write that patch something like:


@@
 const char * const aSource1[] =
 {
     "a"    , "file:///" TEST_VOLUME "bla/a",
     ///TODO: check if last slash must be omitted in resolved path.
 //    "a/"   , "file:///" TEST_VOLUME "bla/a",
     "../a" , "file:///" TEST_VOLUME "a" ,
-     "a/.." , "file:///" TEST_VOLUME "bla/",
     "a/../b" , "file:///" TEST_VOLUME "bla/b",
-     ".."   , "file:///" TEST_VOLUME "",
     "a/b/c/d"   , "file:///" TEST_VOLUME "bla/a/b/c/d",
     "a/./c"   , "file:///" TEST_VOLUME "bla/a/c",
     "file:///bla/blub", "file:///"  "bla/blub",
     0 , 0
 };

+const char * const aSource1_nosymlink[] =
+{
+    "a/.." , "file:///" TEST_VOLUME "bla/",
+    ".."   , "file:///" TEST_VOLUME ""
+}

@@void oldtestfile::test_file_001()
    for( i = 0 ; aSource1[i] ; i +=2 )
    {
      (...)
    }
    if (! allow_symlinks )
      for( i = 0 ; aSource1_nosymlink[i] ; i +=2 )
      {
        (...)
      }


What do you say?

-- 
Lionel