Status

Add-on SDK
Documentation
P2
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: wbamberg, Assigned: wbamberg)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

6 years ago
The docs server adds complexity to the SDK, and has some usability problems. There was a discussion thread in the group: http://groups.google.com/group/mozilla-labs-jetpack/browse_thread/thread/79749e351d3e8cf0 the outcome of which was still to generate the docs locally, but to do it using the cfx sdocs code to build a set of static files, rather than running a docs server to generate the docs dynamically.
(Assignee)

Comment 1

6 years ago
Created attachment 547579 [details] [diff] [review]
Implement cfx docs in terms of cfx sdocs

You can also find this at https://github.com/wbamberg/jetpack-sdk/tree/static-docs if that works better.

The patch is big, but that's mostly because I moved all the docs scripts into their own directory under /docs.

What I've done:

1) made a new script called 'generate.py'. This mostly replicates the old generate_static_docs function out of server.py, though I've refactored it a bit. generate_static_docs calls generate_docs and then zips up the result (I think these functions could possibly do with some renaming).

generate_docs has the following logic:
* if the addon-sdk-docs directory doesn't exist, generate everything
* if it does exist, compute a hash over the names and last modification times on every .md file under packages, and every non-hidden file under static-files (because static-files also includes png, css, js &c). If the hash doesn't match the hash under addon-sdk-docs, regenerate everything.
* if you generate everything, store the hash for later reference
* otherwise, do nothing

2) in cuddlefish's __init.py__, implement cfx docs by calling generate_docs and launching the browser

It's pretty simple. In particular, we don't selectively regenerate files: if anything's changed, we regenerate everything. The idea is to optimize for (a) the case where there are no docs (the user has just downloaded the SDK and (b) the case there the docs haven't changed. These are the cases most likely to be encountered by users. This makes it a bit painful if you're repetitively changing a single docs file, and we could address that in a couple of ways if it's important. However, I think even when it regenerates everything it's not intolerably slow, and a nice side-benefit of this is that case (b) (the most common case of all, for users) is much faster than before.

What I'm most concerned about is the possibility of the state getting messed up in such a way that the docs aren't usable, but that the code thinks they don't need to be regenerated. I can't see anywhere that would happen though. I did wonder if it would be good to try regenerate inside a trap, and if it throws, rmdir and generate_from_scratch.

After this patch, I have no use for server.py any more at all! It is used for something else, which I've never understood (perhaps linked to cfx develop?) so I've left the code in there, but if we don't need it for that other stuff we can remove it.

I also removed the ping code from main.js, as it's also not needed any more.
Assignee: nobody → wbamberg
Attachment #547579 - Flags: review?(warner-bugzilla)
Attachment #547579 - Flags: feedback?(myk)
I like it! Some quick thoughts before I run to the train.. I'll try to get a
proper review up later tonight:


* generate_static_docs() points at the SDOCS_DIR directory in three different
  ways, and could be cleaned up. First, there's an shutil.rmtree() which
  references SDOCS_DIR. Second, it calls generate_docs() and expects it to
  create the docs in the same place (instead of either passing in the desired
  target directory, or receiving it as an output). Third, we call tgz.add()
  with a hardcoded 'addon-sdk-docs' string and expect it to match the
  previous two. The tgz.add() case is sensitive to the CWD of the process:
  the other two only pay attention to $CUDDLEFISH_ROOT (via 'env_root').

* as a general style thing, I'd encourage:

   from cuddlefish.docs import generate
   generate.generate_docs()

  or

   from cuddlefish.docs.generate import generate_docs
   generate_docs()

  rather than:

   import cuddlefish.docs.generate
   cuddlefish.docs.generate.generate_docs()

  but it's a pretty minor thing.

* consider passing in a 'stderr' arg, defaulting to sys.stderr, and emitting
  all messages to it (or maybe use sys.stdout). That makes unit tests easier
  to shush (by passing in a StringIO instead of letting it talk to the real
  stdio). See cuddlefish/manifest.py for samples.

* I tend to use f=open();f.write();f.close() instead of relying upon
  auto-close-on-dealloc, but that might just be my personal paranoia.

* we should use the same "is this file interesting" function everywhere: it'd
  be nice to ignore emacs-tempfiles (ending with ~) too, and we do that when
  building XPIs. I think we can extract the function from xpi.py

* as we discussed, it'd be nice if this behaved like a Makefile: only rebuild
  things that have changed. In addition, I think the two code paths (generate
  everything and generate-for-changed) could probably be merged. I'll file a
  different ticket for that. Implementation note to self: make
  calculate_current_status() return a dictionary mapping
  (list-of-pathname-components) keys to (digest, mtime) value tuples, then
  serializing the dictionary into the status file. If the status file doesn't
  exist, the read-old-status function should return an empty dictionary.
  Then, the rebuild() function should walk all local files, compare the
  (digest,mtime) against the current status, rebuild if different, update the
  current status, write back out to status file.

* RE your concern about state getting messed up: what I'd to is not write out
  the status file until after all MD files have been converted successfully.
  That way, if any exceptions occur, a new invocation of 'cfx docs' ought to
  re-render everything. In general, it's better to (clean up at beginning,
  *not* catch exceptions, put the transaction "commit" at the end) than to
  (catch all exceptions, sometimes clean up in the middle, "commit" only if
  no exception happened).
Comment on attachment 547579 [details] [diff] [review]
Implement cfx docs in terms of cfx sdocs

Looks good to me: the comments above would be nice to have, but I'm happy to see this landed without them too.
Attachment #547579 - Flags: review?(warner-bugzilla) → review+
(Assignee)

Comment 4

6 years ago
Thanks for the review Brian! They're really useful comments.

> * as we discussed, it'd be nice if this behaved like a Makefile: only rebuild
>   things that have changed.

Well, sort of. But I think it's worth being clear about the use cases. I can think of about four use cases:

* calling cfx docs when the doc tree is up to date: normal users will do this a lot

* calling cfx docs when the doc tree is empty: normal users will do this when they get a new release

* calling cfx docs after adding a new package: users of third-party modules will do this when they install a new package

* iteratively editing a doc file and reviewing the changes: developers will do this when they make changes to docs (and I will, of course)

The only case that's substantially more painful in this patch than in the current code is the last one[1]. But, makefile-like behavior doesn't sound to me like the right way to address that case. When I iteratively edit an MD file, I don't keep running cfx docs: I keep the server running and reload the page to see the changes. Then the edit-review cycle is just <ctrl-s><alt-tab><f5>.

Makefile-like behavior only makes cfx docs faster: I still have to navigate to the page I care about in the browser. What would work better (ISTM) is to be able to supply a filename to cfx docs, that will cause it to regenerate that one file, and launch the browser with that file loaded. That would be faster than makefile behavior, with fewer user actions needed, and (probably) simpler code.

