Write the readme.txt files

[dgutov]

I tested it with a few packages, seems to work, but please don't take my word for it.

[milkypostman]

I will try a run on all the packages tomorrow and see if there are any problems that crop up. If not we'll merge it 👍

[purcell]

Another example of @dgutov kicking ass!

[dgutov]

:)

[milkypostman]

@dgutov could you try building solarized-theme and see what happens for you? For me it asks for a coding system which it shouldn't because buffer-file-coding-system is set to utf-8-unix yet it still asks me for a coding system.

I can circumvent this problem by forcibly setting coding-system-for-write but at this point, can't we do a write-file? Because isn't write-file the same as write-region with first argument nil?

The problematic line is line 453 in the pb/write-readme-file.

[dgutov]

Indeed, I see the problem. write-file does a whole bunch of unnecessary stuff, and doesn't actually fix it.

If I change pb/slurp-file to call insert-file-contents instead of insert-file-contents-literally, though, the problem disappears. Any idea why the code calls the latter function?

[milkypostman]

@dgutov would it be better to set coding-system-for-write?

commit 5065aae16ea56f2f1b765785c7d1a9eab99f0242
Author: Donald Curtis <dcurtis@milkbox.net>
Date:   Sun Nov 4 13:53:07 2012 -0600

    write-region ignores buffer-file-coding-system

    For some reason write-region is ignoring `buffer-file-coding-system'
    when it writes the file and will occasionally ask for a coding system.
    As a fix we force `coding-system-for-write' to the value of
    `buffer-file-coding-system'

diff --git a/package-build.el b/package-build.el
index 57210c0..dffc34e 100644
--- a/package-build.el
+++ b/package-build.el
@@ -447,9 +447,10 @@ When RAW, COMMENTARY is used as is."
             (replace-match ""))))
       (when (re-search-forward "[^ \t\n]" nil t)
         (delete-trailing-whitespace)
-        (write-region nil nil
-                      (expand-file-name (concat package-name "-readme.txt")
-                                        package-build-archive-dir))))))
+        (let ((coding-system-for-write buffer-file-coding-system))
+          (write-region nil nil
+                        (expand-file-name (concat package-name "-readme.txt")
+                                          package-build-archive-dir)))))))

 (defun pb/write-pkg-readme (commentary pkg-cwd package-name)
   "Write the package commentary to the *-readme.txt file.

[milkypostman]

That is what I did to fix the problem.

[milkypostman]

@purcell ping.

any consequence to using insert-file-contents instead of insert-file-contents-literally. Looks like it shouldn't be an issue since we do buffer-substring-no-properties to actually fetch the text with the slurp.

[milkypostman]

Another error I mis-captured last time,

;;; shimbun

github 
emacsmirror/w3m

updating /tmp/melpa/working/shimbun/

Warning (mule): Invalid coding system `ascii' is specified
for the current buffer/file by the :coding tag.
It is highly recommended to fix it before writing to a file.
Really proceed with writing? (yes or no) Wrote /tmp/melpa/packages/shimbun-readme.txt

[dgutov]

Looks like it shouldn't be an issue since we do buffer-substring-no-properties to actually fetch the text with the slurp.

The common and the "literal" versions of the function plainly return different strings, so only one of them should be "correct" one. When -literally is used, with this recipe:

(test-enc :fetcher github :repo "dgutov/test-enc")

when archive-contents is written, it contains this string: "Thomas Fr\303\266ssman for president!". That's probably incorrect.

[milkypostman]

checkout the readme branch. I think it works, will merge tomorrow morning.

[milkypostman]

link to readme branch

[dgutov]

Another error I mis-captured last time, ...

Looks like it's a problem with the file, sort of: https://raw.github.com/emacsmirror/w3m/master/README
At the bottom it specifies ascii, which is not a valid Emacs coding system.

By the way, this shows another small problem: this is not the correct README file for the package. Shimbun doesn't have one, and we're just taking the one for w3m.

[dgutov]

Other than the test-enc case I mentioned, the readme branch works fine for me.

[milkypostman]

I get an error about lm-commentary

[milkypostman]

SO, the problem with shimbun is that shimbun.el is not in valid package.el format. If you load shimbun.el and do (package-buffer-info) it will error because there is no Version specification. This is not a problem for MELPA since we generate our own versions but this is the reason your changes are reverting back to the README file.

[milkypostman]

but the other problem is properly searching for the "README" file. We dealt something similar before where we wanted to find the common root for all files being copied, our solution didn't end up using this approach though.

[dgutov]

I see.

The second problem is what I was referring to. I think we can leave this alone for now, though, until someone specifically asks why their README is not showing.

[milkypostman]

What about the lm-commentary function? Where is that from?

[dgutov]

Sorry, you'll need to elaborate. When do you see the errror?

lm-commentary is from the package lisp-mnt, also used by the function package-buffer-info.

[milkypostman]

hendrix default melpa readme●✭❯ make recipes/test-enc
 • Building recipe test-enc ...
rm -vf ./packages/test-enc-*
emacs --no-site-file --batch -l package-build.el --eval "(package-build-archive 'test-enc)"

;;; test-enc

github 
dgutov/test-enc

updating /Users/dcurtis/src/melpa/working/test-enc/

Symbol's function definition is void: lm-commentary
make: *** [recipes/test-enc] Error 255

[dgutov]

Yes, sorry. In this case package-buffer-info is never called, so list-mnt is not loaded.

dgutov@b6cbe3b

On another note, I seem to have broken this pull request with push -f.

[milkypostman]

Hrm. What does lm-commentary do? I guess I'm not quite sure why this function is reparsing the package file when previously the file is searched for package info? Essentially this should happen as part of out building of pkg-info.

