Skip to content

Address blf GzipFile negative seek error #1385

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 40 commits into from
Closed

Address blf GzipFile negative seek error #1385

wants to merge 40 commits into from

Conversation

@j-c-cook
Copy link
Contributor

@j-c-cook j-c-cook commented Aug 27, 2022 •
edited
Loading

The current implementation of the BLFWriter requires that the file be negatively sought (#1379 (comment)). The cpython implementation does not allow for GzipFiles to be negatively sought while in write mode. Modifying gzip is outside of the scope of this project. I see two options here:

  1. Modify the BLFWriter to close, seek then re-open (@christiansandberg says this was previously being done)
  2. Inform the user that the GzipFile format is not compatible with the BLFWriter (later enhancing python-can so that zip formats that can be negatively sought are available)

I'm proposing option 2.

closes #1378

@codecov
Copy link

codecov bot commented Aug 27, 2022 •
edited
Loading

Codecov Report

Merging #1385 (7c0e650) into develop (5f485db) will decrease coverage by 5.41%.
The diff coverage is 78.84%.

Additional details and impacted files 
@@             Coverage Diff             @@
##           develop    #1385      +/-   ##
===========================================
- Coverage    65.28%   59.87%   -5.42%     
===========================================
  Files           81       81              
  Lines         8830     8726     -104     
===========================================
- Hits          5765     5225     -540     
- Misses        3065     3501     +436

@j-c-cook
Copy link
Contributor Author

j-c-cook commented Aug 27, 2022 •
edited
Loading

Now that I've tried out other zip formats:

I'm uncertain what should be done about #1378.

j-c-cook
j-c-cook commented Aug 27, 2022
View reviewed changes
can/io/blf.py
Comment on lines +398 to +402
if type(file) == gzip.GzipFile:
raise ValueError(
f"The BLFWriter is currently incompatible with "
f"{gzip.GzipFile.__name__}s. Try using a different zip format."
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm unaware of any zip format that could be made compatible with the current implementation of BLFWriter.

@j-c-cook j-c-cook mentioned this pull request Aug 27, 2022
Closed
@j-c-cook j-c-cook marked this pull request as draft August 27, 2022 22:38
@j-c-cook j-c-cook changed the title Raise error when blf GzipFile is requested Address blf GzipFile negative seek error Aug 27, 2022
@zariiii9003
Copy link
Collaborator

I would move the check here:

python-can/can/io/logger.py

Line 107 in cd51ec4

real_suffix = pathlib.Path(filename).suffixes[-2].lower()

A comparison of real_suffix against a list of incompatible suffixes (all binary formats) should be sufficient.

I'd suggest to release 4.1.0 once this one is merged.

@j-c-cook j-c-cook marked this pull request as ready for review October 31, 2022 17:36
@j-c-cook
Copy link
Contributor Author

@zariiii9003 Okay, I made modifications.

@j-c-cook
Copy link
Contributor Author

j-c-cook commented Nov 5, 2022

@zariiii9003 tests are passing. Ready for your review.

@felixdivo felixdivo mentioned this pull request Nov 10, 2022
Merged
pierreluctg
pierreluctg reviewed Nov 10, 2022
View reviewed changes
Copy link
Collaborator

@pierreluctg pierreluctg left a comment

Choose a reason for hiding this comment

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

Hi @j-c-cook, why are we also removing GZip support for the other formats? I was under the impression that this PR was only to remove GZip for the BLF format, wich make sense because already uses compression internally and GZip is not really necessary.

Anything wrong with for example the ASC GZip support?

@pierreluctg pierreluctg added the file-io about reading & writing to files label Nov 10, 2022
@j-c-cook
Revert "add busParams and use snake case for VectorChannelConfig (har…
6cfed6b 
…dbyte#1422)"

This reverts commit fb892d6.
@j-c-cook
Revert "update docs (hardbyte#1421)"
a8d0fb1 
This reverts commit d3103d8.
@j-c-cook
Revert "improve vector documentation (hardbyte#1420)"
661a39c 
This reverts commit 4b43f33.
@j-c-cook
Revert "update github actions (hardbyte#1409)"
0095c9c 
This reverts commit 0c82d2c.
@j-c-cook
Revert "setup.cfg: Use license_files instead of license_file (hardbyt…
705da47 
…e#1408)"

This reverts commit b587878.
@j-c-cook
Revert "explicitly set supported file formats (hardbyte#1406)"
89f9f1f 
This reverts commit 99fea55.
@j-c-cook
Revert "Fix Sphinx warnings (hardbyte#1405)"
9d119c0 
This reverts commit 451ce48.
@j-c-cook
Revert "Modify file_size help doc string (hardbyte#1401)"
d76b674 
This reverts commit b639560.
@j-c-cook
Revert "Test load_config() (hardbyte#1396)"
149a20c 
This reverts commit a144f25.
@j-c-cook
Revert "Move windows-curses dependency to an optional extra (hardbyte…
fc01cf0 
…#1395)"

This reverts commit 89c395f.
@j-c-cook
Revert "Update BufferedReader.get_message docstring (hardbyte#1397)"
3f1de8d 
This reverts commit 23b6b19.
@j-c-cook
Revert "Pass file mode to compress function (hardbyte#1384)"
429bcce 
This reverts commit b2b2a80.
@j-c-cook
Revert "Provide meaningful error message for xlGetApplConfig error (h…
547e885 
…ardbyte#1392)"

This reverts commit c3a5c7a.
@j-c-cook
Revert "Test on vector virtual bus if XL API is available (hardbyte#1390
614f650 
)"

This reverts commit 366e239.
@j-c-cook
Revert "VectorBus init refactoring (hardbyte#1389)"
366cd45 
This reverts commit 1e11f21.
@j-c-cook
Revert "extend XL api wrapper (hardbyte#1387)"
b721741 
This reverts commit 40f6cce.
@j-c-cook
Revert "socketcan: Make find_available_interfaces() find slcanX inter…
ceaed9d 
…faces (hardbyte#1369)"

This reverts commit ad8b948.
@j-c-cook
Revert "Fix _default_name for compressed files (hardbyte#1383)"
f6edc4a 
This reverts commit 9d9cb16.
@j-c-cook
Merge branch 'develop' into issue1378_handleblfgz
7c0e650
@j-c-cook
Copy link
Contributor Author

@pierreluctg I've made a mistake here. I'll open up a new PR.

@j-c-cook j-c-cook closed this Nov 11, 2022
@j-c-cook j-c-cook deleted the issue1378_handleblfgz branch November 12, 2022 02:20
j-c-cook added a commit to j-c-cook/python-can that referenced this pull request Nov 12, 2022
@j-c-cook
Add gzip check to compress method
cc18642 
Addresses @zariiii9003 from hardbyte#1385 (comment)
@j-c-cook j-c-cook mentioned this pull request Nov 12, 2022
Merged
@j-c-cook
Copy link
Contributor Author

@pierreluctg @zariiii9003 This PR is now migrated to #1429.

@felixdivo felixdivo added the bug label Nov 13, 2022
@felixdivo felixdivo added this to the 4.1.0 Release milestone Nov 13, 2022
zariiii9003 added a commit that referenced this pull request Nov 13, 2022
@j-c-cook @zariiii9003
Add gzip check to compress method (#1429)
f739bcc 
* Add gzip check to compress method

Addresses @zariiii9003 from #1385 (comment)

* Update can/io/logger.py

Co-authored-by: zariiii9003 <52598363+zariiii9003@users.noreply.github.com>

* Update logger.py

Co-authored-by: zariiii9003 <52598363+zariiii9003@users.noreply.github.com>
@zariiii9003 zariiii9003 removed this from the 4.1.0 Release milestone Feb 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pierreluctg pierreluctg pierreluctg left review comments

Assignees

@j-c-cook j-c-cook

Labels

bug file-io about reading & writing to files

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

GzipFile BLFWriter negative seek error

4 participants

@j-c-cook @zariiii9003 @pierreluctg @felixdivo