unofficial mirror of notmuch@notmuchmail.org
 help / color / mirror / Atom feed
* Dependencies should include "realpath"
@ 2020-10-16 20:42 Ralph Seichter
  2020-10-17  0:18 ` David Bremner
  0 siblings, 1 reply; 8+ messages in thread
From: Ralph Seichter @ 2020-10-16 20:42 UTC (permalink / raw)
  To: notmuch

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Dependencies should include "realpath"
  2020-10-16 20:42 Dependencies should include "realpath" Ralph Seichter
@ 2020-10-17  0:18 ` David Bremner
  2020-10-17  1:15   ` Ralph Seichter
  0 siblings, 1 reply; 8+ messages in thread
From: David Bremner @ 2020-10-17  0:18 UTC (permalink / raw)
  To: Ralph Seichter, notmuch

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.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Dependencies should include "realpath"
  2020-10-17  0:18 ` David Bremner
@ 2020-10-17  1:15   ` Ralph Seichter
  2020-10-17 11:18     ` David Bremner
  2020-10-19 18:48     ` Tomi Ollila
  0 siblings, 2 replies; 8+ messages in thread
From: Ralph Seichter @ 2020-10-17  1:15 UTC (permalink / raw)
  To: notmuch

* 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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Dependencies should include "realpath"
  2020-10-17  1:15   ` Ralph Seichter
@ 2020-10-17 11:18     ` David Bremner
  2020-10-17 23:08       ` Ralph Seichter
  2020-10-19 18:48     ` Tomi Ollila
  1 sibling, 1 reply; 8+ messages in thread
From: David Bremner @ 2020-10-17 11:18 UTC (permalink / raw)
  To: Ralph Seichter, notmuch

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Dependencies should include "realpath"
  2020-10-17 11:18     ` David Bremner
@ 2020-10-17 23:08       ` Ralph Seichter
  0 siblings, 0 replies; 8+ messages in thread
From: Ralph Seichter @ 2020-10-17 23:08 UTC (permalink / raw)
  To: notmuch

* 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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Dependencies should include "realpath"
  2020-10-17  1:15   ` Ralph Seichter
  2020-10-17 11:18     ` David Bremner
@ 2020-10-19 18:48     ` Tomi Ollila
  2020-10-20 14:54       ` Ralph Seichter
  1 sibling, 1 reply; 8+ messages in thread
From: Tomi Ollila @ 2020-10-19 18:48 UTC (permalink / raw)
  To: notmuch

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Dependencies should include "realpath"
  2020-10-19 18:48     ` Tomi Ollila
@ 2020-10-20 14:54       ` Ralph Seichter
  2020-10-20 15:06         ` Tomi Ollila
  0 siblings, 1 reply; 8+ messages in thread
From: Ralph Seichter @ 2020-10-20 14:54 UTC (permalink / raw)
  To: notmuch

* 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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Dependencies should include "realpath"
  2020-10-20 14:54       ` Ralph Seichter
@ 2020-10-20 15:06         ` Tomi Ollila
  0 siblings, 0 replies; 8+ messages in thread
From: Tomi Ollila @ 2020-10-20 15:06 UTC (permalink / raw)
  To: Ralph Seichter, notmuch

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2020-10-20 15:06 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-16 20:42 Dependencies should include "realpath" Ralph Seichter
2020-10-17  0:18 ` David Bremner
2020-10-17  1:15   ` Ralph Seichter
2020-10-17 11:18     ` David Bremner
2020-10-17 23:08       ` Ralph Seichter
2020-10-19 18:48     ` Tomi Ollila
2020-10-20 14:54       ` Ralph Seichter
2020-10-20 15:06         ` Tomi Ollila

unofficial mirror of notmuch@notmuchmail.org

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://yhetil.org/notmuch/0 notmuch/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 notmuch notmuch/ https://yhetil.org/notmuch \
		notmuch@notmuchmail.org
	public-inbox-index notmuch

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.yhetil.org/yhetil.mail.notmuch.general
	nntp://news.gmane.io/gmane.mail.notmuch.general


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git