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

nikola auto 2.0 #3758

Open
aknrdureegaesr opened this issue Feb 13, 2024 · 1 comment
Open

nikola auto 2.0 #3758

aknrdureegaesr opened this issue Feb 13, 2024 · 1 comment
Labels

Comments

@aknrdureegaesr
Copy link
Contributor

aknrdureegaesr commented Feb 13, 2024

This is more an epic rather than a mere bug. I file this as a "bug" as it also reports several bugs.

Bugs: unstable tests/integration/test_dev_server.py

On several occasions, we have seen the tests/integration/test_dev_server.py misbehave

  • At one occasion, that test ran "forever" in the CI pipeline. Shortly thereafter, I could reproduce that once locally on my Debian Bullseye with self-compiled Python 3.12.2 running pytest tests/, with git #454114971f264 or #411ca509fec, not entirely sure.
  • At another occasion, the test complained "there is no current event loop".

Expected state on close of this ticket: These instabilities are no longer observed.

Bugs: event loop not cleaned up

When under auto, rebuilding is triggered, errors may occur. These errors are handled nowhere. It is generally considered bad practice to never harvest exceptions.

Expected state on close of this ticket: By the time _excute finishes, the event loop should be empty, all awaitables "harvested" and, in particular, no pending exception left in Nirvana. There should be no explicit termination of the event loop, so tests can run several instances of auto.

Automated tests

By the time this ticket gets closed, there should be automated tests that perform a series of events like this:

  • A sample site is built initially
  • One markup source file is changed by the test code in an appropriate way.
  • The test code verifies that the appropriate change has been performed.
  • Now, that markup source file is changed in a way that causes the building to run into an error.
  • (Not sure what the test code should verify here. This part will be a bit white-boxy, relying on implementation details of the site building functionality. The test might register a build step that wants the generated output file; ideally, this step should not be called here.)
  • Finally, that markup source file is repaired.
  • The test code verifies that the appropriate change ends up in the generated site in due time.

It will probably a good idea to do these steps for several types of files (posts, pages, ...).

Implementation hint: For the output, a temporary directory can be used that is provided, and cleaned up, by pytest.

Bug: Pertinent conf changes don't restart the web server

Some lines in the site configuration contain information that is used to set up the web server of auto. When those lines in the conf file change, the web server should be shut down and restarted with the new information. This does not happen presently.

By the time this ticket gets closed: Either this bug is fixed, or it is at least documented via a comment line in the template that is used to generate a new conf file when a new site is initialized.

Deprecation: The way we use loop

Presently, nikola auto uses low-level primitives. The general recommendation is, to use more of the high-level stuff where feasible. In particular, we use asyncio.get_event_loop() to automagically create the event loop. But the documentation says about exactly our use:

Deprecated since version 3.12: Deprecation warning is emitted if there is no current event loop. In some future Python release this will become an error.

By the time this ticket gets closed, a serious attempt should have been made to use higher level stuff, such as asyncio.run(), and restrict the low-level stuff such as direct loop access to places where it is not easily avoided.

Race conditions schedule / cleanup

The watcher is taken down after the loop functionality is. This is a potential race condition: A last-minute change on the file system could trigger new building steps at a time when we do no longer want to service the loop.

Also, if many building steps are being processed and the loop is terminated in the heat of battle, it would be good to explicit cancel things that are going on. Presently, loop.call_soon_threadsafe is called and the return value thrown away, so it is not possible to cancel those actions.

By the time this ticket gets closed, these deficiencies are handled. - This is a special case of "event loop gets cleaned up".

Documentation of internal stuff as such

The classes inside the auto code are implementation details, not API. We may want to mention that in the class comments. Not entirely sure, as this could be construed to imply "if not mentioned (at other places), it is API".

Type hints (nice to have)

By the time this ticket gets closed, it would be good if the auto code has type hints for everything.

@Kwpolska
Copy link
Member

Kwpolska commented Feb 16, 2024

Bug: Pertinent conf changes don't restart the web server
By the time this ticket gets closed: Either this bug is fixed, or it is at least documented via a comment line in the template that is used to generate a new conf file when a new site is initialized.

Implementing config reload would be tricky, and a comment in the configuration file would not be very easy to notice, it could go somewhere in the manual or nikola help auto.

(PS. This might be nikola auto v3.0, since we’ve already had a major rewrite from some legacy stuff (wsgiref + ws4py) to asyncio + aiohttp.)

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

No branches or pull requests

2 participants