-
Notifications
You must be signed in to change notification settings - Fork 132
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
Added Installation guide for Gitlab Pages #597
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update!
I've left a few comments and questions.
doc/usage.ipynb
Outdated
"\n", | ||
"before_script:\n", | ||
" - apt-get update -y\n", | ||
" - apt-get install -y pandoc\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I normally like to add the --no-install-recommends
, which might speed up the installation.
I've also just added this to nbsphinx
's own CI: #598.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good!
But when the new nbspinx version will have it on default, will " - apt-get install -y pandoc\n",
be needed at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand. It will still need pandoc
(at least until #36 is done).
doc/usage.ipynb
Outdated
"pages:\n", | ||
" stage: deploy\n", | ||
" script:\n", | ||
" - pip install numpy ipykernel\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about recommending the usage of a doc/requirements.txt
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer it like this, then it is ready for copy+paste
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, fair enough.
doc/usage.ipynb
Outdated
" stage: deploy\n", | ||
" script:\n", | ||
" - pip install numpy ipykernel\n", | ||
" - pip install sphinx nbsphinx\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sphinx is a dependency of nbsphinx
, so it's not strictly necessary.
But if you prefer to mention it, that's fine.
doc/usage.ipynb
Outdated
" script:\n", | ||
" - pip install numpy ipykernel\n", | ||
" - pip install sphinx nbsphinx\n", | ||
" - mkdir -p doc; cd doc\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is that supposed to mean?
Is there no doc
directory yet?
Where is the source directory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw this somewhere on the internet and as it was working, I just applied it without spending much thought on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I don't understand how this can work.
According to the sphinx-build
call below, your source files are in doc/source
but why are you creating the doc
directory? It should exist already?
doc/usage.ipynb
Outdated
" - pip install numpy ipykernel\n", | ||
" - pip install sphinx nbsphinx\n", | ||
" - mkdir -p doc; cd doc\n", | ||
" - sphinx-build -b html source ../public/\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this will create a ../public/.doctrees
directory which is not supposed to be published.
I think it would be good to use the -d
to put the doctrees somewhere else.
Or are hidden directories ignored by Gitlab Pages anyway?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, this is only copy-pasted form some internet forums, I don't know about the details
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fine, but do you know if the .doctree
directory is served by Gitlab pages?
Do you have an example project where I can have a look?
Thanks for your review! |
Co-authored-by: Matthias Geier <Matthias.Geier@gmail.com>
Thanks @mgeier for your in-depth review! |
I did some experiments with Docker and Gitlab and came up with this example image: python:3-slim
variables:
PIP: python3 -m pip
SPHINX: python3 -m sphinx -W --keep-going --color
build-docs:
stage: build
script:
- apt-get update -y
- apt-get install -y --no-install-recommends pandoc
- $PIP install -r doc/requirements.txt
- $SPHINX -d doctrees doc html -b html
- $SPHINX -d doctrees doc linkcheck -b linkcheck
artifacts:
when: always
paths:
- html
- linkcheck/output.*
pages:
stage: deploy
variables:
GIT_STRATEGY: none
script:
- mv html public
artifacts:
paths:
- public
rules:
- if: $CI_COMMIT_REF_NAME == $CI_DEFAULT_BRANCH For this to work, the file IMHO, this has a few advantages:
What do you think about that? |
Hi @mgeier , |
Thanks for trying this out @kolibril13! As you have seen, the Your second error is just a warning-turned-into-an-error. If you don't want this to happen, you can remove the
There are two ways to fix this: either you remove the unused setting |
Nice, that fixed it! :) Thanks for taking time and writing the GitLab script, I've just updated this pr with your script. |
Sorry for the long delay, I've merged your PR now. |
Not problem! |
I forgot to squash your commits, I've done that now in 8f6343a. |
Again, same as #590