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

create logs in temp directory #51

Merged
merged 5 commits into from
Oct 5, 2023
Merged

create logs in temp directory #51

merged 5 commits into from
Oct 5, 2023

Conversation

landam
Copy link
Collaborator

@landam landam commented Feb 17, 2023

Creating logs in installation dependent directory __file__/../.. may cause problems due to missing write permissions. Using pywsdp library in QGIS plugin on Windows leads to error message below:

Unable to create C:\Users\logs

This draft PR creates logs in temporary directory instead of installation dependent path.

Tasks before merging:

  • user should have possibility to defined own log directory (as constructor's argument / method / or env variable)

@landam landam marked this pull request as draft February 17, 2023 11:26
@landam landam added this to the v2.2.0 milestone Feb 17, 2023
@landam
Copy link
Collaborator Author

landam commented Jul 26, 2023

@lindakladivova Any progress?

@lindakarlovska
Copy link
Collaborator

lindakarlovska commented Jul 26, 2023

@lindakladivova Any progress?

I think that this PR already fixes the issue in the way that it creates the default log directory in the Temporary directory.

There is actually the @log_adresar.setter but the problem is that for some reason Sphinx only read the documentation for the @Property method, not the @property.setter one at all (please see https://stackoverflow.com/questions/71713284/types-of-setter-arguments-not-showing-in-sphinx-documentation).

I have added the logger info message to the setter and extended the documentation.

@lindakarlovska
Copy link
Collaborator

Btw WSDP test services are in maintenance till the 28th so pytest fails (https://www.cuzk.cz/Aplikace-DP-do-KN/Aplikace-DP-do-KN/Archiv-DP.aspx).

@landam landam marked this pull request as ready for review August 6, 2023 12:20
@landam
Copy link
Collaborator Author

landam commented Aug 6, 2023

@lindakladivova I tested PR in Python 3.11. There is a warning which should be nice to avoid before merging:

warning:/usr/lib/python3.11/tempfile.py:1034: ResourceWarning: Implicitly cleaning up 
              _warnings.warn(warn_message, ResourceWarning)
             
             traceback: File "/home/martin/git/ctu-geoforall-lab/qgis-vfk-plugin/../qgis-vfk-plugin/mainApp.py", line 259, in runDownloadingPosidents
              ctios = CtiOS([
              File "/home/martin/git/ctu-geoforall-lab/qgis-vfk-plugin/../qgis-vfk-plugin/pywsdp/modules/CtiOS/__init__.py", line 50, in __init__
              super().__init__(creds, trial=trial)
              File "/home/martin/git/ctu-geoforall-lab/qgis-vfk-plugin/../qgis-vfk-plugin/pywsdp/base/__init__.py", line 42, in __init__
              self._log_adresar = self._set_default_log_dir()
              File "/home/martin/git/ctu-geoforall-lab/qgis-vfk-plugin/../qgis-vfk-plugin/pywsdp/base/__init__.py", line 105, in _set_default_log_dir
              log_dir = tempfile.TemporaryDirectory().name
              File "/usr/lib/python3.11/weakref.py", line 590, in __call__
              return info.func(*info.args, **(info.kwargs or {}))
              File "/usr/lib/python3.11/tempfile.py", line 1034, in _cleanup
              _warnings.warn(warn_message, ResourceWarning)

@lindakarlovska
Copy link
Collaborator

Probably the best way how to avoid this is not to use tempfile.TemporaryDirectory in case we do not want to delete the dir after the process is completed. I have changed the way how the log_dir is created. It takes the temp directory and inside it, it creates a new directory based on the service name (e.g. ctiOS). So, in our case the resulting path to log file will be e.g. /tmp/ctiOS/17_58_51_09_08_2023.log .

@landam
Copy link
Collaborator Author

landam commented Oct 3, 2023

@lindakladivova I tested PR and it's ready to be merged. Please create release-2.2 branch and release 2.2.0.

@lindakarlovska lindakarlovska merged commit 44b27f5 into master Oct 5, 2023
3 checks passed
@lindakarlovska lindakarlovska deleted the default_log_dir branch October 5, 2023 07:01
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