[icedtea-web] RFC: add support for packEnabled and versionEnabled
Dr Andrew John Hughes
ahughes at redhat.com
Thu Feb 10 00:26:21 PST 2011
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.
> 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
More information about the distro-pkg-dev
mailing list