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

[master] log directory not created in time when running with pre-made configs #1865

Closed
Torxed opened this issue Jun 12, 2023 · 8 comments · Fixed by #1874
Closed

[master] log directory not created in time when running with pre-made configs #1865

Torxed opened this issue Jun 12, 2023 · 8 comments · Fixed by #1874

Comments

@Torxed
Copy link
Member

Torxed commented Jun 12, 2023

screenshot

@Torxed
Copy link
Member Author

Torxed commented Jun 12, 2023

simple mkdir -p /var/log/archinstall before running is a workaround.

@codefiles
Copy link
Contributor

Is that not what is happening with the below code?

check_log_permissions()

def check_log_permissions():
filename = storage.get('LOG_FILE', None)
log_dir = storage.get('LOG_PATH', Path('./'))
if not filename:
raise ValueError('No log file name defined')
log_file = log_dir / filename
try:
log_dir.mkdir(exist_ok=True, parents=True)

I think this might be the same race condition I warned about here #1831 (comment) just with the directory existing rather than permissions.

@svartkanin
Copy link
Collaborator

svartkanin commented Jun 13, 2023

The error here isn't the actual log problem that is just the result of the upper error which is the actual problem Can't have partition outside disk

But then yes, the logging will break because the setup of the logging hasn't actually happened.

@Torxed
Copy link
Member Author

Torxed commented Jun 13, 2023

We should probably move the first log instance + creation to the very start of archinstall when we log the sysinfo.

The disk thing was a separate issue that I'm working on while experimenting with /boot being two partitions. Got the sectors wrong ^^

@svartkanin
Copy link
Collaborator

I will yield to @codefiles suggestions to always verify log permissions and the directory. All options that went through my head seemed worse :)

I raised a PR to address it

@codefiles
Copy link
Contributor

I really would not consider that yielding to me. What I have suggested is to verify when writing to the log, which is what was done prior to 89cefb9. To me what you implemented previously with _check_log_permissions() and what you are doing in the new pull request appear to be an attempt at optimization. As I demonstrated here #1831 (comment) the optimization saves virtually no time.

@svartkanin
Copy link
Collaborator

I wasn't really trying to optimise it for time, but it just feels wrong performing a file permission check every time when writing to a file that was all I was trying to avoid

@codefiles
Copy link
Contributor

There is a way where checking once is adequate. 🤪

with log_file.open('a') as fp:
    rest_of_program(fp)

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 a pull request may close this issue.

3 participants