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 support of man-page producers. #139

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add support of man-page producers. #139

wants to merge 3 commits into from

Conversation

roman-kashitsyn
Copy link

Lintian wants a man page entry for every executable included into the
package and it wants them gzipped. It's inconvenient to add another
maven plugin to just gzip a man page entry during build. In addition,
it's not so easy to find a correct location for a man page using only
the Debian Policy Manual.

This patch provides a new man-page producer type that can guess
correct location for a page and gzip it. Unfortunately, it looks
impossible to gzip a page with maximal compression level as lintian
wants.

In addition, it removes some code duplication by introducing the
Producers package-private class to hold common constants and methods
and the ProducerFactory class to create producers from run-time
configuration in the ant task and the maven plugin.

Lintian wants a man page entry for every executable included into the
package and it wants them gzipped. It's inconvenient to add another
maven plugin to just gzip a man page entry during build. In addition,
it's not so easy to find a correct location for a man page using only
the Debian Policy Manual.

This patch provides a new `man-page` producer type that can guess
correct location for a page and gzip it. Unfortunately, it looks
impossible to gzip a page with maximal compression level as lintian
wants.

In addition, it removes some code duplication by introducing the
`Producers` package-private class to hold common constants and methods
and the `ProducerFactory` class to create producers from run-time
configuration in the ant task and the maven plugin.
@roman-kashitsyn
Copy link
Author

Any comments on this patch?

@ebourg
Copy link
Collaborator

ebourg commented Dec 13, 2013

Hi Roman, thank you for the patch. It's quite big and I haven't everything yet, it would have been preferable to split the refactoring from the feature addition.

You can get the best compression level with gzip by subclassing GZIPOutputStream:

GZIPOutputStream out = new GZIPOutputStream(out) {
    {
        def.setLevel(Deflater.BEST_COMPRESSION);
    }
};

@roman-kashitsyn
Copy link
Author

Hello Emmanuel,

Thank you very much for your feedback. I will try to reduce patch sizes in future since I don't like large patches too. Special thanks for your advise with GZIP stream.

@roman-kashitsyn
Copy link
Author

Unfortunately, the trick with anonymous GZIPOutputStream subclass doesn't work: the stream writes header in a constructor with a hard-coded compression method, it doesn't ask the deflater (if it even were, it would not see the changes made by the anonymous subclass later).
lintian is still complain on such a compressed file :(

@ebourg
Copy link
Collaborator

ebourg commented Dec 13, 2013

Interesting, that looks like a bug in the JDK. We should be able to work around this issue by modifying the gzip compressor in Commons Compress to write a custom header.

@roman-kashitsyn
Copy link
Author

As I can see, the GzipOutputStream from commons.compress just delegates it's work to java.util.zip.GZIPOutputStream. Possibly, the only option is to write a simple utility using the Deflater directly.

@ebourg
Copy link
Collaborator

ebourg commented Dec 13, 2013

Yes, that's what I'm doing currently :)

@roman-kashitsyn
Copy link
Author

Actually, there is a simpler way. The compression level is encoded in the header flag XFL (http://www.gzip.org/zlib/rfc-gzip.html) that is now hard-coded to zero.
Since we have the compressed file loaded to memory, we could use the trick with anonymous subclass and patch the XFL flag to be 2. I'll try to implement that.

Roman Kashitsyn added 2 commits December 14, 2013 15:08
This patch introduces some low-level hacks on GZIP files to make
lintian happy regarding the man page compression level.
Conflicts:
	docs/maven.md
	src/main/java/org/vafer/jdeb/producers/DataProducerFile.java
@ebourg
Copy link
Collaborator

ebourg commented Dec 16, 2013

Commons Compress now has a better implementation of the gzip output stream. Could you give it a try Roman? The code is on the trunk and will be part of the 1.7 release:

http://svn.apache.org/repos/asf/commons/proper/compress/trunk/

@roman-kashitsyn
Copy link
Author

Yep, it's what is needed. I suppose, the pull request merge is delayed until 1.7 release of commons-compress since it's not a good idea to copy the stream implementation into the jdeb source tree.

@ebourg
Copy link
Collaborator

ebourg commented Jan 28, 2014

Commons Compress 1.7 has been released if you want to rework your patch Roman.

Regarding the additions of a producer, I'm not a big fan of this solution. Following this trend we may quickly end up with a myriad of producers addressing various types of files. jdeb could be smarter and transform automatically the input files to comply with the Debian Policy (compress automatically the man pages, make the files in /usr/bin executable,etc). This could be performed by a filter mechanism, I haven't figured how to implement it though.

From a user point of view it would be easier to just say "package this set of files" and let jdeb do the right things than having to learn the subtleties of the Debian Policy and handle every special case with a specific producer.

@roman-kashitsyn
Copy link
Author

I think the alternative for the producer is another <data> option

<man-page category="N" prefix="/usr/share/man/manN" compressor="gzip"/>

with reasonable defaults for all the attributes.

@tcurdt
Copy link
Owner

tcurdt commented Feb 13, 2014

I hate to see a contribution wasted but I think this probably should be solved differently.
We could accept it for 1.x (not eager) ...but then we should really start thinking of 2.x to solve this properly.

@roman-kashitsyn
Copy link
Author

I was busy with other stuff, but I'll definitely return to this feature in short time. I appreciate any ideas how to implement the documentation support in more general and non-intrusive way. It's also unfair to provide man pages but ignore info pages.
I'll rework the solution as soon as I find a good design for it. Sometimes no feature is better than poorly implemented one...

@roman-kashitsyn
Copy link
Author

Actually, the main problem here is compression. The problem with hard-to-find man page location could be easily solved with a couple of examples in the jdeb manual.
So, I think it's enough to provide ability to compress files on archive construction. I think we don't need any new producers here, just an additional data attribute (e.g. compression):

<data>
  <src>${project.basedir}/src/main/resources/app.1</src>
  <type>file</type>
  <compression type="gzip"/>
  <mapper>
    <type>perm</type>
    <prefix>/usr/share/man/man1</prefix>
  </mapper>
</data>

This also should work for info pages.

The other solution here is to (optionally) scan maven resources and locate all the man pages and info pages, compress them and put into the package. This solution fits maven philosophy better but doesn't look composable with other parts of jdeb.

@tcurdt tcurdt added this to the 2.0 milestone Feb 26, 2014
@tcurdt tcurdt added 2.x and removed 2.x labels Dec 15, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants