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

move css, fonts, js to assets folder; auto install assets folder #19

Merged
merged 41 commits into from
Jun 4, 2021
Merged

Conversation

Clonkk
Copy link
Contributor

@Clonkk Clonkk commented Jun 1, 2021

PR for #12 with auto deploy of assets folder

@Clonkk
Copy link
Contributor Author

Clonkk commented Jun 1, 2021

@pietroppeter
This is a draft because while working on it, I noticed some weird stuff when having 2 nimibook outside of the repo.

When having two books booka and bookb I had reference on booka when working with bookb.
I think some path used in Nimibook gets generated compile time and do not get re-compiled when swapping book, giving you weird side effects (I haven't fully identified yet).

EDIT:

Okay, this was due to the option -f not being passed by the publish proc and me having multiple index.nim files.

IMO, when caching the compiler should be keep information of the path of the file and not just the name... but that's not another issue entirely

nimibook.nimble Outdated Show resolved Hide resolved
@Clonkk
Copy link
Contributor Author

Clonkk commented Jun 1, 2021

Fair enough

@HugoGranstrom
Copy link
Collaborator

It would make sense though to have an explicit assets folder in the future though, which is copied into docs/ on generation of the book 😄 And then we could remove it all

@Clonkk
Copy link
Contributor Author

Clonkk commented Jun 1, 2021

I kept the assets folder I just didn't delete docs but docs/assets instead

@HugoGranstrom
Copy link
Collaborator

Okay, that should work for a while 😄

@Clonkk
Copy link
Contributor Author

Clonkk commented Jun 1, 2021

Other solution would be to have custom static file in a dedicated folder like static remove everything except this.

On another topic, when testing with a dummy book outside of the repo, all the html file are created in the current folder or in book and not in docs.

What am I missing for the html to be generated inside docs ?
I deploy all the assets and all the mustache template file. Is there a config.nims to autodeploy as well ?

@pietroppeter
Copy link
Owner

a few quick notes

  • I would keep the init step which creates/updates the initial structure separate from the build step. After init you commit the assets and they only change if you want to update nimibook
  • the nimble file should be modified using installDirs to keep some folders during installation. Ideally I would create a single init directory that has the initial empty structure with all files, folders and assets needed. init should be called with a "book name" that will use it to initialize some fields
  • ideally I would add in this init folder also the config.nims and gitignore and the rest, basically everything except for stuff contained in book folder (and we should move templates to a template folder and change accordingly nbDoc.templateDirs. The files in init might probably need to be templates themselves (so that you would substitute book name in apprpriate places)
  • probably the missing ingredients for wrong folder output is a nimble file. nimble file is what nimib uses to anchor what the project folder is. even an empty nimble file would work

@Clonkk
Copy link
Contributor Author

Clonkk commented Jun 1, 2021

I would keep the init step which creates/updates the initial structure separate from the build step. After init you commit the assets and they only change if you want to update nimibook

For automated setup from scratch, you will need the explicit init therefore the init will have to be comitted so automated deployment from git clone can work. That's why I think we should init everytime.

What we can do instead is populate assets only and only if it does not exists.
It's what I do with Mustache template that are only copied from if they do not exists.

the nimble file should be modified using installDirs to keep some folders during installation. Ideally I would create a single init directory that has the initial empty structure with all files, folders and assets needed. init should be called with a "book name" that will use it to initialize some fields

installDirswith srcDir="src will cause Nimble to complain on installation (because of nim-lang/nimble#653).

So either

  • we stop using srcDir and flatten the repo (but then nimble will complain about genbook being top level...)
  • We add the folders in src/nimibook/supername and we don't need installDirs -> I vote for this option personnally.

ideally I would add in this init folder also the config.nims and gitignore and the rest, basically everything except for stuff contained in book folder (and we should move templates to a template folder and change accordingly nbDoc.templateDirs. The files in init might probably need to be templates themselves (so that you would substitute book name in apprpriate places)

I can regroup them in a init (or ressources ?) folders init/assets and init/mustache).

probably the missing ingredients for wrong folder output is a nimble file. nimble file is what nimib uses to anchor what the project folder is. even an empty nimble file would work

I don't like the dependence on Nimble. Not all books hould be nimble package IMO. How about using :

template newBookFromToc*(booklabel: string, rootfolder: string, body: untyped): Book =
  initBook(rootfolder)
  static:
    let path_to_rootfolder = getProjectPath() / rootfolder
    putEnv("nimibook_path_to_rootfolder", path_to_rootfolder)

and then :

proc useNimibook*(nbDoc: var NbDoc) =
  # path handling (fix upstream in nimib)
  let
    nbThisFile = changeFileExt(nbDoc.filename.AbsoluteFile, ".nim")
    thisTuple = nbThisFile.splitFile
    pathToRootFolder = getEnv("nimibook_path_to_rootfolder")
    nbThisDir: AbsoluteDir = pathToRootFolder.toAbsoluteDir
    nbHomeDir: AbsoluteDir = nbThisDir / RelativeDir("..") / "docs".RelativeDir
    nbSrcDir = nbThisDir

Also, since we now have explicit path (obtained during the compilation of the user book), we can probably simplify some stuff (because we won't need relativeTo anymore)

@Clonkk
Copy link
Contributor Author

Clonkk commented Jun 1, 2021

Unless I misunderstood somethingI think there's an issue with the init specialization.
Even after removing reference to findNimbleDir in nimibook (and replacing it with a correct path) it still break without nimble file present.

I assume it's because there is still one in nimib that is injected in nbInit but it should be overridden by useNimibook, shouldn't it ?

@pietroppeter
Copy link
Owner

I would keep the init step which creates/updates the initial structure separate from the build step. After init you commit the assets and they only change if you want to update nimibook

For automated setup from scratch, you will need the explicit init therefore the init will have to be comitted so automated deployment from git clone can work. That's why I think we should init everytime.

Automated setup from scratch you mean like in our CI? I might be understanding wrong our CI but I think what is doing is rebasing on main before generating the documentation (the build step). If we commited in main the result of an "update" step (the init but just to update to more recent assets/templates...), I think the CI for gh-pages should be able to incorporate that and do not need an init step.

What I am thinking of is the possible user workflows and their relative frequencies:

  • the user will very frequently want to regenerate a single page (already appearing in the toc). This is currently possible by just running nim r path/to/file.nim (before my last PR this required a separate creation of toc/book.json)
  • the user will rather frequently want to regenerate the whole book (every time it changes the toc or some book configuration). this is currently possible with nimble genbook
  • only occasionally, the user will need to update assets.

What we can do instead is populate assets only and only if it does not exists.
It's what I do with Mustache template that are only copied from if they do not exists.

this would be needed if we decide to go with init every time.

the nimble file should be modified using installDirs to keep some folders during installation. Ideally I would create a single init directory that has the initial empty structure with all files, folders and assets needed. init should be called with a "book name" that will use it to initialize some fields

installDirswith srcDir="src will cause Nimble to complain on installation (because of nim-lang/nimble#653).

So either

  • we stop using srcDir and flatten the repo (but then nimble will complain about genbook being top level...)
  • We add the folders in src/nimibook/supername and we don't need installDirs -> I vote for this option personnally.

I did know installDirs could have issues and I look foward to the frictionless package structure.
I agree with your pick on putting the asset stuff inside a subfolder of src (mdbook does the same btw, there is a theme folder with templates and assets)

ideally I would add in this init folder also the config.nims and gitignore and the rest, basically everything except for stuff contained in book folder (and we should move templates to a template folder and change accordingly nbDoc.templateDirs. The files in init might probably need to be templates themselves (so that you would substitute book name in apprpriate places)

I can regroup them in a init (or ressources ?) folders init/assets and init/mustache).

yes, that would be ok. I would use init/templates instead of init/mustache
for init I am not sure if this is the best name. we could follow the lead of mdbook and use theme also (they do not split templates from assets but I think it is better).

probably the missing ingredients for wrong folder output is a nimble file. nimble file is what nimib uses to anchor what the project folder is. even an empty nimble file would work

I don't like the dependence on Nimble. Not all books hould be nimble package IMO. How about using :

template newBookFromToc*(booklabel: string, rootfolder: string, body: untyped): Book =
  initBook(rootfolder)
  static:
    let path_to_rootfolder = getProjectPath() / rootfolder
    putEnv("nimibook_path_to_rootfolder", path_to_rootfolder)

and then :

proc useNimibook*(nbDoc: var NbDoc) =
  # path handling (fix upstream in nimib)
  let
    nbThisFile = changeFileExt(nbDoc.filename.AbsoluteFile, ".nim")
    thisTuple = nbThisFile.splitFile
    pathToRootFolder = getEnv("nimibook_path_to_rootfolder")
    nbThisDir: AbsoluteDir = pathToRootFolder.toAbsoluteDir
    nbHomeDir: AbsoluteDir = nbThisDir / RelativeDir("..") / "docs".RelativeDir
    nbSrcDir = nbThisDir

Also, since we now have explicit path (obtained during the compilation of the user book), we can probably simplify some stuff (because we won't need relativeTo anymore)

yeah, using the nimble file mechanism is also one of those things I want to rediscuss in nimib (possibly a wrong design decision that we can still reverse).

Something like you have there it is a fine workaround for now (if we could avoid an env variable it would be better but I am not sure how). Note though that nbThisDir is the directory of the current processed nim file (e.g. for basics\plotting.nim is basics and we should not mix it with nbSrcDir).

In summary, agree on most points, only pushing back a little on the init always thing. :)

@HugoGranstrom
Copy link
Collaborator

I'm not a fan of using ENV variables either, feels like some obscure bugs could creep up in the future. Couldn't we just add a field in book.json with the path? The only downside I see is that it has to be regenerated if moved between devices (as CI) but the CI should run it either way.

@Clonkk
Copy link
Contributor Author

Clonkk commented Jun 1, 2021

Automated setup from scratch you mean like in our CI? I might be understanding wrong our CI but I think what is doing is rebasing on main before generating the documentation (the build step). If we commited in main the result of an "update" step (the init but just to update to more recent assets/templates...), I think the CI for gh-pages should be able to incorporate that and do not need an init step.

No, I didn't meant like our CI :). Our CI is based on on github actions and can commit to a branch.

If I want to use Nimibook on a self-hosted server and automate deployment, I need to be able to generate everything from a zip/tarball/clean git clone; using only Bash (whatever scripting language your OS uses).

Conditionnal init that is required but only once make this harder - the workaround would be to manually implement it in each genbook file with when defined(...) .

What I am thinking of is the possible user workflows and their relative frequencies:

  • the user will very frequently want to regenerate a single page (already appearing in the toc). This is currently possible by just running nim r path/to/file.nim (before my last PR this required a separate creation of toc/book.json)
  • the user will rather frequently want to regenerate the whole book (every time it changes the toc or some book configuration). this is currently possible with nimble genbook
  • only occasionally, the user will need to update assets.

If assets are deployed automatically from source then it can only change if you change Nimibook versions or customize it.

So it shouldn't really matter if we re-populate assets (beside maybe the 0.2 milliseconds it takes to copy the folders) in the init everytime - with the condition that the init gets skipped for files that already exists (because customization (or we can put customized stuff into another folder)).

Realistically, I think what will end up happening IMHO is that we will write this in genbook.nim :

if initCond: # or when defined(initCond)
  initBook

To avoid having to modify source code depending on where we are in the deployment process.

So why not incorporate it already in the library ? I really don't see the downsides of it.

yeah, using the nimble file mechanism is also one of those things I want to rediscuss in nimib (possibly a wrong design decision that we can still reverse).

Something like you have there it is a fine workaround for now (if we could avoid an env variable it would be better but I am not sure how). Note though that nbThisDir is the directory of the current processed nim file (e.g. for basics\plotting.nim is basics and we should not mix it with nbSrcDir).

Yeah, the re-working of useNimibook still needs some work, this was just a PoC of a potential solution using getEnv / putEnv.
There's also still a bug I can't quite figure out (even after not using findNimbleDir in ``nimibook) why it still requires a Nimble file, I don't know if it's comes from Nimib or if I missed something.

I'm not a fan of using ENV variables either, feels like some obscure bugs could creep up in the future. Couldn't we just add a field in book.json with the path? The only downside I see is that it has to be regenerated if moved between devices (as CI) but the CI should run it either way.

What's the issue with env variables ? It's not persistent after the program ends and multiple program running in multiple shell concurrently will have their own environment (you can check it with multiple inim running concurrently - they do not modify each other's environment).

@HugoGranstrom
Copy link
Collaborator

HugoGranstrom commented Jun 1, 2021

What's the issue with env variables ? It's not persistent after the program ends and multiple program running in multiple shell concurrently will have their own environment (you can check it with multiple inim running concurrently - they do not modify each other's environment).

My bad then, didn't do my research on what it meant in this context. I thought it was environment variables as in path and JAVA_HOME 🙃 The only situation this wouldn't work is if we generate and publish the toc in different files, but that shouldn't happen :)

I'm ready to take a look at the nimble bug again now

src/nimibook/docs.nim Outdated Show resolved Hide resolved
@pietroppeter
Copy link
Owner

No, I didn't meant like our CI :). Our CI is based on on github actions and can commit to a branch.

If I want to use Nimibook on a self-hosted server and automate deployment, I need to be able to generate everything from a zip/tarball/clean git clone; using only Bash (whatever scripting language your OS uses).

ok, then I did not understand. I created a new RFC #20 for the design of a nimibook cli. With that design, to generate the book starting from scratch from a git clone of an existing repository would require the following commands:

  • nim c nbook
  • nbook init
  • nbook build

What's the issue with env variables ? It's not persistent after the program ends and multiple program running in multiple shell concurrently will have their own environment (you can check it with multiple inim running concurrently - they do not modify each other's environment).

You are right, env variables like that are good. As @HugoGranstrom was mentioning I think putting a path in book.json would also be ok, but I would avoid putting an aboslute path in case. The advantage of using book.json with current structure would be to be able to still run nim r book/basics/plotting.nim and the file is generated (if plotting.nim is not called from genbook it will not use any env variable). With the target structure proposed in #20 there is no particular difference (you would call nbook build basics/plotting.nim). Still since we have already a book.json that is a channel to communicate from main driver to single chapter, I think we could keep using that channel and not use anotehr mechanism.

Note also that in #20 I propose to drop current mechanism in genbook based on compile time switches (which requires to recompile) and propose to pass to a real cli.

@pietroppeter
Copy link
Owner

thanks for the honest and thorough feedback. I will take this into account also when reviewing path handling in nimib.

For nimibook, it is ok to proceed with current PR as it is. I think the missing part was the override mechanism for nbHomeDir (pietroppeter/nimib#53) which has been merged and released in nimib v0.1.3.

@HugoGranstrom
Copy link
Collaborator

In the README I think it's worth noting that if you update the TOC you will have to recompile the CLI. It's obvious if you think about how it works but people don't always think 😛

@Clonkk
Copy link
Contributor Author

Clonkk commented Jun 4, 2021

Good point ! I'll add it.

@Clonkk
Copy link
Contributor Author

Clonkk commented Jun 4, 2021

CI is green, the github page deployed on my fork looks good and everything works locally in my machine !

I think we're good to go :).

EDIT: A second pair on eye to check document.mustache to check that I didn't forget to add assets to any relative path would be useful Ithink; The CI doesn't check for that.

README.md Outdated Show resolved Hide resolved
@Clonkk
Copy link
Contributor Author

Clonkk commented Jun 4, 2021

@pietroppeter I just realized (CI failures related) nimibookCli uses book explicitly.

Basically, this

# var book = ... # Use another variable name instead
var my_super_book = newBookFromToc(...):

nimibookCli

will break the CLI.

So how would you feel aboud newBookFromToc injecting the book variable instead of relying on var book = newBookFromToc() ?

Alternatively, we can pass book as a paramters to nimibookCli(book).

@HugoGranstrom
Copy link
Collaborator

IMO nimibookCli(book) feels more direct and then the user will get a compile error if they forget it

@Clonkk
Copy link
Contributor Author

Clonkk commented Jun 4, 2021

Either solution is fine IMO.
One will look like this :

newBookFromToc(...):
  ...
  
book.git_repository_url = "https://github.com/pietroppeter/nimibook"
nimibookCli

The other will be :

var mybook = newBookFromToc(...):
  ...
  
mybook.git_repository_url = "https://github.com/pietroppeter/nimibook"
nimibookCli(mybook)

I committed solution n°2 for now (to check CI only fails because of that).

@pietroppeter
Copy link
Owner

Either solution is fine IMO.

I committed solution n°2 for now (to check CI only fails because of that).

agree both solution are fine. 2nd is more explicit and ok to proceed with that.

@pietroppeter
Copy link
Owner

I think we are good to go, right? should I merge? 🥁

README.md Show resolved Hide resolved
@pietroppeter
Copy link
Owner

thanks @Clonkk for the work on this and the patience.

🚀 🥳

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants