Add support for XDG_CONFIG_HOME #129

0az · 2020-05-30T18:36:27Z

Implement support for XDG_CONFIG_HOME.

To avoid breaking changes, we use locations in this order:

$B2_ACCOUNT_INFO_ENV_VAR: If set
~/.b2_account_info: If exists
$XDG_CONFIG_HOME/b2/account_info: If set
~/.b2_account_info: If no path takes priority

ppolewicz · 2020-05-30T20:04:16Z

We used to be able to pass a file path to the constructor of SqliteAccountInfo, but you removed it. Please restore it.

Also, could you briefly explain why are you proposing this change?

0az · 2020-05-30T20:16:57Z

We used to be able to pass a file path to the constructor of SqliteAccountInfo, but you removed it. Please restore it.

Passing in the file path still works: I just forgot to add it to the explanatory text and the docstring. I have since done the latter.

Also, could you briefly explain why are you proposing this change

I'm proposing the change so I can remove another dotfile from `$HOME` and add b2sdk/b2 cli to the list of applications that support the XDG spec. If I'm not mistaken, version 1.1.0 should go out soonish, and will be used in the next version of the b2 cli.

ppolewicz · 2020-05-30T20:42:38Z

I think it is a good change. Could you please add a unit test to make sure that we don't hit a regression on this new functionality in the future?

0az · 2020-05-30T22:35:10Z

I added unit tests for two cases:

XDG_CONFIG_HOME
B2_ACCOUNT_INFO_ENV_VAR and XDG_CONFIG_HOME are set.

I'm not sure how to test the third interaction, where XDG_CONFIG_HOME is set, but a database already exists. This may require changing the file_name kwarg to override B2_ACCOUNT_INFO_DEFAULT_FILE instead of overriding everything.

A hermetic testing environment and compatibility with existing code is ensured by modifying _make_sqlite_account_info to take an env kwarg. If it is unset or falsy, it will instantiate SqliteAccountInfo with an empty env. If it is truthy, it will override the usual file_name behavior.

b2sdk/account_info/sqlite_account_info.py

ppolewicz

very nice job overall, wonderful extension, I support the motion to not pollute ~/ with config files. I've asked for a few minor tweaks after closer review.

b2sdk/account_info/sqlite_account_info.py

test/v1/test_account_info.py

ppolewicz · 2020-06-04T15:17:49Z

@0az will you do those changes so that we can fit this PR in the release?

Co-authored-by: Paweł Polewicz <p.polewicz@gmail.com>

ppolewicz

good work, a couple of minor issues remian

b2sdk/account_info/sqlite_account_info.py

test/v1/test_account_info.py

b2sdk/account_info/sqlite_account_info.py

test/v1/test_account_info.py

0az · 2020-06-08T06:38:10Z

I'm really sorry about that. I probably should have mentioned this earlier, but for some unknown reason that I don't have time to debug after midnight, the pre-commit shell script fails with a RuntimeError: "An attempt has been made to start a new process before the current process has finished its bootstrapping phase." Logs are in this gist: https://gist.github.com/0az/234a94a123e15259f3779661feed69e6. Manually running nosetests still works as a workaround, though. I also modified the test class to include a temporary directory acting as HOME.

ppolewicz · 2020-06-08T22:07:43Z

@mlech-reef did we accidentally enable parallel testing in nosetests on Windows? I don't think that ever worked, now appveyor can't run the tests and our contributor here also has issues. Travis is absent from this PR for some reason... Do you have any clue?

mlech-reef · 2020-06-09T17:39:58Z

...
Travis is absent from this PR for some reason... Do you have any clue?

Travis is triggered properly, but for some reason, it's not showing here. I've seen this issue in other PRs. We should investigate it.
https://travis-ci.org/github/Backblaze/b2-sdk-python/builds/696025513

mlech-reef · 2020-06-09T18:22:23Z

@mlech-reef did we accidentally enable parallel testing in nosetests on Windows? I don't think that ever worked, now appveyor can't run the tests and our contributor here also has issues.

I don't recall anything like that. AppVeyor fails not because of the parallel runs but because 2 new tests are failing with:
PermissionError: [WinError 32] The process cannot access the file because it is being used by another process
It's probably because TempDir uses shutils.rmtree on a directory with sqlite db file created inside.
It's similar to the problem described here: https://stackoverflow.com/questions/1213706/what-user-do-python-scripts-run-as-in-windows
Windows and python sometimes don't get along :)

ppolewicz · 2020-06-12T00:38:27Z

b2sdk/account_info/sqlite_account_info.py

@@ -36,17 +36,39 @@ class SqliteAccountInfo(UrlPoolAccountInfo):

    def __init__(self, file_name=None, last_upgrade_to_run=None):
        """
-        If ``file_name`` argument is empty or ``None``, path from ``B2_ACCOUNT_INFO`` environment variable is used. If that is not available, a default of ``~/.b2_account_info`` is used.


I am reading this docstring over and over and I think apiver doesn't let us change the default location to use XDG_CONFIG_HOME in v1. Someone may have written a program that relies on the promise that when the filename was not passed and B2_ACCOUNT_INFO was not set, then the db file will land in ~/.b2_account_info. After running something, the program might clean the environment by removing that file.

If we make this change and XDG_CONFIG_HOME is defined, sdk user would not expect the configuration of the end user to be suddenly leaked (as it is not cleaned up anymore), possibly allowing access to the following user.

I think this is a good change and we should introduce it, but that should happen with the release of from b2sdk.v2 import *, otherwise we risk introducing a breaking change, while the purpose of apiver is to avoid it unless fixing a bug (which we are not doing here) which would change behavior because the old behavior was just wrong.

Furthermore, we should separate the documentation of the interface from the documentation of the current behavior, so that this can be altered in the future, by mentioning that the user should not define of this algorithm staying exactly the same, but should use self.filename to determine the actually determined location of the file in case it needs to be accessed later.

We did not have a compelling reason to release v2, but this is a good one.

@bwbeach @0az do you agree with my viewpoint? Shall we move it to v2?

Good catch: I didn't actually think of that case.

Sounds good to me.

@0az then would you please adjust the docstring here? We'll hold off merging this until b2sdk 1.1.0 and b2cli 2.0.0 are shipped, then we'll increment apiver to v2 and finally we'll merge your thing (though a final adjustment will be necessary to provide backwards compatibility)

Another question for v2: should SqliteAccountInfo support :memory: or anonymous (empty string) databases?

https://www.sqlite.org/c3ref/open.html

If the filename is ":memory:", then a private, temporary in-memory database is created for the connection. This in-memory database will vanish when the database connection is closed. Future versions of SQLite might make use of additional special filenames that begin with the ":" character. It is recommended that when a database filename actually does begin with a ":" character you should prefix the filename with a pathname such as "./" to avoid ambiguity.

If the filename is an empty string, then a private, temporary on-disk database will be created. This private database will be automatically deleted as soon as the database connection is closed.

It looks like that could replace InMemoryAccountInfo with one line of code. I like less code.

b2sdk/account_info/sqlite_account_info.py

Co-authored-by: Andrew Zhou <0az@afzhou.com>

mpnowacki-reef · 2021-04-26T14:22:20Z

@0az I have submitted a PR that merges master into your xdg branch and moves your changes along with tests to v2: 0az#1
We're planning to wrap up v2 by the end of this week, would you be willing to finish this PR?

0az · 2021-04-28T01:21:44Z

Thanks! Was there anything else that I needed to do? It's been a while. 🙃

ppolewicz · 2021-04-28T02:06:56Z

b2sdk/v1/account_info.py

+            v2.B2_ACCOUNT_INFO_ENV_VAR, v2.B2_ACCOUNT_INFO_DEFAULT_FILE
+        )
+        self.filename = file_name or os.path.expanduser(user_account_info_path)
+        logger.debug('%s file path to use: %s', self.__class__.__name__, self.filename)


where is logger initialized?

It's imported from here:

b2-sdk-python/b2sdk/account_info/sqlite_account_info.py

Line 23 in 632b13b

logger = logging.getLogger(__name__)

ok, but here in b2sdk/v1/account_info.py there is no logger, so it will crash. Logger initialization should be added.

Sorry, bad wording. The initialization happens there, and there's an import in this file for that logger.

It probably should get initialized here anyway, though.

from b2sdk.account_info.sqlite_account_info import logger ohhh. I don't think we should do that, that is super confusing. We should initialize it locally, right?

ppolewicz · 2021-04-28T02:09:43Z

This PR seems to contain v2 changes. Can we somehow make it clean? @0az would you mind if we took your changes and rebased them to slide it into v2 at the appropriate moment? The v2 change is a little bit chaotic and I don't want to keep pulling you into this.

0az · 2021-04-28T02:12:31Z

This PR seems to contain v2 changes. Can we somehow make it clean?

Michał sent in a PR with the v2 changes, so I merged that in. I can force push that off of the branch, if you want.

@0az would you mind if we took your changes and rebased them to slide it into v2 at the appropriate moment? The v2 change is a little bit chaotic and I don't want to keep pulling you into this.

This is also fine.

mpnowacki-reef · 2021-04-30T09:44:35Z

@0az I've come to clean up the mess I made. I squashed all your commits on top of the current master and created a new PR (#235). I have made you the author of the first commit, in order not to steal your thunder. Is that okay with you?

0az · 2021-04-30T19:36:10Z

Yeah, 235 looks good. I'll close this PR, then.

Add support for XDG_CONFIG_HOME

3475590

Add XDG to the SqliteAccountInfo docstring

c84f97d

0az added 2 commits May 30, 2020 17:25

Add env argument to SqliteAccountInfo for testing

36064fa

Add unit tests for XDG

55791f3

0az force-pushed the xdg branch from 2fdeb7e to 55791f3 Compare May 30, 2020 22:25

bwbeach reviewed Jun 1, 2020

View reviewed changes

b2sdk/account_info/sqlite_account_info.py Outdated Show resolved Hide resolved

0az added 2 commits June 1, 2020 12:49

Rewrite account info search for clarity

9369c20

Re-add file_name

7a73416

ppolewicz requested changes Jun 1, 2020

View reviewed changes

0az and others added 3 commits June 4, 2020 10:28

Apply suggestions from code review

88b12c5

Co-authored-by: Paweł Polewicz <p.polewicz@gmail.com>

Rewrite tests to use patch.dict

9a159ac

Rewrite tests to use b2sdk.utils.TempDir

0144516

0az force-pushed the xdg branch from 68e5e18 to 0144516 Compare June 4, 2020 15:44

ppolewicz requested changes Jun 5, 2020

View reviewed changes

b2sdk/account_info/sqlite_account_info.py Outdated Show resolved Hide resolved

test/v1/test_account_info.py Outdated Show resolved Hide resolved

b2sdk/account_info/sqlite_account_info.py Outdated Show resolved Hide resolved

Actually apply suggestions from code review

f1a2d9a

ppolewicz approved these changes Jun 8, 2020

View reviewed changes

ppolewicz reviewed Jun 8, 2020

View reviewed changes

test/v1/test_account_info.py Outdated Show resolved Hide resolved

0az added 2 commits June 8, 2020 01:34

Fix tests

43b6620

Add XDG_CONFIG_HOME to README

072bd8f

Merge branch 'master' into xdg

d0e2767

ppolewicz reviewed Jun 12, 2020

View reviewed changes

ppolewicz added the v2 label Jun 17, 2020

0az commented Jun 20, 2020

View reviewed changes

b2sdk/account_info/sqlite_account_info.py Outdated Show resolved Hide resolved

Update b2sdk/account_info/sqlite_account_info.py

9eee217

Co-authored-by: Andrew Zhou <0az@afzhou.com>

ppolewicz mentioned this pull request Jun 21, 2020

List of things to consider for apiver v2 #139

Open

mlech-reef added this to the ApiVer v2 milestone Feb 11, 2021

mlech-reef removed the v2 label Feb 11, 2021

ppolewicz mentioned this pull request Apr 4, 2021

Respect XDG Base Directory specification Backblaze/B2_Command_Line_Tool#699

Closed

mpnowacki-reef added 3 commits April 26, 2021 14:41

Merge remote-tracking branch 'upstream/master' into xdg

4c72147

XDG changes and tests moved to v2

e829822

v1 SqliteAccountInfo tests are working

dc0a4dc

0az and others added 2 commits April 27, 2021 20:17

Merge pull request #1 from reef-technologies/xdg

7bd28b6

Merge branch 'master' into xdg

e1a3715

ppolewicz requested changes Apr 28, 2021

View reviewed changes

Initialize logger locally in v1/account_info

6ee83ad

mpnowacki-reef mentioned this pull request Apr 30, 2021

Add support for XDG_CONFIG_HOME #235

Merged

0az closed this Apr 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for XDG_CONFIG_HOME #129

Add support for XDG_CONFIG_HOME #129

0az commented May 30, 2020

ppolewicz commented May 30, 2020

0az commented May 30, 2020 via email

ppolewicz commented May 30, 2020

0az commented May 30, 2020

ppolewicz left a comment

ppolewicz commented Jun 4, 2020

ppolewicz left a comment

0az commented Jun 8, 2020 via email

ppolewicz commented Jun 8, 2020

mlech-reef commented Jun 9, 2020 •

edited

Loading

mlech-reef commented Jun 9, 2020

ppolewicz Jun 12, 2020

0az Jun 14, 2020

ppolewicz Jun 14, 2020

0az Jun 20, 2020

ppolewicz Jun 21, 2020 •

edited

Loading

mpnowacki-reef commented Apr 26, 2021

0az commented Apr 28, 2021

ppolewicz Apr 28, 2021

0az Apr 28, 2021

ppolewicz Apr 28, 2021

0az Apr 28, 2021

ppolewicz Apr 28, 2021

ppolewicz commented Apr 28, 2021

0az commented Apr 28, 2021

mpnowacki-reef commented Apr 30, 2021

0az commented Apr 30, 2021

Add support for XDG_CONFIG_HOME #129

Add support for XDG_CONFIG_HOME #129

Conversation

0az commented May 30, 2020

ppolewicz commented May 30, 2020

0az commented May 30, 2020 via email

ppolewicz commented May 30, 2020

0az commented May 30, 2020

ppolewicz left a comment

Choose a reason for hiding this comment

ppolewicz commented Jun 4, 2020

ppolewicz left a comment

Choose a reason for hiding this comment

0az commented Jun 8, 2020 via email

ppolewicz commented Jun 8, 2020

mlech-reef commented Jun 9, 2020 • edited Loading

mlech-reef commented Jun 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ppolewicz Jun 21, 2020 • edited Loading

Choose a reason for hiding this comment

mpnowacki-reef commented Apr 26, 2021

0az commented Apr 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ppolewicz commented Apr 28, 2021

0az commented Apr 28, 2021

mpnowacki-reef commented Apr 30, 2021

0az commented Apr 30, 2021

mlech-reef commented Jun 9, 2020 •

edited

Loading

ppolewicz Jun 21, 2020 •

edited

Loading