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

Fixed #24704 -- Made the runserver autoreloader survive SyntaxErrors. #5106

Merged
merged 5 commits into from Aug 29, 2015

Conversation

Projects
None yet
2 participants
@aaugustin
Member

aaugustin commented Aug 5, 2015

No description provided.

@timgraham

View changes

Show outdated Hide outdated django/core/management/__init__.py Outdated

@timgraham timgraham changed the title from Autoreload after syntax error to Fixed #24704 -- Made the runserver autoreloader survive SyntaxErrors. Aug 5, 2015

@aaugustin

This comment has been minimized.

Show comment
Hide comment
@aaugustin

aaugustin Aug 8, 2015

Member

I believe this is ready for checkin.

Member

aaugustin commented Aug 8, 2015

I believe this is ready for checkin.

@timgraham

This comment has been minimized.

Show comment
Hide comment
@timgraham

timgraham Aug 11, 2015

Member

I tested on Python 3.4. Modified polls/models.py, got this exception: http://dpaste.com/1GGDV80, fixed it, but the server didn't reload. Is it expected?

Member

timgraham commented Aug 11, 2015

I tested on Python 3.4. Modified polls/models.py, got this exception: http://dpaste.com/1GGDV80, fixed it, but the server didn't reload. Is it expected?

@timgraham

This comment has been minimized.

Show comment
Hide comment
@timgraham

timgraham Aug 11, 2015

Member

A test failure on Python 2 with non-ascii path:

======================================================================
FAIL: test_app_locales (utils_tests.test_autoreload.TestFilenameGenerator)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/media/evo/djangō/django/test/utils.py", line 182, in inner
    return test_func(*args, **kwargs)
  File "/media/evo/djangō/tests/utils_tests/test_autoreload.py", line 74, in test_app_locales
    os.path.join(admin_dir, 'nl', 'LC_MESSAGES', 'django.mo'))
  File "/media/evo/djangō/tests/utils_tests/test_autoreload.py", line 25, in assertFileFound
    self.assertIn(npath(filename), autoreload.gen_filenames())
AssertionError: '/djang\xc5\x8d/django/contrib/admin/locale/nl/LC_MESSAGES/django.mo' not found in [... long list...]
Member

timgraham commented Aug 11, 2015

A test failure on Python 2 with non-ascii path:

======================================================================
FAIL: test_app_locales (utils_tests.test_autoreload.TestFilenameGenerator)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/media/evo/djangō/django/test/utils.py", line 182, in inner
    return test_func(*args, **kwargs)
  File "/media/evo/djangō/tests/utils_tests/test_autoreload.py", line 74, in test_app_locales
    os.path.join(admin_dir, 'nl', 'LC_MESSAGES', 'django.mo'))
  File "/media/evo/djangō/tests/utils_tests/test_autoreload.py", line 25, in assertFileFound
    self.assertIn(npath(filename), autoreload.gen_filenames())
AssertionError: '/djang\xc5\x8d/django/contrib/admin/locale/nl/LC_MESSAGES/django.mo' not found in [... long list...]
@timgraham

This comment has been minimized.

Show comment
Hide comment
@timgraham

timgraham Aug 11, 2015

Member

You don't plan to backport, correct?

Member

timgraham commented Aug 11, 2015

You don't plan to backport, correct?

@aaugustin

This comment has been minimized.

Show comment
Hide comment
@aaugustin

aaugustin Aug 11, 2015

Member

No, I don't plan to backport.

Member

aaugustin commented Aug 11, 2015

No, I don't plan to backport.

@aaugustin

This comment has been minimized.

Show comment
Hide comment
@aaugustin

aaugustin Aug 29, 2015

Member

I fixed the test failure on Python 2.

Member

aaugustin commented Aug 29, 2015

I fixed the test failure on Python 2.

@aaugustin

This comment has been minimized.

Show comment
Hide comment
@aaugustin

aaugustin Aug 29, 2015

Member

@timgraham: your paste isn't available anymore but the last commit I added to my branch probably fixes the problem you encountered.

Member

aaugustin commented Aug 29, 2015

@timgraham: your paste isn't available anymore but the last commit I added to my branch probably fixes the problem you encountered.

@aaugustin

This comment has been minimized.

Show comment
Hide comment
@aaugustin

aaugustin Aug 29, 2015

Member

Upon further thought, it may be a good idea to backport at least to 1.8, because this can be a real pain in day-to-day development.

Member

aaugustin commented Aug 29, 2015

Upon further thought, it may be a good idea to backport at least to 1.8, because this can be a real pain in day-to-day development.

@timgraham

This comment has been minimized.

Show comment
Hide comment
@timgraham

timgraham Aug 29, 2015

Member

Code looks good. Please add release notes if you backport. But reloading still doesn't seem to work as expected. Using the tutorial, I remove a dot in admin.site.register (AttributeError: 'module' object has no attribute 'siteregister'), add the dot back, but no reloading occurs.

Member

timgraham commented Aug 29, 2015

Code looks good. Please add release notes if you backport. But reloading still doesn't seem to work as expected. Using the tutorial, I remove a dot in admin.site.register (AttributeError: 'module' object has no attribute 'siteregister'), add the dot back, but no reloading occurs.

@aaugustin

This comment has been minimized.

Show comment
Hide comment
@aaugustin

aaugustin Aug 29, 2015

Member

I cannot reproduce your last comment either on Python 2 or Python 3. When adding the dot back, the server reloads.

Anyway, since Django can't figure out which file caused the error in general, there will always be some cases where the autoreload will fail to notice changes that fix an error. That's a fundamental design flaw which cannot be patched without replacing the entire autoreloader, as I explained on the mailing list.

Member

aaugustin commented Aug 29, 2015

I cannot reproduce your last comment either on Python 2 or Python 3. When adding the dot back, the server reloads.

Anyway, since Django can't figure out which file caused the error in general, there will always be some cases where the autoreload will fail to notice changes that fix an error. That's a fundamental design flaw which cannot be patched without replacing the entire autoreloader, as I explained on the mailing list.

@timgraham

This comment has been minimized.

Show comment
Hide comment
@timgraham

timgraham Aug 29, 2015

Member

As noted on IRC, my problem is reproduced with pyinotify installed.

Member

timgraham commented Aug 29, 2015

As noted on IRC, my problem is reproduced with pyinotify installed.

aaugustin added some commits Aug 5, 2015

Accounted for error files in the autoreloader.
* When some old files contain errors, the second call to
  gen_filenames() should return them.
* When some new files contain errors, the first call to
  gen_filenames(only_new=True) should return them.
Refactored autoreload tests.
* Added helpers to test uncached and cached access.
* Fixed test_project_root_locale: it duplicated test_locale_paths_setting.
* Rewrote test_only_new_files: test more cases.
Ensured gen_filenames() yields native strings.
This also fixes a test failure on Python 2 when Django is installed in a
non-ASCII path. This problem cannot happen on Python 3.
Fixed #24704 -- Made the autoreloader survive SyntaxErrors.
With this change, it's expected to survive anything except errors
that make it impossible to import the settings. It's too complex
to fallback to a sensible behavior with a broken settings module.

Harcoding things about runserver in ManagementUtility.execute is
atrocious but it's the only way out of the chicken'n'egg problem:
the current implementation of the autoreloader primarily watches
imported Python modules -- and then a few other things that were
bolted on top of this design -- but we want it to kick in even if
the project contains import-time errors and django.setup() fails.

At some point we should throw away this code and replace it by an
off-the-shelf autoreloader that watches the working directory and
re-runs `django-admin runserver` whenever something changes.
@aaugustin

This comment has been minimized.

Show comment
Hide comment
@aaugustin

aaugustin Aug 29, 2015

Member

As noted on IRC, Tim's problem is resolved with the latest version of the PR.

Member

aaugustin commented Aug 29, 2015

As noted on IRC, Tim's problem is resolved with the latest version of the PR.

@aaugustin aaugustin merged commit b79fc11 into django:master Aug 29, 2015

5 checks passed

default Build finished. No test results found.
Details
docs Build finished. No test results found.
Details
flake8 Build finished. No test results found.
Details
isort Build finished. No test results found.
Details
javascript Build finished. No test results found.
Details

@aaugustin aaugustin deleted the aaugustin:autoreload-after-syntax-error branch Aug 29, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment