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

Add OutputGzipFile, in order to create a gzip compressed file #4128

Merged
merged 1 commit into from
Nov 20, 2020

Conversation

ChrisJefferson
Copy link
Contributor

@ChrisJefferson ChrisJefferson commented Oct 8, 2020

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.

@coveralls
Copy link

coveralls commented Oct 8, 2020

Coverage Status

Coverage decreased (-0.0009%) to 93.724% when pulling f73982b on ChrisJefferson:raw-file into 0f8f080 on gap-system:master.

@wilfwilson
Copy link
Member

Which PR are you referring to? #4410 doesn't seem to exist on gap-system/gap (which is here).

@ChrisJefferson
Copy link
Contributor Author

Sorry, meant #4110 (which I've corrected in the first message, in case anyone else comes along)

@ChrisJefferson ChrisJefferson added the release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes label Oct 9, 2020
@ChrisJefferson
Copy link
Contributor Author

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.

@fingolfin fingolfin added this to In Progress in Better I/O system Oct 13, 2020
src/sysfiles.c Outdated Show resolved Hide resolved
lib/streams.gi Outdated Show resolved Hide resolved
@fingolfin
Copy link
Member

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 GzipOutputStream, which then also would work with arbitrary extensions?

@fingolfin
Copy link
Member

At the very least, we should remove the automatic compression from the stable-4.11 branch, so that if we ever make a 4.11.1 release it doesn't regress existing code; if we decide we want the regression, it should be in 4.12.0.

@ChrisJefferson
Copy link
Contributor Author

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.

@ChrisJefferson ChrisJefferson changed the title Add TextInputFileRaw and TextOutputFileRaw Add OutputGzipFile ( was TextInputFileRaw and TextOutputFileRaw ) Nov 17, 2020
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
Copy link
Member

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?

Copy link
Contributor Author

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.

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 see the problem, yes, docs are still wrong.

Copy link
Contributor Author

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.

src/sysfiles.c Outdated Show resolved Hide resolved
@fingolfin fingolfin mentioned this pull request Nov 20, 2020
src/io.c Outdated Show resolved Hide resolved
@ChrisJefferson
Copy link
Contributor Author

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.

@ChrisJefferson
Copy link
Contributor Author

EDIT: Found the issue, was simple (needed to fix HPC-GAP's console code to use new functions)

Copy link
Member

@fingolfin fingolfin left a 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...)

Better I/O system automation moved this from In Progress to Approved Nov 20, 2020
@fingolfin fingolfin merged commit 8138b8c into gap-system:master Nov 20, 2020
Better I/O system automation moved this from Approved to Done Nov 20, 2020
@ChrisJefferson ChrisJefferson deleted the raw-file branch February 15, 2021 14:39
@ThomasBreuer ThomasBreuer self-assigned this Feb 16, 2021
@ThomasBreuer ThomasBreuer changed the title Add OutputGzipFile ( was TextInputFileRaw and TextOutputFileRaw ) Added OutputGzipFile, in order to create a gzip compressed file Feb 16, 2021
@ThomasBreuer ThomasBreuer added release notes: added PRs introducing changes that have since been mentioned in the release notes and removed release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Feb 16, 2021
@ThomasBreuer ThomasBreuer removed their assignment Feb 16, 2021
@fingolfin fingolfin changed the title Added OutputGzipFile, in order to create a gzip compressed file Add OutputGzipFile, in order to create a gzip compressed file Aug 17, 2022
@fingolfin fingolfin added kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements kind: new feature labels Aug 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements kind: new feature release notes: added PRs introducing changes that have since been mentioned in the release notes
Projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants