Skip to content

Fix user-specific log file initialization#117

Open
Raihan93-coder wants to merge 5 commits into
BusKill:devfrom
Raihan93-coder:fix-user-specific-logging
Open

Fix user-specific log file initialization#117
Raihan93-coder wants to merge 5 commits into
BusKill:devfrom
Raihan93-coder:fix-user-specific-logging

Conversation

@Raihan93-coder
Copy link
Copy Markdown

Problem

PermissionError caused by shared /tmp/buskill.log across users

To verify if it is just a read write permission I tested the program in my VM (ParrotOS) and the problem still exist so next thing I did was to check the file permission and over there I found something interesting -rw-r--r-- (Which means the owner only have the right to write into the file) So I tested it for 2 users root and a regular and again (My naive brain was thinking when I first generate the log file as regular user I can write it when I ran as root) but NO. So I further checked about the permission problem and I was convinced we can't change the permission. Next thought process the program was running smooth when I try to delete the log file (But this is also a naive as if system crash and someone wanted to check the log file this program will not store then what is the reason for log itself 😕. So finally I took inspiration from The author of the problem itself and implemented it

Solution

How did I changed it to user cache ?

I initial had the most naive Idea that was actually finding $HOME but then it will another head weight so I was looking through the program and found out about the CACHE function of its own so my main Idea right now was to implement this user cache with the existing architecture. So I read through the log generating logic and found a way to implement the cache (Thank's to OOP's)

Problem Faced

Did I face any problem while solving this ?

Yes I have faced a major problem when I was implementing that was

self.LOG_FILE_PATH = logger.root.handlers[0].baseFilename

Now the problem with this line was when the log file was not initialised it has nothing to point and we have changed the log file not to be generated in the tmp DIR but a user cache DIR so basically the LOG_FILE_PATH had to wait before indexing something so the initial value of the LOG_FILE_PATH must be None (Logically speaking) so updated that to by adding a small ternary operator

self.LOG_FILE_PATH = logger.root.handlers[0].baseFilename if logger.root.handlers else None


This PR and the new comments in the code was written explicitly by Me. No AI was involved in doing any contribution to this PR
Sincerely
Raihan 🤓

@maltfield
Copy link
Copy Markdown
Member

Thanks for the PR :)

This fixes #75

@maltfield
Copy link
Copy Markdown
Member

maltfield commented May 25, 2026

I reviewed the code. Sorry, but I think the solution doesn't work.

We use the CACHE dir to store untrusted files that we download from the Internet (eg when doing an app upgrade). If the files don't pass the cryptographic signature verification, then we wipe the CACHE dir. With this solution, we'd be wiping the buskill debug log too.

https://github.com/BusKill/buskill-app/blob/924ca985f8ed2ccec90312e443138dcad45fb35d/src/packages/buskill/__init__.py#L1702-1709

Merging this code change would mean that, if there's some bug with the app's self-updating, then users who are experiencing the bug wouldn't be able to post their debug log :(

As stated in the original ticket, I think it makes more sense to catch the Permission Denied exception and then, if-and-only-if this happens, just change the buskill.log filename by appending a datestamp. To avoid spaces or non-POSIX special characters,, I would prefer %Y%m%d_%H%M%S (eg buskill.20260525_134245.log)

Would you be open to coding these changes?

@Raihan93-coder
Copy link
Copy Markdown
Author

Okay, that make sense as we wanted the log file to be present in the system and i didn't notice the wipe cache functionality it was a slip-up from my side. Sorry for that
Now the new idea proposed sounds good but the naming convention feels goofy like one will have one and other doesn't and all
So, I would like to propose that let all the log file contain the timestamp irrespective of the user or the conflict

If you have any suggestion please let me know 🤓

@Raihan93-coder
Copy link
Copy Markdown
Author

I have made the changes according to your idea, If you wanted to change anything let me know
Also just have a review of my idea too, It is not anything related to bug but just a design choice and you are free to choose anything :)

@maltfield
Copy link
Copy Markdown
Member

Attempting to get the unicode_warn workflow to run

@BusKill BusKill locked and limited conversation to collaborators May 26, 2026
@BusKill BusKill unlocked this conversation May 26, 2026
@github-actions
Copy link
Copy Markdown

INFO: No unicode characters found in PR's commits

(source)

@maltfield
Copy link
Copy Markdown
Member

let all the log file contain the timestamp irrespective of the user or the conflict
...
Also just have a review of my idea too

I don't like that because it just makes it one degree harder to explain to users how to find and submit their debug log files. >99% of the time (at least on Linux), it'll be a hardcoded file path /tmp/buskill.log.

Anything we can do to lower the barrier of entry to make it easier for users to find & upload their debug logs is better, as it's sometimes like pulling teeth just to get users who report bugs to include the debug log in their report (and it's immensely helpful and time saving for us to have this debug log!)

Otherwise, please see my review notes. Move the import up, and I think we're ready to merge this. Thanks :)

@github-actions
Copy link
Copy Markdown

INFO: No unicode characters found in PR's commits

(source)

@Raihan93-coder
Copy link
Copy Markdown
Author

Thank you for the review and I have moved the imports up ( below the imports banner ). Also I do have a question I just tested it and found something interesting and I don't think it is a bug but feels like one,

Let the root run this program first a /tmp/buskill.log will be created only root has access to write.
Now lets say a regular user ran this program so due to permission error it will create a buskill..log file as filename ( A new file ) and then again a regular user (Same user) run this again too will hit the permission error irrespective of the log file existing but just with a different name and will create again a new log file with new timestamp ( eg: /tmp/buskill..log ). So should we access this problem or is it fine as it is

@maltfield
Copy link
Copy Markdown
Member

maltfield commented May 27, 2026

After a quick test, I see that it's working .. but, even when it writes to the timestammped log file, it outputs to the user the wrong file name

usb1.__version__:|3.1.0|
===============================================================================
INFO: Writing to log file '/tmp/buskill.log'

...this happened when it was actually writing to /tmp/buskill.20260526_195703.log.

I think the best way to fix this would be to put these lines of code into a function (say def start_logging( log_file_path ))
`

		logging.basicConfig(
	 		filename = log_file_path,
	 		filemode = 'a',
	 		format = '%(asctime)s,%(msecs)d %(name)s %(levelname)s %(message)s',
	 		datefmt = '%H:%M:%S',
	 		level = logging.DEBUG
		)

...where log_file_path is an argument in the function. And, instead of using a distinct variable fallback_log, we just overwrite the value of log_file_path to the timestamped version and call the function inside the exception catch.

Please put the function under a FUNCTIONS section, as you see done in other scripts. For example:

################################################################################
# FUNCTIONS #
################################################################################
# TODO: figure out why dialog's Label fonts aren't being updated
# TODO: update this to also update RecycleView's data dicts
def update_font_recursive(widget):
# is widget actually a list of widgets?
if type(widget) == type(list()) \

@maltfield
Copy link
Copy Markdown
Member

maltfield commented May 27, 2026

Let the root run this program first a /tmp/buskill.log will be created only root has access to write.
Now lets say a regular user ran this program so due to permission error it will create a buskill..log file as filename ( A new file ) and then again a regular user (Same user) run this again too will hit the permission error irrespective of the log file existing but just with a different name and will create again a new log file with new timestamp ( eg: /tmp/buskill..log ). So should we access this problem or is it fine as it is

I don't think we need to fix this edge case. It's fine.

Probably we should be warning users not to run the app as root. Running the BusKill app as root is a bad idea, anyway. But the app shouldn't crash if that does happen..

@Raihan93-coder
Copy link
Copy Markdown
Author

I don't think we need to fix this edge case. It's fine.

Probably we should be warning users not to run the app as root. Running the BusKill app as root is a bad idea, anyway. But the app shouldn't crash if that does happen..

Ok, Then I will just make the changes that you have suggested and then push it :)

@github-actions
Copy link
Copy Markdown

INFO: No unicode characters found in PR's commits

(source)

@Raihan93-coder
Copy link
Copy Markdown
Author

I have made the changes, I created a functions banner in the main.py file itself if you wanted me to make any changes just let me know

Comment thread src/main.py
Copy link
Copy Markdown
Member

@maltfield maltfield May 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

line 74 is now redundant. can you please delete it?

And move the comment below, to inside the exception where it's relevant.

Comment thread src/main.py
)
# adding a fallback log to avoid PermissionError
# This creates a new file that appends the timestamp to the log file
log_file_path = os.path.join( tempfile.gettempdir() , 'buskill.log' )
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

line 74 is now redundant. can you please delete it?

Comment thread src/main.py
datefmt = '%H:%M:%S',
level = logging.DEBUG
)
# adding a fallback log to avoid PermissionError
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please move the comment below, to inside the exception where it's relevant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants