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

Update pull_request_template.md #357

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Conversation

florisvdh
Copy link
Member

@florisvdh florisvdh commented Feb 10, 2025

While reviewing #356, the instructions of the PR template for reviewing seem to no longer work to render the site from the GHA artifact, using python -m http.server 8887.

With these changes, it seems to work again though, using python -m http.server 8887.

I did not check Google Chrome though, having Chromium installed instead. @ElsLommelen I think you tested this in the past; can you test whether the new instructions also work with the Google Chrome Web Server app? You can e.g. use the artifact from #356.

EDIT: closes #343.

While reviewing #356, the instructions of the PR template for reviewing seem to no longer work to render the site from the GHA artifact.

With these changes, it seems to work again though, using python -m http.server 8887
@falkmielke
Copy link
Contributor

For me, the preview still works with the described procedure (e.g. from the parent of the extracted tutorial...). Also, localhost is synonymous to 127.0.0.1, and not to 0.0.0.0 (see here).
We should briefly re-attempt this together, @florisvdh, to investigate what caused the unexpected behavior.

@ElsLommelen
Copy link
Contributor

@florisvdh See issue #343. It seems the situation hasn't changed since I wrote this, so I don't know how to deal with it. (I don't have any knowledge of this kind of tools, I used to just follow the provided instructions.)

@florisvdh
Copy link
Member Author

Thanks @ElsLommelen; yeah I saw the same about Chrome OS from Chromium, so it's the same in Chrome then. Good that you made the issue.

Also, localhost is synonymous to 127.0.0.1, and not to 0.0.0.0 (see here)

@falkmielke yes of course, and I first tried that (as in the instructions), but there the CSS was not found (as seen from the console errors) so the website rendered without CSS: this is not the served path. However then I looked better at below output: it literally points to 0.0.0.0 and indeed that did work. But the website only works well if run it in the root of the tutorials site, not from the parent directory as prescribed (hence my changes).

$ python -m http.server 8887
Serving HTTP on 0.0.0.0 port 8887 (http://0.0.0.0:8887/) ...

So this is puzzling (it used to work before, the way you describe it). After reading some of the docs at https://docs.python.org/3/library/http.server.html (no docs available locally?), I tried:

$ python -m http.server --bind 127.0.0.1 8887
Serving HTTP on 127.0.0.1 port 8887 (http://127.0.0.1:8887/) ...

That works; for me this is required to use localhost. Using python 3.10. The --bind feature is there since 3.4.

Still it only works for all subpages (like 'categories', 'installation +' etc) when run directly from the tutorials directory, not from the parent directory.

Would it be due to some sys config in Linux Mint 21.3, since I'm using the system's python here?

So I tried in an 'isolated' environment (actually this is just by controlling PATH as far as I understand conda, so not sure if this provides enough isolation). Same result though.

$ conda activate floris
(floris) $ which python
/home/floris/miniconda3/envs/floris/bin/python
(floris) $ python --version
Python 3.11.9
(floris) $ python -m http.server 8887
Serving HTTP on 0.0.0.0 port 8887 (http://0.0.0.0:8887/) ...

More isolation, still the same:

$ docker run -it -v .:/website/tutorials -p 8887:8887 rocker/tidyverse bash
root@56ac6c351a97:/# which python
root@56ac6c351a97:/# which python3
/usr/bin/python3
root@56ac6c351a97:/# ls website/tutorials/
404.html  assets   categories  create_tutorial  favicon-32x32.png  favicon.png  images      index.xml     js                  rmarkdown-libs  search       tags
articles  authors  code        css              favicon.ico        html         index.html  installation  list_of_categories  robots.txt      sitemap.xml  tutorials
root@56ac6c351a97:/# cd website/
root@56ac6c351a97:/website# python3 -m http.server 8887
Serving HTTP on 0.0.0.0 port 8887 (http://0.0.0.0:8887/) ...
172.17.0.1 - - [10/Feb/2025 20:09:08] code 404, message File not found
172.17.0.1 - - [10/Feb/2025 20:09:08] "GET /create_tutorial/ HTTP/1.1" 404 -
172.17.0.1 - - [10/Feb/2025 20:09:11] code 404, message File not found
172.17.0.1 - - [10/Feb/2025 20:09:11] "GET /create_tutorial/ HTTP/1.1" 404 -
172.17.0.1 - - [10/Feb/2025 20:09:13] code 404, message File not found
172.17.0.1 - - [10/Feb/2025 20:09:13] "GET /categories/ HTTP/1.1" 404 -
^C
Keyboard interrupt received, exiting.
root@56ac6c351a97:/website# cd tutorials/
root@56ac6c351a97:/website/tutorials# python3 -m http.server 8887
Serving HTTP on 0.0.0.0 port 8887 (http://0.0.0.0:8887/) ...
172.17.0.1 - - [10/Feb/2025 20:09:30] "GET /create_tutorial/ HTTP/1.1" 200 -
172.17.0.1 - - [10/Feb/2025 20:09:30] "GET /create_tutorial/images/new_file.png HTTP/1.1" 200 -
172.17.0.1 - - [10/Feb/2025 20:09:30] "GET /create_tutorial/images/folder_file_name.png HTTP/1.1" 200 -
172.17.0.1 - - [10/Feb/2025 20:09:30] "GET /create_tutorial/images/knit_md.png HTTP/1.1" 200 -
172.17.0.1 - - [10/Feb/2025 20:09:30] "GET /create_tutorial/images/propose_file.png HTTP/1.1" 200 -
172.17.0.1 - - [10/Feb/2025 20:09:32] "GET /categories/ HTTP/1.1" 200 -

And it's only when running from the tutorials site root (second try above in docker) that all subpages work well (e.g. 'create tutorial' / 'categories' doesn't when running from parent directory).

But, if it works for you, then we may not want to change it.

@falkmielke
Copy link
Contributor

falkmielke commented Feb 12, 2025

I found out that this worked for me because I had accidentally unpacked the tutorials website into top level of my messy Download folder. Sorry!

I got this to work with another keyword to python::http.server: --directory.
Assuming the zip is unzipped into a subfolder ./tutorials:

python -m http.server 8887 --bind localhost --directory tutorials

I suggest to document the whole command to make it safe.

My fullly repetitive one-line linux pipeline now is

rm -rf tutorials_website && unzip pr-<*>-inbo-tutorials-website.zip -d tutorials_website && python -m http.server 8887 --bind localhost --directory tutorials_website

(you have to replace the <*> per PR)

Finally, to see python docs for http.server locally, try pydoc http.server or to search: pydoc -k http.server.
(via)

I will try to find an alternative for the chrome web app on the other issue.

@falkmielke
Copy link
Contributor

@florisvdh mind if I take over this PR to adjust the documentation to use servr?

@florisvdh
Copy link
Member Author

florisvdh commented Feb 13, 2025

Thanks for the research @falkmielke !

Finally, to see python docs for http.server locally, try pydoc http.server or to search: pydoc -k http.server.
(via)

Thanks for the hint! Out-of-the-box I had to use pydoc3, but indeed it's there!

@florisvdh mind if I take over this PR to adjust the documentation to use servr?

I'd be delighted, great that you found an R solution! I have a lot of trust in packages by the author of {rmarkdown}, {bookdown}, {knitr} etc. 🙂

So you're welcome to take over update_pr_template from me.

@falkmielke
Copy link
Contributor

closes #343

@falkmielke
Copy link
Contributor

@florisvdh I have re-read and adjusted the relevant files and would appreciate your comments in turn. (the PR is yours, so I cannot involve you as a reviewer)

Copy link
Contributor

@falkmielke falkmielke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There were more additions from my side, also to tackle #343
Thank you for double checking!

Copy link
Member Author

@florisvdh florisvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @falkmielke for these very thoughtful additions; excellent work. Sorry for the late response from my side.

Ha, I cannot approve in GitHub as I started the PR (in effect, I usually recommend to not share branches & PRs, so here we see an example why).

But let this be my approval of your commits. That means, I only added optional 🙂 suggestions.

So once you're all fine, please approve the PR and merge into master. (It should be main actually, but that migration is a separate job.)

- `yaml` header:
- [ ] *(recommended)* I am included as author in the `authors` yaml tag, using `[MY_AUTHOR_ID]`. An author information file exists in `<tutorials>/data/authors/<author>.toml`.
- [ ] I have added `categories` to the YAML header and my category tags are from the [list of category tags](https://github.com/inbo/tutorials/blob/master/static/list_of_categories)
- [ ] *(optional)* I have included `tags` in the YAML header (see the tags listed in the [tutorials website side bar](https://inbo.github.io/tutorials/) for tags that have been used before)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have tags not always been added in the past? (I'm wondering why you added 'optional')

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I understood it, "tags" enhance visibility of an article/tutorial because they add to the word cloud and cluster in categories. They are technically optional (i.e. tutorial will render without tags), but in fact we encourage their use.
I slightly re-worded (push follows). Feel free to adjust!

3) From the target directory, run `python -m http.server 8887`, _or_ launch the Google Chrome [Web Server app](https://chrome.google.com/webstore/detail/web-server-for-chrome/ofhbbkphhbklhfoeikjpcbhemlocgigb) and point it at the target directory.
4) Point your browser to http://0.0.0.0:8887.
2) Decompress it into a target directory, e.g. `Downloads/tutorials_preview`.
3) To preview the website, use a program which can serve `http` sites on your local machine. One such option is [the `servr` package](https://github.com/yihui/servr) in R: `& '\C:\Program Files\R\R-4.4.2\bin\Rscript.exe' -e "servr::httd('./tutorials_preview')" -p8887` (*make sure to adjust the path to your `Rscript.exe`*).
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could it be that a space is needed?

Suggested change
3) To preview the website, use a program which can serve `http` sites on your local machine. One such option is [the `servr` package](https://github.com/yihui/servr) in R: `& '\C:\Program Files\R\R-4.4.2\bin\Rscript.exe' -e "servr::httd('./tutorials_preview')" -p8887` (*make sure to adjust the path to your `Rscript.exe`*).
3) To preview the website, use a program which can serve `http` sites on your local machine. One such option is [the `servr` package](https://github.com/yihui/servr) in R: `& '\C:\Program Files\R\R-4.4.2\bin\Rscript.exe' -e "servr::httd('./tutorials_preview')" -p 8887` (*make sure to adjust the path to your `Rscript.exe`*).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, the Linux sibling of the command is welcome.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On linux, it should just be Rscript -e ... works on my arch and on rocker/r-base. I have added this information.

No, for some weird reason, -p 8887 with the space does not work (serves on a random port); I guess this is R args emulating proper arg pairs.

@falkmielke falkmielke self-requested a review February 20, 2025 08:05
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.

Instructions to preview pull request need update
3 participants