Skip to content

Conversation

@rocallahan
Copy link
Contributor

The current working directory may not be writable so it's better to write files explicitly to /tmp and clean them up afterward.

@rocallahan rocallahan marked this pull request as draft July 1, 2024 08:18
@rocallahan
Copy link
Contributor Author

I need to work out a Windows-compatible solution here.

@rocallahan rocallahan force-pushed the serialize-temp-file branch 2 times, most recently from 95460d5 to f5e7011 Compare July 2, 2024 01:11
@rocallahan rocallahan marked this pull request as ready for review July 2, 2024 01:46
@rocallahan rocallahan marked this pull request as draft July 2, 2024 02:37
@rocallahan
Copy link
Contributor Author

Actually it looks like a lot of tests assume the current working directory is writable, so we should fix this another way.

@rocallahan rocallahan force-pushed the serialize-temp-file branch from f5e7011 to c6b66cf Compare July 2, 2024 02:45
@rocallahan rocallahan marked this pull request as ready for review July 2, 2024 02:46
@codecov
Copy link

codecov bot commented Jul 2, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 12 lines in your changes missing coverage. Please review.

Project coverage is 83.66%. Comparing base (93a37e8) to head (c6b66cf).
Report is 9 commits behind head on master.

Current head c6b66cf differs from pull request most recent head 1db53c9

Please upload reports for the commit 1db53c9 to get more accurate results.

Files Patch % Lines
include/mockturtle/io/serialize.hpp 50.00% 12 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #652      +/-   ##
==========================================
+ Coverage   83.47%   83.66%   +0.19%     
==========================================
  Files         187      187              
  Lines       29692    29740      +48     
==========================================
+ Hits        24784    24881      +97     
+ Misses       4908     4859      -49     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

I assume this was added for debugging purposes at some point.
The test doesn't need it and the current working directory might not be
writable.
…ng tests

The incoming current working directory may not be writable.
This is especially important when deserializing. If we don't catch
errors after reading `size`, we may use its uninitialized value as a
loop bound, which is very bad.
@costamag
Copy link
Contributor

@aletempiac, the coverage report is triggered by the branching added by @rocallahan. Testing all of them requires considering many cases, and from the code we can see that he is giving us a better handling of the files. Do you suggest to address the coverage issue or can we proceed with merging this PR?

@rocallahan rocallahan force-pushed the serialize-temp-file branch from 1db53c9 to 9101eb0 Compare July 15, 2024 04:38
@rocallahan
Copy link
Contributor Author

Hmm, the tests pass for me.

@rocallahan rocallahan force-pushed the serialize-temp-file branch from 9101eb0 to be07785 Compare July 15, 2024 06:05
@costamag
Copy link
Contributor

Dear @rocallahan,this is great, thank you for the tests!

@costamag costamag merged commit b78357b into lsils:master Jul 15, 2024
@rocallahan rocallahan deleted the serialize-temp-file branch July 16, 2024 00:46
costamag pushed a commit to costamag/mockturtle that referenced this pull request Dec 15, 2025
* Don't write `test.aig` from `write_aiger` test

I assume this was added for debugging purposes at some point.
The test doesn't need it and the current working directory might not be
writable.

* Set the current working directory to a temporary directory when running tests

The incoming current working directory may not be writable.

* Propagate I/O errors when (de)serializing

This is especially important when deserializing. If we don't catch
errors after reading `size`, we may use its uninitialized value as a
loop bound, which is very bad.

* Add tests for I/O error propagation
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