Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix #6583, fix #14376, index+search now generated for all projects, many bug fixes with nim doc #14324

Merged
merged 9 commits into from
May 25, 2020

Conversation

timotheecour
Copy link
Member

@timotheecour timotheecour commented May 12, 2020

summary: lots of bug fixes, nim doc now directly generates index and search for user projects; saner semantics for default location where docs are generated

example

git clone https://github.com/nim-lang/Nim
nim doc --project --index --docroot -r Nim/compiler/nim
Hint: 198847 LOC; 7.538 sec; 438.93MiB peakmem; Debug build; proj: Nim/compiler/nim; out: /Users/timothee/.cache/nim/nim_d/htmldocs/compiler/nim.html [SuccessX]

this creates and opens compiler docs (with an index) in your browser on the fly in 6s and doesn't clobber sources files nor current dir

details

  • fix doc2 outputs in current work dir #6583 as follows: all doc-like commands now output to $nimcache/htmldocs unless --outdir is provided
    this avoids clobbering current dir as well as sources, which was a common annoyance in particular with --project. This was already inconsistent with other commands before this PR, but I chose this semantics over outputting next to sources because nim doc is special: it installs many other things (eg nimdoc.out.css, idx files, html files, and subdirs) in particular with --project

  • fix nim doc recursive should allow specifying a directory and flattened structure #14376 (in particular all items in nim doc recursive should allow specifying a directory and flattened structure #14376 (comment)) before PR, nim doc --project was inconsistant with nim doc (files were output next to their sources) which also was a regression from a prior time; now it's consistent with nim doc and outputs by default under $nimcache/htmldocs.
    $nimcache/htmldocs is a better default than $PWD/htmldocs as it avoids clobbering your current directory in particular if you run commands from different dirs.

In particular, if you just want to see docs, you can do: nim doc -r main, and all is kept in nimcache (but main html file is shown with successX msg as with other commands)

  • nim buildIndex -o:htmldocs/theindex.html is now done automatically with nim doc --project --index main (more DRY, and avoids an extra step)

  • fix a recent regression introduced sometime after fix #13150 nim doc --project now works reliably #13223 where --docroot would cause nimdoc.out.css to be misplaced

  • fix a bug where nim doc --docroot:$PWD main would give wrong relative paths if $PWD is a symlink (eg common on OSX, where after cd /tmp/ you'd have since problem since /tmp is a symlink to /private/tmp on OSX)

  • dochack.js is now installed by default when you use --project so that search works by default; it gets compiled (once) if it doesn't exist

  • updated docgen which didn't reflect reality (even before this PR)

  • theindex.html is now shown by default as an index entry, making it easy to navigate across pages in docs

  • I added regression tests testing a number of configurations to avoid more regressions in doc generation (see trunner.nim)

for future PR

@timotheecour timotheecour force-pushed the pr_fix_6583_nim_doc_dir branch from 7aa3e04 to ca4fcad Compare May 12, 2020 16:27
changelog.md Outdated Show resolved Hide resolved
@timotheecour timotheecour force-pushed the pr_fix_6583_nim_doc_dir branch 2 times, most recently from ebbab45 to b54318e Compare May 13, 2020 09:35
@timotheecour timotheecour force-pushed the pr_fix_6583_nim_doc_dir branch 2 times, most recently from 62d73fe to d7198ee Compare May 20, 2020 06:52
@Clyybber
Copy link
Contributor

Clyybber commented May 20, 2020

fix #6583 as follows: all doc-like commands now output to $nimcache/htmldocs unless --outdir is provided

Thats going to break a ton of scripts and is not any more so consistent with the way output binaries are generated, IMO its just surprising and inconvinient behaviour.

@timotheecour
Copy link
Member Author

timotheecour commented May 20, 2020

The only point of contention with this PR (IMO, please clarify if not) is the default choice for outdir when --outdir is omitted (this PR also fixes other issues with or without --outdir).

Everything below is in reference to --outdir being omitted.

Note that it's a small change in this PR to add some kind of fallback compatibility behavior (-d:nimDeprecatedDocOutput), and I can add that if needed but it's not even clear what that fallback behavior should be, in light of what follows below.

Please make sure you understand the problem before jumping to conclusions, I'm not sure you know the current and past behavior of nim doc without --outdir and I wouldn't blame you because it makes no sense. In fact the only thing common every single past release has in common in that respect is that they've all been equally broken and inconsistent in their own, unique ways.

  • this was never in test suite until this PR
  • the behavior has kept changing over time (as direct consequence of not being tested), from 0.17=>0.18=>0.19=>0.20=>1.2.0
  • nim doc --project and nim doc have never been consistent with each other nor with other commands nor with the docs.
  • docs in devel don't suggest nim doc is consistent with other commands, they reflect something else (and they also don't reflect reality)
  • above all else, current behavior is downright bad, for reasons I gave in top post: nim doc, unlike all other commands, generates a lot of auxiliary files that not only clutter your project but could overwrite existing files; on top of that, redundant nimdoc.out.css entries are generated for a single --project command