(What might be better for UX, perhaps, would be for the page itself to detect whether there's a newer version of the doc, and regenerate the HTML before reloading. Then the existing <ctrl-s><alt-tab><f5> cycle still works. But then we need to be able to generate the doc in the browser.)

[1] the third one is also more painful, but (I hope) happens rarely enough to be tolerable
/me nods

Yeah, it might be worth considering a mode for 'cfx docs' (maybe 'cfx docs --server'?) in which the process sticks around, watches all .md files for changes (polling to see when their mtime changes), then re-generates the corresponding .html . That would allow your edit-review cycle to be <ctrl-s><pause><alt-tab><f5>, with maybe one second between polls. Another trick I've used in similar editing-docs tasks is to add an actual Makefile to the top of the tree, use *it* to invoke 'cfx docs' or whatever, and then teach emacs to invoke it each time I save an .md file. Being able to run 'cfx docs FILENAME' (and regenerate a single file) would make that pretty easy.
Priority: -- → P2
Target Milestone: --- → 1.1
(Assignee)

Comment 6

6 years ago
Created attachment 549253 [details] [diff] [review]
Added the ability to generate individual files

So this patch adds the ability to do something like:

cfx docs --docfile="packages/pkg_name/docs/module_name"

...which makes it generate just that file, and run the browser with it loaded.

I also moved the "dev-guide" files out from under "static-docs": this is (to me) a more logical organization (MD files aren't "static", inasmuch as they are source files for HTML generation and not something to be simply copied), and means we can just straight copy everything in static-docs, since nothing there needs to be built, ever.

It would be better still to collect all these static-docs (i.e. HTML templates, JS, CSS, PNGs) under a single top-level directory in the output directory. But that would break the links, and I'm not sure whether it's worth it.
Attachment #547579 - Attachment is obsolete: true
Attachment #547579 - Flags: feedback?(myk)
Attachment #549253 - Flags: review?(warner-bugzilla)
Attachment #549253 - Flags: feedback?(myk)
Comment on attachment 549253 [details] [diff] [review]
Added the ability to generate individual files

Nice feature! A few tiny things I'd suggest:

 * keep the argument order consistent between generate_static_docs,
   generate_docs, generate_named_file, and generate_docs_from_scratch:
    _static_docs      (env_root, base_url)
    _docs             (env_root, filename, base_url, stdout)
    _named_file       (env_root, base_url, filename)
    _docs_from_scratch(env_root, base_url, docs_dir)
   I'd suggest (env_root, base_url, [filename], [docs_dir], [stdout]),
 * use filename=None to mean "build all files", instead of filename=''.
   Minor, but we've got a distinct None type here, feels a bit better to use
   it as such
 * consider using argv[2] instead of --docfile= . It's easier to type, easier
   to script (in a Makefile or something), and.. I dunno, the file to
   translate feels more like an "argument" than an "option", you know? It'll
   probably be called "args[1]" at the point in __init__.py when you invoke
   generate_docs().

But I'm happy with it as-is. r+
Attachment #549253 - Flags: review?(warner-bugzilla) → review+
Oh, one more suggestion: modify .gitignore to ignore addon-sdk-docs/ instead of jetpack-sdk-docs/ . After doing 'cfx docs', a 'git status' should not show a bunch of new "untracked files".
Created attachment 549278 [details] [diff] [review]
delete server.py entirely

I see your patch and raise you one: this one (applied on top of yours) removes the server entirely (server.py, test_server.py, and the code in __init__.py that could invoke it). It also removes the "cfx develop" command, and the --use-server option to 'cfx run'.

Whaddya think?
Attachment #549278 - Flags: review?(wbamberg)
Attachment #549278 - Flags: review?(myk)
(Assignee)

Comment 10

6 years ago
Comment on attachment 549278 [details] [diff] [review]
delete server.py entirely

This looks good to me, as long as we're OK with retiring cfx develop. Of course, this is the payoff of the other stuff.
Attachment #549278 - Flags: review?(wbamberg) → review+
If cfx develop is going away, bug 598723 should probably go away, too.
Comment on attachment 549253 [details] [diff] [review]
Added the ability to generate individual files

Mostly, this is great, so feedback+.  A few thoughts:

1. As Brian says, --docfile= -> argv[2]. (In the future, it would be handy for this command to slurp the rest of the arguments on the command line as names of files to regenerate.)

2. "addon-sdk-" in "addon-sdk-docs" is redundant, given that the directory is inside a directory named "addon-sdk[-VERSION]" by default.

We have a bin/ directory, and we'll probably get a lib/ directory in the near future per discussion about removing packages, which suggests these should go into a doc/ (singular, for consistency) directory.

(I may add a build step to generate this directory so it is present in the release packages.)

Also worth considering: move the source files to doc/, then generate them in the same directory, producing .html files alongside the .md source files while leaving the static files alone, so it isn't necessary to copy the static files from one directory to another, and users who go rooting around can see the connection between the source files and the generated ones.

3. The dev-guide directory at the top level is misleading, since it suggests that users will find a dev guide inside it, whereas they'll actually find the source files for one.

We've previously talked about putting internal files like python-lib and the development-mode and test-harness packages into an "internal" directory, and the dev-guide source files are also internal files (as are the static files), so if we're going to move them (and we aren't going to put them into doc/), then let's move them to an internal/ directory in the process.
Attachment #549253 - Flags: feedback?(myk) → feedback+
Comment on attachment 549278 [details] [diff] [review]
delete server.py entirely

r=myk, although I wish we already had a new supported (or at least experimental) way to reinstall an addon in an existing Firefox session (f.e. a smarter cfx run).  I suppose telling folks to create and install a XPI after each change is reasonable enough until we come up with a better option.
Attachment #549278 - Flags: review?(myk) → review+
(Assignee)

Comment 14

6 years ago
> 
> We have a bin/ directory, and we'll probably get a lib/ directory in the
> near future per discussion about removing packages, which suggests these
> should go into a doc/ (singular, for consistency) directory.
> 
> (I may add a build step to generate this directory so it is present in the
> release packages.)
> 
> Also worth considering: move the source files to doc/, then generate them in
> the same directory, producing .html files alongside the .md source files
> while leaving the static files alone, so it isn't necessary to copy the
> static files from one directory to another, and users who go rooting around
> can see the connection between the source files and the generated ones.

I'm not keen on this, because it makes everything more complicated. When we decide to regenerate, if the generated files are in a dedicated directory, it's simple. If they're not, then we have to understand which files in a directory tree are generated and which aren't (in general, HTML files are generated, but some HTML files are not). We also have to explain this distinction to .gitignore. We also have to delete all the generated files piecemeal (I think).

How about a top-level tree structure like this:

(env_root)
    doc
        dev-guide                # generated HTML dev guide
        packages/<pkg_name>/doc  # generated API docs
        dev-guide-source         # Markdown files for dev-guide 
        static-files             # CSS, JS PNG &c
> I'm not keen on this, because it makes everything more complicated. When we
> decide to regenerate, if the generated files are in a dedicated directory,
> it's simple. If they're not, then we have to understand which files in a
> directory tree are generated and which aren't (in general, HTML files are
> generated, but some HTML files are not). We also have to explain this
> distinction to .gitignore. We also have to delete all the generated files
> piecemeal (I think).

Distinguishing between generated and non-generated HTML files seems pretty easy, given that the former should have MD equivalents.  But explaining the distinction to .gitignore does seem complicated, and deleting generated files is also complicated.  Ok, points well taken.


> How about a top-level tree structure like this:
> 
> (env_root)
>     doc
>         dev-guide                # generated HTML dev guide
>         packages/<pkg_name>/doc  # generated API docs
>         dev-guide-source         # Markdown files for dev-guide 
>         static-files             # CSS, JS PNG &c

It seems strange to enter the doc/ directory and not see a file representing the entry point for the documentation.

I'm not completely familiar with the filesystem layout here, but I would make the web page at http://127.0.0.1:8888/ be the index.html file in the doc/ directory, with a layout something like:

  (env_root)
      doc
          index.html               # entry point
          dev-guide                # generated HTML dev guide
          packages/<pkg_name>      # generated API docs
          static-files             # CSS, JS PNG &c

And then I'd put the dev guide source somewhere else, such as (env_root)/internal/dev-guide-source.  (The build step I mentioned earlier might strip that out.)

Also, the second "doc" in (env_root)/doc/packages/<pkg_name>/doc seems redundant.  I'd leave it off.

Finally, it isn't clear, but is static-files intended to be a source directory of files that the docs generator moves into (env_root)/doc/dev-guide?  If so, I'd put it into dev-guide-source and make the generator copy it over.  Otherwise, I'd blast it into its subdirectories, as it seems like an unnecessary collation of too few subdirectories, i.e.:

  (env_root)
      doc
          index.html               # entry point
          dev-guide                # generated HTML dev guide
          packages/<pkg_name>      # generated API docs
          css                      # stylesheets
          js                       # JavaScript scripts
          media                    # images
(Assignee)

Comment 16

6 years ago
(In reply to Myk Melez [:myk] from comment #15)
> > I'm not keen on this, because it makes everything more complicated. When we
> > decide to regenerate, if the generated files are in a dedicated directory,
> > it's simple. If they're not, then we have to understand which files in a
> > directory tree are generated and which aren't (in general, HTML files are
> > generated, but some HTML files are not). We also have to explain this
> > distinction to .gitignore. We also have to delete all the generated files
> > piecemeal (I think).
> 
> Distinguishing between generated and non-generated HTML files seems pretty
> easy, given that the former should have MD equivalents.  But explaining the
> distinction to .gitignore does seem complicated, and deleting generated
> files is also complicated.  Ok, points well taken.
> 
> 
> > How about a top-level tree structure like this:
> > 
> > (env_root)
> >     doc
> >         dev-guide                # generated HTML dev guide
> >         packages/<pkg_name>/doc  # generated API docs
> >         dev-guide-source         # Markdown files for dev-guide 
> >         static-files             # CSS, JS PNG &c
> 
> It seems strange to enter the doc/ directory and not see a file representing
> the entry point for the documentation.
> 
> I'm not completely familiar with the filesystem layout here, but I would
> make the web page at http://127.0.0.1:8888/ be the index.html file in the
> doc/ directory, with a layout something like:
> 
>   (env_root)
>       doc
>           index.html               # entry point
>           dev-guide                # generated HTML dev guide
>           packages/<pkg_name>      # generated API docs
>           static-files             # CSS, JS PNG &c

Sorry, I didn't mention that, but yes: index.html is directly under doc.
 
> And then I'd put the dev guide source somewhere else, such as
> (env_root)/internal/dev-guide-source.  (The build step I mentioned earlier
> might strip that out.)
> 
> Also, the second "doc" in (env_root)/doc/packages/<pkg_name>/doc seems
> redundant.  I'd leave it off.

I could do this, but it would mean updating every single link to every module doc. Of course it's doable, but it makes for a bigger, more complex patch, and more chance of missing an update and having broken links. My view is that it's not worth doing, but I can happily do it if you disagree.

> Finally, it isn't clear, but is static-files intended to be a source
> directory of files that the docs generator moves into
> (env_root)/doc/dev-guide? 

No, static-files is supposed (in my mind) to be files that are copied as-is from source to destination, with no generation step: for example, JS, CSS, images.

> If so, I'd put it into dev-guide-source and make
> the generator copy it over.  Otherwise, I'd blast it into its
> subdirectories, as it seems like an unnecessary collation of too few
> subdirectories, i.e.:
> 
>   (env_root)
>       doc
>           index.html               # entry point
>           dev-guide                # generated HTML dev guide
>           packages/<pkg_name>      # generated API docs
>           css                      # stylesheets
>           js                       # JavaScript scripts
>           media                    # images

That's what's there in the current patch, more or less. The reason for suggesting a top level static-files is that all those files are just copied from source to destination, with no generation needed. Especially in the case where we're regenerating a single file, we want:

- to leave all the generated files alone (except the one we are regenerating)
- to blow away all the 'static-files', and copy them again from the source dir

If all the static-files are together, this is simple. If they're all in separate top-level dirs, we need something ugly and potentially fragile (for example, it is broken if we add a new generated top-level dir) like this (taken from the current patch):

def copy_static_files(env_root, docs_root):
    for docs_dir in os.listdir(docs_root):
        if docs_dir == "packages" or docs_dir == "dev-guide":
            continue
        if os.path.isdir(os.path.join(docs_root, docs_dir)):
            shutil.rmtree(os.path.join(docs_root, docs_dir))
            shutil.copytree(os.path.join(env_root, "static-files", docs_dir), os.path.join(docs_root, docs_dir));

It's not a big deal, I just wanted to explain the reasoning...
(In reply to Will Bamberg [:wbamberg] from comment #16)
> > Also, the second "doc" in (env_root)/doc/packages/<pkg_name>/doc seems
> > redundant.  I'd leave it off.
> 
> I could do this, but it would mean updating every single link to every
> module doc. Of course it's doable, but it makes for a bigger, more complex
> patch, and more chance of missing an update and having broken links. My view
> is that it's not worth doing, but I can happily do it if you disagree.

Ok, let's leave it be.  We can always change it separately later.


> > Finally, it isn't clear, but is static-files intended to be a source
> > directory of files that the docs generator moves into
> > (env_root)/doc/dev-guide? 
> 
> No, static-files is supposed (in my mind) to be files that are copied as-is
> from source to destination, with no generation step: for example, JS, CSS,
> images.
> 
> > If so, I'd put it into dev-guide-source and make
> > the generator copy it over.  Otherwise, I'd blast it into its
> > subdirectories, as it seems like an unnecessary collation of too few
> > subdirectories, i.e.:
> > 
> >   (env_root)
> >       doc
> >           index.html               # entry point
> >           dev-guide                # generated HTML dev guide
> >           packages/<pkg_name>      # generated API docs
> >           css                      # stylesheets
> >           js                       # JavaScript scripts
> >           media                    # images
> 
>
> That's what's there in the current patch, more or less. The reason for
> suggesting a top level static-files is that all those files are just copied
> from source to destination, with no generation needed.

I suppose my concern is about having top-level directories that aren't user-facing, since their presence may distract and confuse users.  However, this is another issue that we can tackle separately from this bug, given that we currently have a static-files top-level directory, so I'm ok with leaving it there.
(Pushing all open bugs to the --- milestone for the new triage system)
Target Milestone: 1.1 → ---
(Assignee)

Comment 19

6 years ago
Landed in:

https://github.com/mozilla/addon-sdk/commit/6d19ec4b68fd651da5b0163bdc62bc5d6b418f33

and

https://github.com/mozilla/addon-sdk/commit/3b08b3b4ffccfcc16d04e381ccc11607f0f46c4c
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.