[icedtea-web] RFC: add support for packEnabled and versionEnabled
Dr Andrew John Hughes
ahughes at redhat.com
Thu Feb 10 00:27:28 PST 2011
On 08:26 Thu 10 Feb , Dr Andrew John Hughes wrote:
> On 18:13 Wed 09 Feb , Omair Majid wrote:
> > On 02/09/2011 05:33 PM, Dr Andrew John Hughes wrote:
> > > On 13:04 Wed 09 Feb , Omair Majid wrote:
> > >> On 02/09/2011 11:42 AM, Dr Andrew John Hughes wrote:
> > >>> On 15:42 Wed 26 Jan , Omair Majid wrote:
> > >>>> Hi,
> > >>>>
> > >>>> The attached patch adds support for packEnabled and versionEnabled
> > >>>> properties in JNLP files. This allows JNLP applications to use pack200
> > >>>> compression and versioned jars without requiring special server
> > >>>> configuration/software. It also fixes bug RH669942.
> > >>>>
> > >>>> The patch adds a new class DownloadOptions which specify download
> > >>>> options for jars (currently just version and pack200 support). Another
> > >>>> new class ResourceUrlCreator is used to get a list of valid urls for a
> > >>>> resource. ResourceTracker now uses ResourceUrlCreator to get the list of
> > >>>> valid urls for downloading a jar and then tries to find the best one.
> > >>>> initializeResource() and downloadResource() both use this url.
> > >>>> downloadResource() has been modified to look at the file extension as
> > >>>> well as the content-encoding when deciding whether a file is compressed
> > >>>> or not.
> > >>>>
> > >>>> The patch is much bigger than I had expected it to be. It also affects
> > >>>> core functionality - downloading jars. I would appreciate any comments
> > >>>> and suggestions for improvements.
> > >>>>
> > >>>> Thanks,
> > >>>> Omair
> > >>>>
> > >>>> [1] https://bugzilla.redhat.com/show_bug.cgi?id=669942
> > >>>
> > >>> Comments inline.
> > >>>
> > >>
> > >> Thanks for looking it over.
> > >>
> > >>>> diff -r 64da2a80df88 netx/net/sourceforge/jnlp/cache/ResourceTracker.java
> > >>>> --- a/netx/net/sourceforge/jnlp/cache/ResourceTracker.java Tue Jan 25 10:19:20 2011 -0500
> > >>>> +++ b/netx/net/sourceforge/jnlp/cache/ResourceTracker.java Wed Jan 26 15:31:51 2011 -0500
> > >>>> @@ -636,16 +648,16 @@
> > >>>> String contentEncoding = con.getContentEncoding();
> > >>>>
> > >>>> if (JNLPRuntime.isDebug()) {
> > >>>> - System.err.println("Content encoding for " + resource.location + ": "
> > >>>> - + contentEncoding);
> > >>>> + System.out.println("Downloading" + resource.location + " using " +
> > >>>> + realLocation + " (encoding : " + contentEncoding + ")");
> > >>>> +
> > >>>
> > >>> Shouldn't this still be System.err?
> > >>>
> > >>
> > >> I like to send most debug output to System.out, but I dont think it's a
> > >> big deal. I can change it if you like.
> > >>
> > >
> > > I'd prefer System.err but I could live with System.out as long as we're consistent.
> > >
> > > Is there output on System.out that appears regardless of whether debugging is on or off?
> > > If so, the debug dependent stuff should be on System.err so it can be captured separately.
> > >
> >
> > Netx is not supposed to output anything to console - other than
> > exceptions that we want the user to see. If it does (outside of
> > debug/verbose mode) it's a bug.
> >
> > At the moment, it's a fairly mixed bag: some parts use system.out,
> > others use system.err:
> >
> > $ grep -rin 'err.print' netx | wc -l
> > 43
> > $ grep -rin 'out.print' netx | wc -l
> > 130
> >
> > I suppose I can make everything use System.err, but that would be
> > separate patch. For this patch, which one should I use?
> >
>
> Ok so use System.out for the exceptions the user should always see,
> and System.err for debug output. Then 2> can capture the debug output
> separate from the other output.
>
> I agree it should be a separate cleanup patch.
>
> I think they are both if (debug) cases in this patch so use System.err.
>
> > >>>> @@ -686,7 +698,8 @@
> > >>>> inputStream.close();
> > >>>> gzInputStream.close();
> > >>>>
> > >>>> - } else if (contentEncoding.equals("pack200-gzip")) {
> > >>>> + } else if ("pack200-gzip".equals(contentEncoding) ||
> > >>>> + realLocation.getPath().endsWith(".pack.gz")) {
> > >>>> GZIPInputStream gzInputStream = new GZIPInputStream(new FileInputStream(
> > >>>> CacheUtil.getCacheFile(downloadLocation, resource.downloadVersion)));
> > >>>> InputStream inputStream = new BufferedInputStream(gzInputStream);
> > >>>
> > >>> I'm not sure these changes are necessary. Although you eliminate the null check, it does create
> > >>> a lot of additional noise for no apparent change. Maybe something for a seperate patch?
> > >>>
> > >>
> > >> No, it's actually required here. It's the simplest way I can think of to
> > >> make the existing code that dealt with content-encoding also work for
> > >> pack.gz files. Before, we would ask the server for foo.jar and the
> > >> server could send it with content-encoding pack200-gzip (by actually
> > >> sending foo.pack.gz). With this patch, we ask the server for foo.pack.gz
> > >> directly. The server sends foo.pack.gz with null content encoding. We
> > >> must deal with this in the exact same way as foo.jar with content
> > >> encoding pack200-gzip.
> > >>
> > >> Notice these lines in the patch:
> > >> - } else if (contentEncoding.equals("pack200-gzip")) {
> > >> + } else if ("pack200-gzip".equals(contentEncoding) ||
> > >> + realLocation.getPath().endsWith(".pack.gz")) {
> > >>
> > >> If realLocation.getPath().endsWith(".pack.gz") is true, then
> > >> contentEncoding is null. Perhaps I should have made it more explicit.
> > >
> > > You've snipped some of what I was commenting about. I agree the additional
> > > conditional needs to be there but there's a lot of change in that area just
> > > to flip the equals comparison which hides the actual if extension. The bit
> > > above that you snipped is JUST a change in the equals and could be done
> > > in a separate patch.
> > >
> >
> > Yes, but it's necessary here. This was the overall code before:
> >
> > if (contentEncoding != null) {
> > if (contentEncoding.equals("gzip")) {
> > // handle gzip
> > } else if (contentEncoding.equals("pack200-gzip")) {
> > // handle pack200-gzip
> > }
> > }
> >
> > I want to execute the same code as in the pack200-gzip branch but using
> > explicit files. In this case,
> > realLocation.getPath().endsWith(".pack.gz") is true but contentEncoding
> > is null (since the server does not encode the content). So the code becomes:
> >
> > if (contentEncoding.equals("gzip")) {
> > // handle gzip
> > } else if (contentEncoding.equals("pack200-gzip") ||
> > realLocation.getPath().endsWith(".pack.gz")) {
> > // handle pack200-gzip
> > }
> >
> > But since contentEncoding is null, the first if statement will throw a
> > NullPointerException. The smallest change I can think of to make this
> > work is to flip the equals (since "constant string".equals(null) returns
> > false). The following code handles (or should handle, barring any bugs)
> > both implicit and explicit pack200 compression correctly.
> >
> > if ("gzip".equals(contentEncoding)) {
> > // handle gzip
> > } else if ("pack200-gzip".equals(contentEncoding) ||
> > realLocation.getPath().endsWith(".pack.gz")) {
> > // handle pack200-gzip
> > }
> >
> > Does that make more sense?
> >
>
> Ok. My concern was not with this block but the one prior to it which
> you snipped from the e-mail.
>
Ignore that, seems they are both the same after all.
> > As a side note, this patch messes up indentation - I have removed the
> > original "if (contentEncoding != null)" but left the indentation there.
> >
> > Cheers,
> > Omair
>
> --
> Andrew :)
>
> Free Java Software Engineer
> Red Hat, Inc. (http://www.redhat.com)
>
> Support Free Java!
> Contribute to GNU Classpath and IcedTea
> http://www.gnu.org/software/classpath
> http://icedtea.classpath.org
> PGP Key: F5862A37 (https://keys.indymedia.org/)
> Fingerprint = EA30 D855 D50F 90CD F54D 0698 0713 C3ED F586 2A37
--
Andrew :)
Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)
Support Free Java!
Contribute to GNU Classpath and IcedTea
http://www.gnu.org/software/classpath
http://icedtea.classpath.org
PGP Key: F5862A37 (https://keys.indymedia.org/)
Fingerprint = EA30 D855 D50F 90CD F54D 0698 0713 C3ED F586 2A37
More information about the distro-pkg-dev
mailing list