Ticket26862: config file argument by juga0 · Pull Request #238 · torproject/sbws

juga0 · 2018-07-18T20:32:39Z

No description provided.

and create minimal user config when it does not exist.

there is not anymore sbws_home including config

and other features to make sbws more compatible with system packages.

pastly

Nothing major. Pretty simple changes requested.

pastly · 2018-07-23T16:08:12Z

+def _create_user_config_file(fname):
+    """Copy minimal user config to user config path."""
+    if not os.path.isfile(fname):
+        shutil.copyfile(MINIMUM_USER_CONFIG_PATH, fname)


Do you think we should assert on fname not existing here? I think so.

If the file exists already, we don't create/overwrite anything. We silently do nothing. As a programmer, I'd expect a function with "create" in its name to do something when the file already exists. I would expect it to do (at least) one of the following.

overwrite it

warn about it existing (and about overwriting it, if it does so)

assert that the file doesn't exist

I would prefer the assert. If not that, then a different function name. If not that, a debug log warning that it exists.

pastly · 2018-07-23T16:10:49Z

+    """Extend ConfigParser from file configuration."""
+    conf = _read_config_file(conf, fpath)
+    return conf
+


Unless I'm missing something, _extend_config() is an unnecessary wrapper around _read_config_file(), thus every time _extend_config() is called, _read_config_file() could be called instead.

If you like the name _extend_config() better (I do), I'd just rename _read_config_file() to that.

pastly · 2018-07-23T16:13:01Z

-    p.add_argument('-d', '--directory', default=_default_dot_sbws_dname(),
-                   help='Name of the .sbws directory')
+    p.add_argument('-c', '--config',
+                   help='Path the sbws config file')


"Path to the sbws config file"

pastly · 2018-07-23T16:18:20Z

+            _create_user_config_file(args.config)
+            return conf
+        return _extend_config(conf, args.config)
+    _create_user_config_file(USER_CONFIG_PATH)


If we make _create_user_config_file() assert on the file not existing, then we should only conditionally call this function here.

pastly · 2018-07-23T16:39:55Z

 @pytest.fixture(scope='session')
 def sbwshome(sbwshome_dir, args, conf):
-    init.main(args, conf)
    return sbwshome_dir


This function looks unnecessary. Instead of using this fixture, why not just use the parent sbwshome_dir fixture in tests?

pastly

Some issues in the _ensure_dirs() function. Stay tuned after this for potentially more issues as I continue testing

pastly · 2018-07-23T16:45:51Z

+    os.makedirs(conf['paths']['datadir'])
+    os.makedirs(conf['paths']['v3bw'])
+    os.makedirs(conf['paths']['log'])
+


This needs to have exist_ok=True for each.

v3bw_dname and log_dname

pastly

After typing all this out, I really like the beautiful fix and think it would be very easy to implement. Let me know what you think.

So there's an issue now where if ~ is in the path, Python doesn't turn that into $HOME (or into /home/pastly).

Here's an easy fix: add sbws_home to the freshly-generated user config and expand ~ into /home/pastly with os.path.expanduser()

Here's another possible fix: after getting the complete config in get_config(), call something that expands sbws_home (or all the paths in the config). If we only expand sbws_home, then we're treating it as a special case, and I don't want that (consider a user wanting to store v3bw files somewhere other than sbws_home but that's still in the user's home). If we expand them all ... okay I guess. I'd want to see something added that requires the developer to update the function if a new path is added so they don't forget.

Here's a beautiful fix: https://docs.python.org/3/library/configparser.html#customizing-parser-behaviour Use the converters kwarg to ConfigParser, specify {'path': _parse_path}, and define _parse_path() as a function that calls both os.path.expanduser() and os.path.expandvars(). Then we can use conf.getpath('key', value')

juga0 · 2018-07-23T22:42:35Z

i made the changes you requested though in a bit different way.
Let me know if more explanations or additional changes are needed.

juga0 · 2018-07-23T22:52:41Z

Thinking it better, if we allow the config paths to have env vars or ~, then all paths should be expanded (in case the user choose paths not relative to sbws_home).
I think logic would be simpler not allowing config paths to have ~ nor env vars for now (we are not taking env vars into account anywhere else in the config). Then sbws_home can be set to ~ by default when not provided in the config file. Opinions?

pastly · 2018-07-23T23:45:40Z

Thinking it better, if we allow the config paths to have env vars or ~, then all paths should be expanded (in case the user choose paths not relative to sbws_home).

This is my second suggested fix. "Here's another possible fix: after getting the complete config in get_config(), call something that expands sbws_home (or all the paths in the config)." My second suggested fix was also my least favorite.

I think logic would be simpler not allowing config paths to have ~ nor env vars for now

I would agree, but then you made this PR that adds a~ in sbws_home and I convinced myself that allowing ~ would be important for flexible software that lends itself to packaging.

I think the beautiful solution keeps logic very simple. In fact, it might be simpler since no paths are special cases and they all get handled the same way automatically.

Define the following code (but turned into real Python)

# Add in sbws/util/config.py
def _parse_path(path):
    return os.path.expanduser(os.path.expandvars(path))

# And everywhere we create a ConfigParser, do it this way
conf = ConfigParser(interpolation=ExtendedInterpolation(), converters={'path': _parse_path})
# (ExtendedInterpolation() is not new, just the coverters argument)

#############

# And everywhere we get a path from the config, do it this way.
# Just like we get ints with .getint() and bools with .getboolean()
some_fname = conf.getpath('foo', 'some_fname')

pastly · 2018-07-24T00:41:30Z

-    assert os.path.isfile(fname)
-    conf = _read_config_file(conf, fname)
+    # it is only needed to expand user and vars to have the rest of the
+    # paths correctly expanded too


To elaborate a little more on why I like the "beautiful solution" the most: this comment is true for the default configuration options as they exist today and is true for how I expect users to use sbws. But other than that, it isn't necessarily true. I don't think there's anything stopping users from configuring paths in such a way that this comment becomes false.

and add config.example.ini

juga0 · 2018-07-24T19:42:24Z

i used the "beautiful fix" you suggested.

After thinking more about, i changed again the function to create the user config. I think we should not create any user config by default, as when using system packages it would be overwritten by the system.
Instead, i moved back the example config to examples, so that a user can create a user config from it easily.
I also changed some default tor directories, as at some point, in a system package, those directories would be managed by a service and would be in different locations.
And i also changed the logging to stdout by default, as that's would a system service would expect.

juga0 · 2018-07-24T23:21:20Z

meh, it seems that in python 3.4 ConfigParser does not accept converters...

pastly · 2018-08-01T21:07:32Z

@juga0, how do you feel about dropping support for Python 3.4? Stretch has 3.5 as does Xenial. People on other distros (1) will not have a nice .deb package to install and will have to git clone or pip install, (2) can use pyenv to get a newer python.

pastly

Some easy things remain. Let me know if I should do them real quick.

Also, you missed a couple places to change to conf.getpath(...) ;) This is actually preventing me from running sbws

$ find sbws -type f -name '*.py' | xargs grep "conf.*paths']"
sbws/lib/v3bwfile.py:        filepath = conf['paths']['started_filepath']
sbws/core/generate.py:    output = args.output or conf['paths']['v3bw_fname'].format(now_fname())
sbws/core/scanner.py:    filepath = conf['paths']['started_filepath']

(venv-editable) matt@spacecow:~/src/simple-bw-scanner$ sbws scanner
[2018-08-01 17:18:59,141] [sbws.sbws] [INFO] sbws 0.6.1-dev with python 3.5.3 on Linux-4.9.0-7-amd64-x86_64-with-debian-9.5, stem 1.6.0-dev, and requests 2.18.4
[2018-08-01 17:18:59,142] [sbws.core.scanner] [INFO] Scanner started at 2018-08-01T21:18:59
Traceback (most recent call last):
  File "/home/matt/src/simple-bw-scanner/venv-editable/bin/sbws", line 11, in <module>
    load_entry_point('sbws', 'console_scripts', 'sbws')()
  File "/home/matt/src/simple-bw-scanner/sbws/sbws.py", line 72, in main
    exit(comm['f'](*comm['a'], **comm['kw']))
  File "/home/matt/src/simple-bw-scanner/sbws/core/scanner.py", line 400, in main
    run_speedtest(args, conf)
  File "/home/matt/src/simple-bw-scanner/sbws/core/scanner.py", line 334, in run_speedtest
    write_start_ts(conf)
  File "/home/matt/src/simple-bw-scanner/sbws/core/scanner.py", line 328, in write_start_ts
    with FileLock(filepath):
  File "/home/matt/src/simple-bw-scanner/sbws/util/filelock.py", line 16, in __enter__
    self._fd = os.open(self._lock_fname, mode)
FileNotFoundError: [Errno 2] No such file or directory: '~/.sbws/started_at.lockfile'

pastly · 2018-08-01T21:20:04Z

+    If a custom path is passed, search for user configuration there.
+    In any case, create a minimal user configuration in user configuration path
+    , in case it needs to be overwritten.
+    """


This function doesn't currently create a file. I think that just means the comment needs to be updated.

pastly · 2018-08-01T21:21:19Z

-.. automodule:: sbws.core
-    :members:
-    :undoc-members:
-    :show-inheritance:


Did you mean to delete this file?

pastly · 2018-08-01T21:21:40Z

+probably needs to edit in ``examples``. For more advanced configuration options see
+documentation below.
+``sbws`` will check by default whether there's a user customized file in
+``~/sbws.ini`` and use it when it exists.


~/.sbws.ini (missing the dot)

juga0 · 2018-08-02T17:16:39Z

@juga0, how do you feel about dropping support for Python 3.4?

up for it. There're has been already a couple of tickets where i was also asking this since giving support for Python 3.4 required extra work

This reverts commit b91697c. Removed by mistake

juga0 · 2018-08-02T17:53:10Z

Thanks for pointing out the mistakes. They should be fixed now

juga0 · 2018-08-02T17:53:55Z

When you're happy with the changes, let me know and i can rebase to master and fix the conflicting files problems

juga0 · 2018-08-03T10:59:23Z

Continuing in #242 cause of rebase

juga0 · 2018-08-03T10:59:53Z

For the next time, let's try to merge old PRs before creating new features directly into master.

pastly · 2018-08-03T13:39:06Z

Merged via #242

juga0 added 19 commits July 18, 2018 19:48

Add default sbws_home in config

ff961c4

Remove unused config example

79b1c53

Add instead minimal user config

2e687b6

Remove is_innited dependency

63ad83a

Remove is_innited dependency

8c31067

Remove is_innited dependency

6435bf1

Remove is_innited dependency

6d06883

Remove is_initted and add config paths

29536ea

Remove init dependency and create default dirs

88892bc

Remove init and add config arg

7655425

Use config default paths

8379760

and create minimal user config when it does not exist.

Remove init

cf6b58f

Remove sbws_home including config

138d749

there is not anymore sbws_home including config

Remove init and sbwshome with config

0c30b08

Remove init and sbwshome with config

020f732

Remove init

b8e7917

Remove init

2d0484b

Remove init and sbwshome with config

def00ed

Add changelog entry about configuration argument

59d361c

and other features to make sbws more compatible with system packages.

juga0 added the enhancement New feature or request label Jul 18, 2018

juga0 added this to the 1.0 (MVP must) milestone Jul 18, 2018

Fix line length broken after replacing arg

1132f04

pastly self-requested a review July 23, 2018 15:56

pastly suggested changes Jul 23, 2018

View reviewed changes

pastly reviewed Jul 24, 2018

View reviewed changes

juga0 added 8 commits July 24, 2018 18:55

Include examples

1f78675

Include examples

5cf4219

Change logging to stdout by default

8b3f201

Remove init from documentation

b91697c

Delete config.ini from documentation

69dd12c

and add config.example.ini

Update doc with config files changes

67c472f

Remove init section

cd3b3a9

Remove init section

9fd8a6e

juga0 force-pushed the ticket26862 branch from c090a6b to 9fd8a6e Compare July 24, 2018 19:40

pastly changed the title ~~Ticket26862~~ Ticket26862: config file argument Aug 1, 2018

pastly reviewed Aug 1, 2018

View reviewed changes

juga0 added 7 commits August 2, 2018 17:44

Replace conf['paths'] by conf.getpath('paths'

424e1df

Replace conf['paths'] by conf.getpath('paths'

ff84486

Replace conf['paths'] by conf.getpath('paths'

52bec67

Update docstring, it does not create user conf

036b434

sbws default user conf starts by .

9980d52

Revert "Remove init from documentation"

dfed5a5

This reverts commit b91697c. Removed by mistake

Remove instead init documentation

b5b40f0

pastly approved these changes Aug 2, 2018

View reviewed changes

juga0 mentioned this pull request Aug 3, 2018

Ticket26862 rebase #242

Merged

pastly closed this Aug 3, 2018

Conversation

juga0 commented Jul 18, 2018

Uh oh!

pastly left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pastly left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pastly left a comment

Choose a reason for hiding this comment

Uh oh!

juga0 commented Jul 23, 2018

Uh oh!

juga0 commented Jul 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pastly commented Jul 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juga0 commented Jul 24, 2018

Uh oh!

juga0 commented Jul 24, 2018

Uh oh!

pastly commented Aug 1, 2018

Uh oh!

pastly left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juga0 commented Aug 2, 2018

Uh oh!

juga0 commented Aug 2, 2018

Uh oh!

juga0 commented Aug 2, 2018

Uh oh!

juga0 commented Aug 3, 2018

Uh oh!

juga0 commented Aug 3, 2018

Uh oh!

pastly commented Aug 3, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

juga0 commented Jul 23, 2018 •

edited

Loading