Updates the XDG path creation to create needed dirs and panic on failure #113

sloum · 2019-12-04T03:27:10Z

sloum commented

2019-12-04 03:27:10 +00:00

It was reported that when there is no XDG_CONFIG_HOME and no ~/.config directory exists that Bombadillo silently dies. To fix this jboverf suggested, very correctly, that we use os.MkdirAll to make sure all elements of the path get built out. This has been done. If the attempt to make the path elements fails then the program will panic with a message explaining what it was trying to do.

This was reported as being an issue present on OpenBSD (and likely NetBSD), but I think it likely is present on all systems (which is a big bummer). I am hopeful that a 2.1.0 release will come about before the end of the month with a few such updates that do usability and stability fixes to the 2.0.0 build.

This being merged in should close out issue #112

It was reported that when there is no `XDG_CONFIG_HOME` _and_ no `~/.config` directory exists that Bombadillo silently dies. To fix this jboverf suggested, very correctly, that we use `os.MkdirAll` to make sure all elements of the path get built out. This has been done. If the attempt to make the path elements fails then the program will panic with a message explaining what it was trying to do. This was reported as being an issue present on OpenBSD (and likely NetBSD), but I think it likely is present on all systems (which is a big bummer). I am hopeful that a 2.1.0 release will come about before the end of the month with a few such updates that do usability and stability fixes to the 2.0.0 build. This being merged in should close out issue #112

sloum added this to the 2.1.0 milestone 2019-12-04 03:27:10 +00:00

sloum added the

bug

urgent

labels 2019-12-04 03:27:10 +00:00

jboverf was assigned by sloum

2019-12-04 03:27:15 +00:00

sloum referenced this pull request

2019-12-04 03:29:01 +00:00

Startup failure when $XDG_CONFIG_HOME is unset and ~/.config doesn't exist #112

asdf commented

2019-12-04 03:35:03 +00:00

This issue will occur for any directory specified in "configlocation" that does not exist.

What are your thoughts about doing this as part of saveConfig() instead?

Also, is this something that we could add error messages for?

This issue will occur for any directory specified in "configlocation" that does not exist. What are your thoughts about doing this as part of `saveConfig()` instead? Also, is this something that we could add error messages for?

sloum commented

2019-12-04 04:00:38 +00:00

@asdf You are right. This could affect any path put into defaults.go. As such, I have moved the logic to loadConfig. This only gets called once at the beginning of a session, which would be the proper place to error out if we are going to, rather than saveConfig which gets called a lot (and not right at the beginning of a session). I think this makes sense. It also showed me a place where we were not joining paths correctly, so that got updated as well. I am now passing the error in accordance with how loadConfig has been set up.

@asdf You are right. This could affect any path put into `defaults.go`. As such, I have moved the logic to `loadConfig`. This only gets called once at the beginning of a session, which would be the proper place to error out if we are going to, rather than `saveConfig` which gets called a lot (and not right at the beginning of a session). I think this makes sense. It also showed me a place where we were not joining paths correctly, so that got updated as well. I am now passing the error in accordance with how `loadConfig` has been set up.

asdf commented

2019-12-04 09:06:56 +00:00

Another non-filepath join...I'm really surprised at how hard they are to find.

I found an issue where this implementation will produce a directory named configlocation + bombadillo.ini. I've made a small change to the MkdirAll using only configlocation.

I also made a change to the error message. I can't tell if it's useful yet, as explained below...

There are other situations that will still result in this same situation:

configlocation is empty
configlocation already exists as a file
if configlocation can't be created due to permissions

I don't think any of these should be handled differently, but some error should be returned. It seems that when initialisation fails, the program panics and (maybe?) is meant to return these messages, but they don't appear.

I think this should probably go in as it is, and I'll raise another issue to investigate where the error messages are going.

Another non-filepath join...I'm really surprised at how hard they are to find. I found an issue where this implementation will produce a directory named `configlocation + bombadillo.ini`. I've made a small change to the `MkdirAll` using only `configlocation`. I also made a change to the error message. I can't tell if it's useful yet, as explained below... There are other situations that will still result in this same situation: - `configlocation` is empty - `configlocation` already exists as a file - if `configlocation` can't be created due to permissions I don't think any of these should be handled differently, but some error should be returned. It seems that when initialisation fails, the program panics and (maybe?) is meant to return these messages, but they don't appear. I think this should probably go in as it is, and I'll raise another issue to investigate where the error messages are going.

👍 1

asdf referenced this pull request

2019-12-04 09:14:10 +00:00

Where do error messages from `panic(err) go? #114

jboverf closed this pull request

2019-12-04 12:39:04 +00:00

sloum commented

2019-12-04 16:53:15 +00:00

Oh! Thanks for catching that I had joined when I didnt need to for the MkdirAll. I am surprised I didnt catch that. I definitely agree that messaging should be provided to the user. If possible trying out a situation that forces each of the three issues to see what error gets generated would be good. Then we can make sure the messaging makes it clear what is happening, where, and how it can be resolved.

Oh! Thanks for catching that I had joined when I didnt need to for the `MkdirAll`. I am surprised I didnt catch that. I definitely agree that messaging should be provided to the user. If possible trying out a situation that forces each of the three issues to see what error gets generated would be good. Then we can make sure the messaging makes it clear what is happening, where, and how it can be resolved.

sloum referenced this pull request

2019-12-06 03:23:37 +00:00

Startup failure when $XDG_CONFIG_HOME is unset and ~/.config doesn't exist #112

Sign in to join this conversation.

No reviewers