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

Removed call to `TryCreateDirectory` from `GetDefaultDataDir` in `src/util.cpp`. #7850

Merged
merged 1 commit into from Apr 14, 2016

Conversation

Projects
None yet
8 participants
@alexreg
Contributor

alexreg commented Apr 9, 2016

@kirkalx

This comment has been minimized.

Show comment
Hide comment
@kirkalx

kirkalx Apr 9, 2016

Contributor

utACK. If there is a good reason for that line to be there on OSX only, then it needs a comment justifying it.

Contributor

kirkalx commented Apr 9, 2016

utACK. If there is a good reason for that line to be there on OSX only, then it needs a comment justifying it.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Apr 10, 2016

Member

It seems that this was there as long as GetDefaultDatadir has been there: https://github.com/bitcoin/bitcoin/tree/d882773789ea3894de7163f7bb880c5b23072882/util.cpp

Member

sipa commented Apr 10, 2016

It seems that this was there as long as GetDefaultDatadir has been there: https://github.com/bitcoin/bitcoin/tree/d882773789ea3894de7163f7bb880c5b23072882/util.cpp

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Apr 10, 2016

Contributor

Looks like this is not for merging...

Contributor

paveljanik commented Apr 10, 2016

Looks like this is not for merging...

@alexreg

This comment has been minimized.

Show comment
Hide comment
@alexreg

alexreg Apr 10, 2016

Contributor

??

Sent from my iPhone

On 10 Apr 2016, at 16:40, paveljanik notifications@github.com wrote:

Looks like this is not for merging...


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub

Contributor

alexreg commented Apr 10, 2016

??

Sent from my iPhone

On 10 Apr 2016, at 16:40, paveljanik notifications@github.com wrote:

Looks like this is not for merging...


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Apr 10, 2016

Contributor

According to the comment mentioned above, you are only trying if this will work. Your real problem is different though, I think. Can you investigate it a bit more please?

pavel$ sudo -u test env | grep HOME
HOME=/Users/pavel
pavel$ sudo -H -u test env | grep HOME
HOME=/Users/test
pavel$ 
Contributor

paveljanik commented Apr 10, 2016

According to the comment mentioned above, you are only trying if this will work. Your real problem is different though, I think. Can you investigate it a bit more please?

pavel$ sudo -u test env | grep HOME
HOME=/Users/pavel
pavel$ sudo -H -u test env | grep HOME
HOME=/Users/test
pavel$ 
@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik
Contributor

paveljanik commented Apr 10, 2016

@kirkalx

This comment has been minimized.

Show comment
Hide comment
@kirkalx

kirkalx Apr 10, 2016

Contributor

@paveljanik The change is for merging. As I said above, if there is a reason for the line to be there it needs to be justified with a comment.

Contributor

kirkalx commented Apr 10, 2016

@paveljanik The change is for merging. As I said above, if there is a reason for the line to be there it needs to be justified with a comment.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Apr 11, 2016

Member
Member

sipa commented Apr 11, 2016

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Apr 11, 2016

Contributor

@sipa yes, but someone should really investigate, how bitcoind/-Qt works when the parent directory (ie. Library/Application Support) is not created/is not writable by the current user or does not exist at all (where/how will we write the config file, blocks, chainstate etc?). I suspect this hides some other problem down in the code...

But if proven to work (not just compile ;-), I'm happy with making OS X the same as "Unix" branch there - @alexreg can you please make the OS X branch to directly return the full path and not changing it in two passes (1. add L/A S, 2. add /Bitcoin) and then test with a real user without that directory or with bad permissions on that directory? How will bitcoind/-Qt work in such cases?

Contributor

paveljanik commented Apr 11, 2016

@sipa yes, but someone should really investigate, how bitcoind/-Qt works when the parent directory (ie. Library/Application Support) is not created/is not writable by the current user or does not exist at all (where/how will we write the config file, blocks, chainstate etc?). I suspect this hides some other problem down in the code...

But if proven to work (not just compile ;-), I'm happy with making OS X the same as "Unix" branch there - @alexreg can you please make the OS X branch to directly return the full path and not changing it in two passes (1. add L/A S, 2. add /Bitcoin) and then test with a real user without that directory or with bad permissions on that directory? How will bitcoind/-Qt work in such cases?

@kirkalx

This comment has been minimized.

Show comment
Hide comment
@kirkalx

kirkalx Apr 11, 2016

Contributor

But if the data directory has been changed to a different value, the only reason GetDefaultDataDir() is called is to display what the default would have been. (I'm ignoring the slightly more complicated behaviour in -qt). So making sure that (part of) that path exists makes no sense.

Contributor

kirkalx commented Apr 11, 2016

But if the data directory has been changed to a different value, the only reason GetDefaultDataDir() is called is to display what the default would have been. (I'm ignoring the slightly more complicated behaviour in -qt). So making sure that (part of) that path exists makes no sense.

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Apr 11, 2016

Contributor

... exist and is writable...

Contributor

paveljanik commented Apr 11, 2016

... exist and is writable...

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Apr 11, 2016

Member

utACK.
Creating ~/Library/Application Support is useless (on OSX).

Member

jonasschnelli commented Apr 11, 2016

utACK.
Creating ~/Library/Application Support is useless (on OSX).

@alexreg

This comment has been minimized.

Show comment
Hide comment
@alexreg

alexreg Apr 11, 2016

Contributor

I’m not sure exactly what you mean. I don’t really code C++. Could you give me the diff please, and I’ll apply it?

I’ve already tested both those cases though, and it works as it should.

On 11 Apr 2016, at 08:30, paveljanik notifications@github.com wrote:

@sipa https://github.com/sipa yes, but someone should really investigate, how bitcoind/-Qt works when the parent directory (ie. Library/Application Support) is not created/is not writable by the current user or does not exist at all (where/how will we write the config file, blocks, chainstate etc?). I suspect this hides some other problem down in the code...

But if proven to work (not just compile ;-), I'm happy with making OS X the same as "Unix" branch there - @alexreg https://github.com/alexreg can you please make the OS X branch to directly return the full path and not changing it in two passes (1. add L/A S, 2. add /Bitcoin) and then test with a real user without that directory or with bad permissions on that directory? How will bitcoind/-Qt work in such cases?


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub #7850 (comment)

Contributor

alexreg commented Apr 11, 2016

I’m not sure exactly what you mean. I don’t really code C++. Could you give me the diff please, and I’ll apply it?

I’ve already tested both those cases though, and it works as it should.

On 11 Apr 2016, at 08:30, paveljanik notifications@github.com wrote:

@sipa https://github.com/sipa yes, but someone should really investigate, how bitcoind/-Qt works when the parent directory (ie. Library/Application Support) is not created/is not writable by the current user or does not exist at all (where/how will we write the config file, blocks, chainstate etc?). I suspect this hides some other problem down in the code...

But if proven to work (not just compile ;-), I'm happy with making OS X the same as "Unix" branch there - @alexreg https://github.com/alexreg can you please make the OS X branch to directly return the full path and not changing it in two passes (1. add L/A S, 2. add /Bitcoin) and then test with a real user without that directory or with bad permissions on that directory? How will bitcoind/-Qt work in such cases?


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub #7850 (comment)

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Apr 11, 2016

Contributor

Just return pathRet / "Library/Application Support/Bitcoin" ...

Contributor

paveljanik commented Apr 11, 2016

Just return pathRet / "Library/Application Support/Bitcoin" ...

@alexreg

This comment has been minimized.

Show comment
Hide comment
@alexreg

alexreg Apr 11, 2016

Contributor

Oh right. Done now.

Contributor

alexreg commented Apr 11, 2016

Oh right. Done now.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Apr 12, 2016

Member

utACK 0f2fd2cec05b90d071b30eb522061e1c5144b367 after squash

Member

sipa commented Apr 12, 2016

utACK 0f2fd2cec05b90d071b30eb522061e1c5144b367 after squash

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Apr 12, 2016

Contributor

Please squash both commits into one.

Contributor

paveljanik commented Apr 12, 2016

Please squash both commits into one.

@alexreg

This comment has been minimized.

Show comment
Hide comment
@alexreg

alexreg Apr 12, 2016

Contributor

I don’t use Git much, so no idea how to do that.

On 12 Apr 2016, at 21:00, paveljanik notifications@github.com wrote:

Please squash both commits into one.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub #7850 (comment)

Contributor

alexreg commented Apr 12, 2016

I don’t use Git much, so no idea how to do that.

On 12 Apr 2016, at 21:00, paveljanik notifications@github.com wrote:

Please squash both commits into one.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub #7850 (comment)

@kirkalx

This comment has been minimized.

Show comment
Hide comment
@kirkalx

kirkalx Apr 12, 2016

Contributor

Wladimir explains in #7458, or just do a google search

Contributor

kirkalx commented Apr 12, 2016

Wladimir explains in #7458, or just do a google search

Removed call to `TryCreateDirectory` from `GetDefaultDataDir` in `src…
…/util.cpp`.

See #7845 (comment).
Also refactored `GetDefaultDataDir` function to return path for Mac in one expression.
@fanquake

This comment has been minimized.

Show comment
Hide comment
@fanquake
Member

fanquake commented Apr 13, 2016

utACK 41dbc48

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Apr 13, 2016

Contributor

ACK 41dbc48

Contributor

paveljanik commented Apr 13, 2016

ACK 41dbc48

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Apr 14, 2016

Member

Hmm this is indeed weird, why would this need a TryCreateDirectory on OSX.

Happy that this solves your problem.

utACK 41dbc48

Member

laanwj commented Apr 14, 2016

Hmm this is indeed weird, why would this need a TryCreateDirectory on OSX.

Happy that this solves your problem.

utACK 41dbc48

@laanwj laanwj merged commit 41dbc48 into bitcoin:master Apr 14, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Apr 14, 2016

Merge #7850: Removed call to `TryCreateDirectory` from `GetDefaultDat…
…aDir` in `src/util.cpp`.

41dbc48 Removed call to `TryCreateDirectory` from `GetDefaultDataDir` in `src/util.cpp`. (Alexander Regueiro)
@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Apr 14, 2016

Contributor

I digged in the git history, but could not find something important. But here is the wild guess: on OS X, the config files are in ~/Library/Application Support. On other unixes, config directory .bitcoin is directly in the home directory. And the wild guess is that this "mkdir -p parent_directory" had to be somewhere, so someone decided it could be here 8)

Contributor

paveljanik commented Apr 14, 2016

I digged in the git history, but could not find something important. But here is the wild guess: on OS X, the config files are in ~/Library/Application Support. On other unixes, config directory .bitcoin is directly in the home directory. And the wild guess is that this "mkdir -p parent_directory" had to be somewhere, so someone decided it could be here 8)

@alexreg

This comment has been minimized.

Show comment
Hide comment
@alexreg

alexreg Apr 14, 2016

Contributor

Thanks for accepting this, guys.

Sent from my iPhone

On 14 Apr 2016, at 12:28, Wladimir J. van der Laan notifications@github.com wrote:

Hmm this is indeed weird, why would this need a TryCreateDirectory on OSX.

Happy that this solves your problem.

utACK 41dbc48


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub

Contributor

alexreg commented Apr 14, 2016

Thanks for accepting this, guys.

Sent from my iPhone

On 14 Apr 2016, at 12:28, Wladimir J. van der Laan notifications@github.com wrote:

Hmm this is indeed weird, why would this need a TryCreateDirectory on OSX.

Happy that this solves your problem.

utACK 41dbc48


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Apr 27, 2016

Removed call to `TryCreateDirectory` from `GetDefaultDataDir` in `src…
…/util.cpp`.

See bitcoin#7845 (comment).
Also refactored `GetDefaultDataDir` function to return path for Mac in one expression.

Github-Pull: #7850
Rebased-From: 41dbc48
@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Jun 9, 2016

Member

Backported as part of #7938. Removing label 'Needs backport'.

Member

MarcoFalke commented Jun 9, 2016

Backported as part of #7938. Removing label 'Needs backport'.

zander added a commit to zander/bitcoinclassic that referenced this pull request Jun 16, 2016

Removed call to `TryCreateDirectory` from `GetDefaultDataDir` in `src…
…/util.cpp`.

See bitcoin#7845 (comment).
Also refactored `GetDefaultDataDir` function to return path for Mac in one expression.

Github-Pull: #7850
Rebased-From: 41dbc48

nomnombtc added a commit to nomnombtc/bitcoin that referenced this pull request Nov 12, 2016

Removed call to `TryCreateDirectory` from `GetDefaultDataDir` in `src…
…/util.cpp`.

See bitcoin#7845 (comment).
Also refactored `GetDefaultDataDir` function to return path for Mac in one expression.

Github-Pull: #7850
Rebased-From: 41dbc48

nomnombtc added a commit to nomnombtc/bitcoin that referenced this pull request Nov 12, 2016

Removed call to `TryCreateDirectory` from `GetDefaultDataDir` in `src…
…/util.cpp`.

See bitcoin#7845 (comment).
Also refactored `GetDefaultDataDir` function to return path for Mac in one expression.

Github-Pull: #7850
Rebased-From: 41dbc48

nomnombtc added a commit to nomnombtc/bitcoin that referenced this pull request Nov 13, 2016

Removed call to `TryCreateDirectory` from `GetDefaultDataDir` in `src…
…/util.cpp`.

See bitcoin#7845 (comment).
Also refactored `GetDefaultDataDir` function to return path for Mac in one expression.

Github-Pull: #7850
Rebased-From: 41dbc48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment