Dependencies should include "realpath"

Dependencies should include "realpath"

[Ralph Seichter]

Since this issue just came up related to my MacPorts port: The list of
dependencies for notmuch should include "realpath". configure [1] relies
on realpath to generate the value for rsti_dir.

[1] https://git.notmuchmail.org/git?p=notmuch;a=blob;f=configure;h=40e8b2559e86b40985ef2c6dc142eb0104c3ddb6;hb=45193bab16c728ba892a5d45fc62ef59e2a6ef85#l1539

While Linux usually comes with realpath included, macOS does not, and
I am not quite sure about BSD variants.

-Ralph

Re: Dependencies should include "realpath"

[David Bremner]

Ralph Seichter <ralph@ml.seichter.de> writes:

> Since this issue just came up related to my MacPorts port: The list of
> dependencies for notmuch should include "realpath". configure [1] relies
> on realpath to generate the value for rsti_dir.
>

Do you have a suggested replacement?  I guess some inline perl with "use
Cwd 'realpath'" would probably work, although I haven't tested it.

Re: Dependencies should include "realpath"

[Ralph Seichter]

* David Bremner:

> Do you have a suggested replacement? I guess some inline perl with "use
> Cwd 'realpath'" would probably work, although I haven't tested it.

At a quick glance, that particular section of "configure" is run by
doc/conf.py to generate three lines of Python code and store the result
as sphinx.config, correct? If so, my preferred choice would be to use
Python to figure out the absolute path, e.g. like so:

  rsti_dir = os.path.abspath('emacs')

This shows the generated result, and I assume that emacs is a directory
in the source tree? I also wonder if an absolute directory path is really
required for the doc-build to work.

The segment of conf.py which uses the generated config file does not
look convincing to me anyway. Apparently the original author did not
like it either, which is why the segment is labelled as "hacky". It
should probably be overhauled, and not only because it uses the
statement open(rsti_dir+'/'+file) which will potentially fail, depending
on the build platform.

-Ralph

Re: Dependencies should include "realpath"

[David Bremner]

Ralph Seichter <ralph@ml.seichter.de> writes:

> * David Bremner:
>
>> Do you have a suggested replacement? I guess some inline perl with "use
>> Cwd 'realpath'" would probably work, although I haven't tested it.
>
> At a quick glance, that particular section of "configure" is run by
> doc/conf.py to generate three lines of Python code and store the result
> as sphinx.config, correct? If so, my preferred choice would be to use
> Python to figure out the absolute path, e.g. like so:
>
>   rsti_dir = os.path.abspath('emacs')
>
> This shows the generated result, and I assume that emacs is a directory
> in the source tree? I also wonder if an absolute directory path is really
> required for the doc-build to work.

Someone (TM) would have to check that this version did not break
out-of-tree builds. Other than that it seems plausible.

> The segment of conf.py which uses the generated config file does not
> look convincing to me anyway. Apparently the original author did not
> like it either, which is why the segment is labelled as "hacky". It
> should probably be overhauled, and not only because it uses the
> statement open(rsti_dir+'/'+file) which will potentially fail, depending
> on the build platform.

It's somewhat orthogonal to this discussion, but I don't object to a
patch replacing the hardcoded '/' with the appropriate python path
manipulation code. I think windows compatibility for notmuch has much
bigger issues, but it doesn't hurt to clean up the code.

Other than that, I'm not sure what an "overhaul" would involve.

d

Re: Dependencies should include "realpath"

[Ralph Seichter]

* David Bremner:

> I'm not sure what an "overhaul" would involve.

Looking at conf.py, I find the following confusing:

  lines = ['.. include:: /../emacs/rstdoc.rsti\n\n'] # in the source tree
  for file in ('notmuch.rsti', 'notmuch-lib.rsti', 'notmuch-show.rsti', 'notmuch-tag.rsti'):
      lines.extend(open(rsti_dir+'/'+file))
  rst_epilog = ''.join(lines)
  del lines

"lines" is of type List[str]. In a loop which uses "file" (a reserved
expression), open() returns file handles on success. The string (!) list
is then extended with file handles, and after the loop, members of the
list (one string, n file handles) are concatenated using an empty string.
What is the reasoning behind this code segment?

