Skip to content
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

Logger fails to create log file when channel name includes forward slash #214

Closed
2 tasks done
osa1 opened this issue Jun 21, 2020 · 20 comments
Closed
2 tasks done

Logger fails to create log file when channel name includes forward slash #214

osa1 opened this issue Jun 21, 2020 · 20 comments
Labels
Milestone

Comments

@osa1
Copy link
Owner

osa1 commented Jun 21, 2020

@Kabouik reports in #211

I think the issue might be related to Gitter rooms being formatted with a slash in their name, which must conflict with the way tiny sets filenames for logs. You can see above in my configuration file that the channel I wanted to join on irc.gitter.im was "#jarun/nnn". If this is really the cause, then perhaps an acceptable workaround would be to replace / with another character when creating new log files, such as -?

In other words when the channel name contains / logger fails to create the log
file and reports this in "mentions" tab:

Logger error: Os { code: 2, kind: NotFound, message: "No such file or directory" }

Two TODOs:

  • The error message should show the file path
  • We should handle channel names with /s in them (as far as I can see RFC 2812 allows this)
@osa1 osa1 added the bug label Jun 21, 2020
@osa1 osa1 added this to the 0.6.0 milestone Jun 21, 2020
osa1 added a commit that referenced this issue Jun 21, 2020
Previously when we couldn't create a file we'd report

    Logger error: Os { code: 2, kind: NotFound, message: "No such file or directory" }

Now

    Logger error: Couldn't open file "<file path>": No such file or directory (os error 2)

(#214)
@osa1 osa1 closed this as completed in 5a87269 Jun 21, 2020
@osa1
Copy link
Owner Author

osa1 commented Jun 21, 2020

@Kabouik thanks again for reporting this. Please let me know if you encounter any other problems.

@Kabouik
Copy link

Kabouik commented Jun 21, 2020

Thanks for the quick fix. This solved the issue on my computer, but for some reason the issue persists on my SailfishOS device. tiny 0.5.1 (4af06df), installed with native tls and --force. The issue is probably on my end since it works fine on the computer, but both are using the same config.yaml so I don't know where to start troubleshooting yet.

@osa1
Copy link
Owner Author

osa1 commented Jun 21, 2020

@Kabouik what exactly does the error message say? I updated the error logging today so you should see a more detailed error message that also show the problematic file path.

@Kabouik
Copy link

Kabouik commented Jun 21, 2020

Actually that is what surprised me, I should have said that in my first message: the error is the same as the one posted here, which is why I double checked the version to make sure it included your fix. That's also why I believe the issue may be on my end only.

@osa1
Copy link
Owner Author

osa1 commented Jun 21, 2020

It seems like you need to recompile and reinstall tiny. Make sure tiny --version prints tiny 0.5.1 (4af06df).

@Kabouik
Copy link

Kabouik commented Jun 21, 2020

It does.

@osa1
Copy link
Owner Author

osa1 commented Jun 21, 2020

What does the error message say exactly?

@Kabouik
Copy link

Kabouik commented Jun 21, 2020

Can't create logger: on such file or directory (os error2)

@osa1
Copy link
Owner Author

osa1 commented Jun 21, 2020

That can't be the actual message. If you paste the actual message I'll try to help.

@Kabouik
Copy link

Kabouik commented Jun 21, 2020

I had something else in my clipboard on the device I didn't want to lose, so I just typed it and did not notice the two small typos (sorry):

Screenshot_20200621_001

Of note, as opposed to the initial issue, the message appears only once even if I leave tiny running. I believe with the previous version, when there was an issue with / in channel names, the message was showing regularly (as if tiny was attempting to create the log file several times).

osa1 added a commit that referenced this issue Jun 22, 2020
Previously we would report this in "mentions" tab:

    Can't create logger: No such file or directory (os error 2)

With this patch:

    Could not create log directory "/home/omer/no/such/dir": No such file or directory (os error 2)

Not sure about the wording, but showing the path in the error message is helpful
to realize the typos or when a user reports the error to issue tracker.

(#214)
@osa1
Copy link
Owner Author

osa1 commented Jun 22, 2020

@Kabouik thanks.. so the problem is logger can't create the log directory. I slightly improved the error message in b7dfd9c so you'll now see the path it failed to create. I think the most common reason for this is you have /foo/bar/baz as your log directory but /foo/bar doesn't exist. We currently don't create parent directory, we assume that it exists. Perhaps we should change this to create all directories. Any opinions on that? cc @trevarj

@trevarj
Copy link
Contributor

trevarj commented Jun 22, 2020

Yeah, I think using create_dir_all would be better.

osa1 added a commit that referenced this issue Jun 22, 2020
@osa1
Copy link
Owner Author

osa1 commented Jun 22, 2020

OK, done.

@Kabouik
Copy link

Kabouik commented Jun 22, 2020

I am sorry to say that I realized now (thanks to the improved error message in b7dfd9c) that the issue on the SailfishOS device was probably due to me synchronizing the config file between the two devices after the forward slash fix. I didn't notice that the path to the log files was written in the form of /home/user/ instead of $HOME, which of course caused issues due to different user names between the two machines. How dumb. I'm sorry if I wasted your time. The improved error reports will surely be useful though, and I hope that b640776 is stilll a progress for tiny regardless.

@osa1
Copy link
Owner Author

osa1 commented Jun 22, 2020

No worries -- as you say these improvements will be helpful to others. They could've even helped you by showing the full path as you'd realize the mistake there.

Btw, we have a WIP PR #171 to support $HOME in log dir path. We couldn't find a good design so it's on hold currently. If you have any opinions on that let us know in the PR.

@Kabouik
Copy link

Kabouik commented Jun 22, 2020

Uh, the master version doesn't have that already? Because that's what I have put in my config.yml since my message above, and it solved the issue for me:

# Where to put log files
log_dir: '$HOME/.config/tiny/tiny_logs'

Is that really what #171 is about? By the way I think #171 is a good idea, I'm not a big fan of having passwords written in plain text files, and reading env variables would allow conveying the password from pass or secret-tool for instance.

@osa1
Copy link
Owner Author

osa1 commented Jun 23, 2020

@Kabouik If you use $HOME in your log dir path a directory named $HOME will be created, so for example if you run tiny in /home/user/tiny you'll find your logs in /home/user/tiny/$HOME :-)

Perhaps we should prioritize #171.

@Kabouik
Copy link

Kabouik commented Jun 23, 2020

You're right, confirmed! With log_dir: '$HOME/.config/tiny/tiny_logs' in my configuration, logs are stored in /home/kabouik/\$HOME/.config/tiny/tiny_logs/. Now I understand why I couldn't find the latest history in my logs in the config folder!

But then if tiny automatically created the $HOME/ directory in /home/kabouik, can't I just use log_dir: .config/tiny/tiny_logs and have it automatically created in my real $HOME regardless of the username?

@osa1
Copy link
Owner Author

osa1 commented Jun 23, 2020

But then if tiny automatically created the $HOME directory in /home/kabouik, can't I just use log_dir: .config/tiny/tiny_logs and have it automatically created in my real $HOME regardless of the username?

This would work if you make sure you run tiny in $HOME, because .config/tiny/tiny_logs is relative to the current working directory. Perhaps you could write a wrapper script that changes the working directory to $HOME first and then runs tiny.

@Kabouik
Copy link

Kabouik commented Jun 23, 2020

I see. I ran tiny from ~/ indeed. I can do a script or alias tiny to cd ~/ && tiny for now (or just put the real user names in my config files on different machines), but for the long term, support for $HOME or ~/ will be great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants