* 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
Code repositories for project(s) associated with this public inbox https://yhetil.org/notmuch.git/ This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).