As for generating a config file with configure and then reading and
executing individual lines from Python: Why not write to doc/dyncf.py 
and use "from .dyncf import tags, rsti_dir" in conf.py?

Maybe I am off on a tangent, though. It would help me if I knew what
problem you were actually trying to solve?`

-Ralph

Re: Dependencies should include "realpath"

[Tomi Ollila]

On Sat, Oct 17 2020, Ralph Seichter wrote:

> * David Bremner:
>
>> Do you have a suggested replacement? I guess some inline perl with "use
>> Cwd 'realpath'" would probably work, although I haven't tested it.
>
> At a quick glance, that particular section of "configure" is run by
> doc/conf.py to generate three lines of Python code and store the result
> as sphinx.config, correct? If so, my preferred choice would be to use
> Python to figure out the absolute path, e.g. like so:
>
>   rsti_dir = os.path.abspath('emacs')

Good suggestion, anyway, the simplest change would be just:

-    printf "rsti_dir = '%s'\n" $(realpath emacs)
+    printf "rsti_dir = '%s'\n" $(cd emacs && pwd -P)

> This shows the generated result, and I assume that emacs is a directory
> in the source tree? I also wonder if an absolute directory path is really
> required for the doc-build to work.

Absolute path is safer when doing out-of-tree builds

> The segment of conf.py which uses the generated config file does not
> look convincing to me anyway. Apparently the original author did not
> like it either, which is why the segment is labelled as "hacky". It
> should probably be overhauled, and not only because it uses the
> statement open(rsti_dir+'/'+file) which will potentially fail, depending
> on the build platform.

Sure, the doc/conf.py is somewhat hacky, but has done the work for couple
of years already :D (not that anyone who would like to make is better would
not be welcome to do so). Then, just for the record, I think 
open(rsti_dir+'/'+file) is fine, and I don't see it failing on any
imaginable system notmuch work (now?;) -- I am even personally changing
some os.path.join(...) commands to use that concatenation instead, just
to reduce complexity and line count elsewhere...

Tomi

>
> -Ralph

Re: Dependencies should include "realpath"

[Ralph Seichter]

* Tomi Ollila:

>> rsti_dir = os.path.abspath('emacs')
>
> Good suggestion, anyway, the simplest change would be just:
>
> -  printf "rsti_dir = '%s'\n" $(realpath emacs)
> +  printf "rsti_dir = '%s'\n" $(cd emacs && pwd -P)

Looks like "pwd -P" is part of the Open Group base spec for pwd, so it
should be available on supported platforms.

> Then, just for the record, I think open(rsti_dir+'/'+file) is fine,
> and I don't see it failing on any imaginable system notmuch work
> (now?;)

As they say, you have the right to your opinion. ;-) I don't think it is
"fine" at all. While rsti_dir+'/'+file may work, depending on the
platform used, os.path.join(rsti_dir, file) *will* work.

Quoting Python's "os" description, first sentence: "This module provides
a portable way of using operating system dependent functionality." I see
no viable reason not to use "os".

> I am even personally changing some os.path.join(...) commands to use
> that concatenation instead, just to reduce complexity and line count
> elsewhere...

I am sorry to hear that. There is nothing complex about using os.path,
and as far as I am concerned, correctness and ease of maintenance beats
a reduced line count every single time.

-Ralph

Re: Dependencies should include "realpath"

[Tomi Ollila]

On Tue, Oct 20 2020, Ralph Seichter wrote:

> * Tomi Ollila:
>
>>> rsti_dir = os.path.abspath('emacs')
>>
>> Good suggestion, anyway, the simplest change would be just:
>>
>> -  printf "rsti_dir = '%s'\n" $(realpath emacs)
>> +  printf "rsti_dir = '%s'\n" $(cd emacs && pwd -P)
>
> Looks like "pwd -P" is part of the Open Group base spec for pwd, so it
> should be available on supported platforms.

oh, you're right, I remembered configure was bash(1) script, but, it
has #!/bin/sh -- (although "traditional" bourne shell would not do it)

Thanks for checking!

(dropped our conversation about use of os.path... :D)

>
> -Ralph

Tomi