Also do not worry too much about rebuilding a merge request. At this point I will probably modify your original request into a super commit that resolves the whole thing.

[dgutov]

When pkg-info comes from a -pkg.el file, it doesn't have the commentary field.
When there's also no README file around, I open <package>.el and look for the commentary there.

[milkypostman]

@purcell Would like your input on how we should handle detection of README files.

Right now packages don't specify a README file and I don't think that's the right approach.

My idea is to search the directories of all files being included in the package, if those directories contain a README then we include the README from the shallowest path.

[dgutov]

I think the idea is fine, but we should look at the existing recipes. Here's some examples.

These will benefit from this:
https://github.com/mozilla/rust/tree/master/src/etc/emacs
https://github.com/erlang/otp/tree/maint/lib/tools/emacs

These will definitely not:
https://github.com/tkf/emacs-ipython-notebook/tree/master/lisp
https://github.com/davidmiller/pony-mode/tree/master/src

These two are weird:
http://code.google.com/p/go/source/browse#hg/misc/emacs
^-- No README for Emacs anywhere. Only a comment inside go-mode.el (but the file has no "Version" specification), and a general README at the top of the repo for the whole Go language.
https://github.com/pallet/ritz/tree/develop/nrepl/src
^-- There is a README at least mentioning Emacs, but it's one level above the directory with .el files.

To sum up, I think adding a :readme recipe argument is more or less inevitable.

[purcell]

I agree that :readme looks inevitable if we're determined to maximise the use of README files.

My approach would be this, though: if there's a README in a directory no shallower than the shallowest entry in :files, then use it, otherwise use the commentary in the dominant .el file if present. Done. And yes, that would rule out the README file in the ritz example above.

I wouldn't personally bother with :readme at first because I don't think it's worth the complication, and people would eventually use it to specify some marked-up file like README.md.

Just my 2c.

[milkypostman]

I'd rather complicate the code with a better README search algorithm than require more out of recipes. I think :readme is avoidable with a clear set of guidelines for finding the readme. Anything that doesn't fit those rules is packaged so poorly it deserves to not have documentation and we can report people to the package maintainers.

Here are the rules I propose. From highest to lowest priority.

1. Use commentary in the -pkg file or in the main elisp file.
2. Use the README in the shallowest directory of those specified in :files
3. Use the README in the nearest parent directory. Although maybe this should be "repository root" only.

I think this should handle all the cases you specified above as far as I can tell.

Donald

[purcell]

Yeah, exactly; I don't care about the few cases where a reasonable set of rules can't find a README or commentary. And I might even skip your "nearest parent directory" rule. Emacs is a self-documenting editor, after all, so libraries should have documentation in the source code commentary.

[dgutov]

How can a -pkg file can contain commentary?

Regarding 3., I'd like to point out that without it, ein and pony-mode won't have proper readmes either. And, I don't know, they seem packaged well enough to me.

ein.el does have a Commentary section, but it's short and contains a few notes for developers.

But anyway, supporting just 1. and 2. and adding some additional logic later on seems fine to me.

[purcell]

@dgutov Agreed, no point looking in a -pkg.el file.

I reckon we should just roll out 1 first to test the waters, and then follow up with 2 and perhaps 3 when and if they seem helpful.

[milkypostman]

Ok let me take a stab at this and see what I come up with. I'd like to try to simplify the original push a bit.

Donald

On Nov 8, 2012, at 13:55, Steve Purcell notifications@github.com wrote:

@dgutov Agreed, no point looking in a -pkg.el file.

I reckon we should just roll out 1 first to test the waters, and then follow up with 2 and perhaps 3 when and if they seem helpful.

—
Reply to this email directly or view it on GitHub.

[tkf]

EIN author here. I hope melpa supports README in parent (and/or repository root) directory. As EIN has many lisp files, putting files in top directory does not work well. Also, emacs lisp file is not a good format for document and emacs-lisp-mode is not a good mode for writing documents. That's why I prefer writing README, rather than commentary in the top level file. I agree that Emacs is a self-documenting editor, but I believe that does not imply user must open the source file to see the document. I think it is rather about docstring and info manual.

See also: tkf/emacs-ipython-notebook#6

[purcell]

@tkf We're not saying that the full documentation should be in a commentary section inside your main .el file -- just that there should be text there which explains the basic purpose of the package.

People won't use this mechanism to access full documentation for packages; they just use it to decide whether or not to install a particular package.

[tkf]

BTW, how do you find the main file? If package is composed of PACKAGE.el and PACKAGE-*.el files that should not be difficult, but I suppose some packages have different style. For example, you can have PACKAGE.el and test-PACKAGE.el. I guess you need a lot of heuristics, or am I missing something? Anyway, I am just mentioning it.

[purcell]

@tkf The main file is the <packagename>.el file. We make sure the package names match up with the main .el file that users will require.

[tkf]

Also note that many good old "traditional" emacs lisp projects has README at the top + lisp directory style, such as in org-mode, gnus, ess and reftex. It also reflects the repository organization of Emacs itself. I think it worth supporting this style. (tkf/emacs-ipython-notebook#6).

[tkf]

@purcell that's a simple answer! thanks

[purcell]

The more I think about it, the more I believe we should just ignore README files. We're over-thinking all this. The sole purpose of this change will be to make a paragraph of text appear when someone presses "?" in the package list.

[dgutov]

One of the goals I had is to at least recognize the packages already submitted and working in GNU ELPA and Marmalade. And they both read the plain README file.

In Marmalade docs, this is actually the recommended way.

[dgutov]

@milkypostman @purcell Would you like to chip in here? http://lists.gnu.org/archive/html/emacs-devel/2013-08/msg00670.html