-
Notifications
You must be signed in to change notification settings - Fork 162
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
Add OutputGzipFile
, in order to create a gzip compressed file
#4128
Conversation
Which PR are you referring to? #4410 doesn't seem to exist on |
Sorry, meant #4110 (which I've corrected in the first message, in case anyone else comes along) |
b0674f3
to
4dd30a6
Compare
4dd30a6
to
f73982b
Compare
There was a failing test which is, I believe, unrelated to this PR but I have fixed here anyway -- ReadLine can get only a partial line when reading from a process which hasn't yet finished outputting a whole line. |
Since I've run into the PackageManager issue myself now (paging @mtorpey), I've thought about this PR. First off, for PackageManager, I suggest that it directly downloads into a file when possible, sidestepping the double compression issue (see also gap-packages/PackageManager#59). But overall, I am scared that the double compression bug will hit more people; while the automatic gzip compression is a cool feature, it's also a trap inviting regressions and weird bugs. This PR attempts to mitigate it by at least adding a way to bypass the issue (so an opt-out solution). But this strikes me as the wrong approach: It still breaks existing code which needs to be adapted (and then the adaption means that old code won't work with old GAP versions). Shouldn't we rather return the default behavior to what it was (i.e., no automatic compression), and then add a way to opt-in to compression? E.g. via a |
At the very least, we should remove the automatic compression from the |
f73982b
to
a81f6c1
Compare
Now significantly changed based on feedback (although the internals are similar). Remove all 'Raw' methods. Add 'OutputGzipFile' (to match OutputTextFile), which gzips anything. OutputTextFile now never compresses. InputTextFile still always uncompresses (as it has for many years, although now it does it more situations). Even if this was disabled, you still couldn't read gzipped files with InputTextFile, because we have always (and we have no way to disable currently) rewriten line endings in windows, which would break binary files. |
029de4e
to
dea6cd0
Compare
lib/streams.gd
Outdated
@@ -726,14 +726,16 @@ DeclareOperation( "OutputTextString", [ IsList, IsBool ] ); | |||
## <#GAPDoc Label="OutputTextFile"> | |||
## <ManSection> | |||
## <Oper Name="OutputTextFile" Arg='filename, append'/> | |||
## <Oper Name="OutputGzipFile" Arg='filename, append'/> | |||
## | |||
## <Description> | |||
## <C>OutputTextFile( <A>filename</A>, <A>append</A> )</C> returns an output stream in the | |||
## category <C>IsOutputTextFile</C> that writes received characters to the file | |||
## <A>filename</A>. If <A>append</A> is <K>false</K>, then the file is emptied first, | |||
## otherwise received characters are added at the end of the file. | |||
## If <A>filename</A> ends in <C>.gz</C> then the file will be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, wait, isn't this supposed to have been changed with this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, your message is too brief for me to guess the problem?
My aim is now OutputTextFile has it's 4.10 behaviour -- it never gzips, and you use OutputGzipFile explicitly if you want to output with gzip compression.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see the problem, yes, docs are still wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I noticed I was still compressing things like OpenLog
. Maybe we want that, but for now let's do the minimal.
The only thing I'm now compressing if it ends .gz without prompting is SaveWorkspace, because it's just a binary file most people wouldn't understand anyway, and it compresses really well.
dea6cd0
to
6912564
Compare
6c5f6d3
to
531507f
Compare
Sorry, just cleaning up and pushing some changes. I am having one consistent problem, which is this patch seems to stop HPCGAP from even getting to the gap prompt, which is strange and I'm investigating. |
531507f
to
09b5b70
Compare
EDIT: Found the issue, was simple (needed to fix HPC-GAP's console code to use new functions) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
The testmanuals check failed to download the bootstrap archive; I've restarted the checks to make sure it passes then (unfortunately one can only restart all checks at once...)
OutputGzipFile
, in order to create a gzip compressed file
OutputGzipFile
, in order to create a gzip compressed fileOutputGzipFile
, in order to create a gzip compressed file
Continues #4110, as I stupidly put that branch in gap-system/gap, and shouldn't do that. All suggested fixes from that patch have been applied here.