Thats going to break a ton of scripts

I doubt it, the last breaking change to date in how nim doc without --outdir works was with 1.2 release, just 1 month ago

Since behavior without --outdir has always be broken and keeps changing at every release, ppl would use explicit --outdir if their script actually relies on the generated html/idx/css files; in particular current behavior with --project is downright harmful

just surprising and inconvinient behaviour

no it's useful, it's the same rationale as why nimcache is now by default in a fixed place instead of relative to pwd or project location; it avoids cluttering your pwd or project sources with files when you just want to check that nim doc works and look it's output; I already explained why nim doc is different from other commands.

It also makes nim doc -r main (with or without --project) actually useful and clutter-free (see example I added above with single cmd to generate docs + index + opening in browser for nim compiler)

If you have a better behavior in mind, please suggest in detail how it would work (with and without --project)

docs in devel:

delete any htmldocs/*.idx file before starting
nim doc --project --index:on --git.url:<url> --git.commit:<tag> <main_filename>.nim
nim buildIndex -o:htmldocs/theindex.html htmldocs

=> this implies nim doc --project outputs to htmldocs, which is neither true, nor consistent with nim doc, nor consistent with the way output binaries are generated

current design is not the result of some "intended design", it's just result of regressions after regressions, eg:

#6583, #14376

in all cases below, I'm using the same sample project (see below)

nim doc --index:on --skipusercfg --skipparentcfg src/sub/fun2.nim

  • $nim_0170_X
    fun2.idx
    src/sub/fun2.html

  • $nim_0180_X
    fun2.html
    fun2.idx

  • $nim_0190_X
    src/sub/fun2.html
    src/sub/htmldocs/fun2.idx

  • $nim_0200_X
    fun2.html
    src/sub/htmldocs/fun2.idx

  • $nim_120_X
    fun2.html
    nimdoc.out.css
    src/sub/fun2.idx

nim doc --project --index:on --skipusercfg --skipparentcfg src/sub/fun2.nim

  • $nim_0170_X
    Error: cannot open 'htmldocs/Users/timothee/temp/D20200518T234820/d01/d02/d03/src/sub/fun2.html'

  • $nim_0180_X
    Warning: cannot open 'htmldocs/Users/timothee/temp/D20200518T234820/d01/d02/d03/src/fun1.html' [CannotOpenFile]
    Warning: cannot open 'htmldocs/bar/fun3.html' [CannotOpenFile]
    fun1.idx
    fun2.idx
    fun3.idx
    htmldocs/fun2.html

  • $nim_0190_X
    src/sub/fun1.html
    src/sub/fun1.idx
    src/sub/htmldocs/bar/fun3.html
    src/sub/htmldocs/bar/fun3.idx
    src/sub/htmldocs/fun2.html
    src/sub/htmldocs/fun2.idx

  • $nim_120_X
    @@/fun1.html
    @@/fun1.idx
    @@/nimdoc.out.css
    bar/fun3.html
    bar/fun3.idx
    bar/nimdoc.out.css
    fun2.html
    fun2.idx
    nimdoc.out.css

(the @@ was introduced in #13223 and (among other fixes) made sure that generated docs don't escape $outdir, whatever $outdir turns out to be, so that when $outdir is implicitly pwd, it's at least consistent in that respect)

sample project used for all above commands:

just 3 files, each exporting 1 symbol; with main file importing above and below its level:

# src/fun1.nim
proc fun1fn* = discard

# src/sub/bar/fun3.nim
proc fun3fn* = discard

# src/sub/fun2.nim
import ../fun1
import bar/fun3
proc fun2fn* = discard

@timotheecour timotheecour marked this pull request as ready for review May 20, 2020 23:44
@timotheecour timotheecour force-pushed the pr_fix_6583_nim_doc_dir branch 2 times, most recently from 3d0b133 to 781895a Compare May 21, 2020 00:06
@timotheecour timotheecour changed the title WIP fix #6583, fix #14376, index now generated for all projects, many bug fixes with nim doc May 21, 2020
@timotheecour timotheecour changed the title fix #6583, fix #14376, index now generated for all projects, many bug fixes with nim doc fix #6583, fix #14376, index+search now generated for all projects, many bug fixes with nim doc May 21, 2020
@Araq
Copy link
Member

Araq commented May 21, 2020

In fact the only thing common every single past release has in common in that respect is that they've all been equally broken and inconsistent in their own, unique ways.

Well that's not only "regressions because untested", it's also the result of a combinational command line switch explosion, and you yourself are too quick to introduce always more switches. (And putting the swiches into a hierarchy doesn't change the underlying maths.)

@timotheecour
Copy link
Member Author

timotheecour commented May 21, 2020

just to be clear, I was passing no judgement on quality of past releases, which are excellent; I'm only referring to the narrow problem at hand: behavior when --outdir is unspecified (emphasis: has in common in that respect [...]).

it's also the result of a combinational command line switch explosion.. a hierarchy doesn't change the underlying maths

it's not so much the number of flags as it is how flags interact; N flags with orthogonal behavior needs O(N) testing; the difficulty is for flags with complex interactions with other flags like --outdir -o --project, which need to be tested in combination. Hierarchy does help with reducing interactions and discoverability.

We shouldn't introduce flags that don't pull their weight, sure; but I use these all the time, eg --outdir (which I suggested in #9513), --backend:x, nim r main, and every time I'm debugging --hint:msgorigin, --stacktracemsg + others.

In fact I'm considering after this PR turning --index:on into a noop (on by default) since this PR enables index for user projects, so there's really no point in skipping the index, which is cheap to compute and always useful.

FWIW, git shows it's possible to have a very large number of command line flags, yet be well tested and usable by the masses; most flags are just there "in case you need it" but not needed for basic operations; uniform design and yes, hierarchy, makes them discoverable when you need them.

At least we have nims, which is far far better than any config system I've seen (nims eats its own dogfood, so improvements in VM improves nims instead of being something entirely separate like cfg)

@Araq
Copy link
Member

Araq commented May 21, 2020

In fact I'm considering after this PR turning --index:on into a noop (on by default) since this PR enables index for user projects, so there's really no point in skipping the index, which is cheap to compute and always useful.

Ok, maybe.

We shouldn't introduce flags that don't pull their weight, sure; but I use these all the time, eg --outdir (which I suggested in #9513), --backend:x, nim r main, and every time I'm debugging --hint:msgorigin, --stacktracemsg + others.

That's fine but hopefully we're slowly getting to a point where you are happy with the switches we offer.

@alaviss
Copy link
Collaborator

alaviss commented May 21, 2020

Since behavior without --outdir has always be broken and keeps changing at every release, ppl would use explicit --outdir if their script actually relies on the generated html/idx/css files; in particular current behavior with --project is downright harmful

just surprising and inconvinient behaviour

no it's useful, it's the same rationale as why nimcache is now by default in a fixed place instead of relative to pwd or project location; it avoids cluttering your pwd or project sources with files when you just want to check that nim doc works and look it's output; I already explained why nim doc is different from other commands.

Isn't this just because they are temporaries that you should not care about? I do care about my docs, so why should they be generated somewhere outside of my project directory?

Instead, make it so that it will always output to a htmldocs folder relative to the project file when --project is used. This is how we do it with binaries by default, then we should just replicate it with docs.

For when --project is not used, just output the html next to the .nim file. This is the standard behavior for all of our other commands.

@timotheecour timotheecour force-pushed the pr_fix_6583_nim_doc_dir branch from f1e53ba to c7fcef3 Compare May 23, 2020 17:33
@timotheecour
Copy link
Member Author

timotheecour commented May 23, 2020

@Araq PTAL

outdir when --outdir not explicitly specified

(of course, specifying --outdir will override all of this so commands with explicitly set --outdir won't be affected)

when outdir is not provided, for a nim doc command it now resolves to:

if --usenimcache: $nimcache/htmldocs
else: $projectPath/htmldocs

the only difference with what @alaviss described is that I'm using $projectPath/htmldocs even for --project:off, instead of $projectPath
IMO it's more consistent than to have a different behavior with or without --project:on|off, and behaves better wrt source control: you only need htmldocs in your gitignore, and removing or moving all doc generated files is easier than if you had doc generated files output directly to $projectPath/; changing this would be easy (1 line change + test fix) but unless I'm missing something this would be worse.

doc generated auxiliary files can be considered as implementation details subject to change (in particular dochack.js, css, idx, theindex.html) so best to just encapsulate those inside a htmldocs dir so users don't have to worry about what to gitignore

--index:on now implied by --project

that's in the same vein, this is to provide something that "just works" with the simplest command (nim doc --project main), which now generates index and search as default

potential future work

idx should IMO be replaced by json instead of this adhoc format, it'd help avoid bugs like #14434 and be generally simpler; also js can parse json directly so it'd open up frontend customization

likewise, dochack.js should IMO be replaced by karax at some point (would be more responsive as single page app and result in cleaner implementation vs current hand-written html template magic) and the change should be transparent to users (which is why I think it's a good idea to simplify the doc generation command to avoid exposing internal implementation details)

@Araq Araq merged commit 5828254 into nim-lang:devel May 25, 2020
@timotheecour timotheecour deleted the pr_fix_6583_nim_doc_dir branch May 25, 2020 12:04
EchoPouet pushed a commit to EchoPouet/Nim that referenced this pull request Jun 13, 2020
… all projects, many bug fixes with nim doc (nim-lang#14324)

* refs nim-lang#6583 fix nim doc output
* changelog
* change default for outDir when unspecified
* cleanups
* --project implies --index
@a-mr a-mr mentioned this pull request Feb 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants