/synapse

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Improve error handling for missing files #2551

Merged
merged 2 commits into from Oct 17, 2017

Conversation

Reviewers

@erikjohnston erikjohnston

Assignees

@erikjohnston erikjohnston

Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@richvdh @erikjohnston
@richvdh
Member

richvdh commented Oct 17, 2017 • edited

os.path.exists doesn't allow us to distinguish between permissions errors and
the path actually not existing, which repeatedly confuses people. It also means
that we try to overwrite existing key files, which is super-confusing. (cf
issues #2455, #2379). Use os.stat instead.

Also, don't recomemnd the the use of --generate-config, which screws everything
up if you're using debian (cf #2455).
@richvdh
Improve error handling for missing files 
`os.path.exists` doesn't allow us to distinguish between permissions errors and
the path actually not existing, which repeatedly confuses people. It also means
that we try to overwrite existing key files, which is super-confusing. (cf
issues #2455, #2379). Use os.stat instead.

Also, don't recomemnd the the use of --generate-config, which screws everything
up if you're using debian (cf #2455).

Some checks were not successful

1 errored, 1 failing, and 6 successful checks
@matrixbot
Sytest Dendron (Commit) — Build #2811 origin/rav/keyfile_loading failed in 4 min 51 sec
Details
@matrixbot
Sytest Dendron (Merged PR) — Build finished.
Details
@matrixbot
Sytest Postgres (Commit) — Build #3645 origin/rav/keyfile_loading succeeded in 4 min 31 sec
Details
@matrixbot
Sytest Postgres (Merged PR) — Build finished.
Details
@matrixbot
Sytest SQLite (Commit) — Build #3747 origin/rav/keyfile_loading succeeded in 1 min 45 sec
Details
@matrixbot
Sytest SQLite (Merged PR) — Build finished.
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
continuous-integration/travis-ci/push — The Travis CI build passed
Details
abd502b

@richvdh richvdh assigned erikjohnston Oct 17, 2017

@richvdh richvdh referenced this pull request Oct 17, 2017

Closed

Feedback on the repository-based installation of matrix-synapse on Debian 9/Stretch (Spoiler: It sucks.) #2455

@erikjohnston
Owner

erikjohnston commented Oct 17, 2017

I'm a bit scared that we're losing some helpful hints for people tying to use python directly, or do we have error messages elsewhere if a config file isn't specified? Generally we can rewrite the help text in the debian build where appropriate (e.g. we already remove references to pip install)
@richvdh
more path_exists fixes

Some checks were not successful

1 errored, 1 failing, and 6 successful checks
@matrixbot
Sytest Dendron (Commit) — Build #2814 origin/rav/keyfile_loading failed in 4 min 38 sec
Details
@matrixbot
Sytest Dendron (Merged PR) — Build finished.
Details
@matrixbot
Sytest Postgres (Commit) — Build #3649 origin/rav/keyfile_loading succeeded in 4 min 13 sec
Details
@matrixbot
Sytest Postgres (Merged PR) — Build finished.
Details
@matrixbot
Sytest SQLite (Commit) — Build #3751 origin/rav/keyfile_loading succeeded in 2 min 58 sec
Details
@matrixbot
Sytest SQLite (Merged PR) — Build finished.
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
continuous-integration/travis-ci/push — The Travis CI build passed
Details
9097f3f
synapse/config/_base.py
def check_file(cls, file_path, config_name):
if file_path is None:
raise ConfigError(
"Missing config for %s."
- " You must specify a path for the config file. You can "
@richvdh

richvdh Oct 17, 2017

Member

this text was completely bogus, afaict. This function is called to check that files referenced from the config file exist, rather than the config file itself.

@richvdh
Member

richvdh commented Oct 17, 2017

I'm a bit scared that we're losing some helpful hints for people tying to use python directly, or do we have error messages elsewhere if a config file isn't specified?

if no config file is specified, you hit

synapse/synapse/config/_base.py

Line 319 in 9097f3f

config_parser.error(
, which one could argue has a similar problem with the debian package, but at least you're less likely to hit it under debian than some of the others.

Of all the things caught by the messages I'm removing, I'm unconvinced that --generate-config will solve any of them rather than making everything worse.
@erikjohnston
Owner

erikjohnston commented Oct 17, 2017

Ok, then that works for me

@richvdh richvdh merged commit 7216c76 into develop Oct 17, 2017
6 of 8 checks passed

6 of 8 checks passed

Sytest Dendron (Commit) Build #2814 origin/rav/keyfile_loading failed in 4 min 38 sec
Details
Sytest Dendron (Merged PR) Build finished.
Details
Sytest Postgres (Commit) Build #3649 origin/rav/keyfile_loading succeeded in 4 min 13 sec
Details
Sytest Postgres (Merged PR) Build finished.
Details
Sytest SQLite (Commit) Build #3751 origin/rav/keyfile_loading succeeded in 2 min 58 sec
Details
Sytest SQLite (Merged PR) Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@richvdh richvdh deleted the rav/keyfile_loading branch Oct 17, 2017

This was referenced Oct 18, 2017

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