Re: RFR JDK-8206389: JarEntry.setCreation/LastAccessTime without setLastModifiedTime causes Invalid CEN header
OK, looks good! I would add something to check() to make sure that e.g. atime == null iff ze.getLastAccessTime() == null The zip time stuff is surprisingly messy. On Fri, Jul 6, 2018 at 8:50 PM, Xueming Shen wrote: > On 7/6/18, 5:43 PM, Martin Buchholz wrote: > > I would use different timestamps for the 4 possible times used in this > test, if only to make it clearer which value comes from where. > > > webrev has been updated accordingly. > > > +// ze.setLastModifiedTime(now);+ > ze.setTime(now.toMillis()); > > > setTime only sets the DOS time? Which only has a granularity of 2 > seconds? If so, how do you get back the same value you put in if the > current second is odd? Or am I misunderstanding the test? > > > no misunderstanding, good catch. The test does fail when hits the odd > second. > added a special "check" version for the 2 second granularity set/getTime(). > > Thanks, > Sherman > > >
Re: Prototype of jpackager in jdk/sandbox [was: Draft JEP proposal: JDK-8200758: Packaging Tool]
- Mail original -
> De: "Kevin Rushforth"
> À: "Remi Forax"
> Cc: "core-libs-dev" , "Alexey Semenyuk"
> , "Andy Herrick"
>
> Envoyé: Samedi 7 Juillet 2018 15:47:01
> Objet: Re: Prototype of jpackager in jdk/sandbox [was: Draft JEP proposal:
> JDK-8200758: Packaging Tool]
> Hi Remy,
>
> Thank you for taking a look.
>
> Yes, the javapackager code that forms the basis for the jpackager
> prototype was initially developed on older JDKs and evolved from there.
> I'm sure the improvements you suggest are all good ones, and it doesn't
> seem like it would be too hard to address the most important of them,
> especially the try-with-resources or anything else that would affect the
> robustness of the tool. As long as we do address the robustness issues,
> I think it is more important to get the feature set right, and make sure
> that the public interfaces -- the command line options and ToolProvider
> interface -- are clean. I don't see the need to rewrite the tool or take
> an extra couple of months to modernize all of the implementation to use
> JDK 11 APIs everywhere.
>
> Also, I don't agree that jpackager is too large for jdk/sandbox or that
> it needs it own project. The jdk/sandbox is perfect for new modules /
> new tools that don't impact other parts of the JDK.
>
> -- Kevin
Hi Kevin,
like you, i don't think that a full rewrite is necessary, as you said having
the right public 'interfaces' is enough,
but reducing the size the duplicated code (with the JDK and internally) is as
important in my opinion.
For the first point, it means that jpackager should use jopt for the argument
parsing (to be fully compatible with the GNU style of options).
For the second point, it means to change a lot of code that may break because
it's less mechanical than introducing try-with-resources.
regards,
Rémi
>
>
> On 7/6/2018 3:07 PM, Remi Forax wrote:
>> I've just taking a look at the patch,
>> i don't see how this can be integrated soon, the code is consistently
>> inconsistent as one of my colleague would say, even the coding conventions
>> are
>> not respected.
>> i believe that's it's because the code have been written first in Java 6 an
>> without refactoring was moved to use Java 7, 8, 9, 10 and 11.
>>
>> The I/O code still using java.io.File for some parts, no try-with-resources
>> so
>> most of the try/finally are not safe,
>> a lot of code like new BufferedWriter(new FileWriter(file)) instead of
>> Files.newBufferedWriter, etc. The code should use the package java.nio.file,
>> and not the old java.io,
>> most of the code try to manage the exception right were they appear instead
>> of
>> propagating them so there are too many try/catch,
>> a lot of catch are ignored which is a code smell, some codes use the internal
>> logger (jdk.packager.internal.Log), but a lot of codes doesn't,
>> for the collection code, there is a lot of copy of data small structures
>> (which
>> suggest that published collections are not immutable),
>> there are dubious @SuppressWarnings("unchecked"), some or them are due to the
>> fact that the code use Class as a type token instead of using lambdas,
>> Stream are not used when they should (to avoid multiple copy between data
>> structures) and streams that need to be closed are not (the result of
>> Files.list by example),
>> there are usual "don't do that in Java" like a loop using an integer index to
>> traverse a List without knowing if it's a random access list or not,
>> there is a lot of nullchecks instead of using Optional,
>> a lot of code initialize local variables to null which is a code smell (and a
>> side effect of having a lot of try/catch but not only),
>> constructors should not do work, just initialization, use static factory
>> method
>> instead (so you will not have to debug half constructed objects),
>> the code uses BigIntegers to parse a bundle version, just in case,
>> the code uses an AtomicReference as a box that a lambda can mutate, instead
>> of
>> wrapping the exception into a runtime and unwrapping it at call site,
>> The code of jdk.packager.internal.IOUtils should be updated to use methods of
>> the JDK 11 and methods like readFully should be replaced by the JDK's one.
>> listOfPathToString and setOfStringToString are just WTF, like in
>> getRedistributableModules(), where the variable stream is an Optional,
>> A class like Platform should be used everywhere to do platform specific
>> stuff, a
>> lot of code still use String matching (the version parsing should use
>> System.Version).
>> All the argument parsing should be delegated to JOpt (the one integrated with
>> the JDK), so it will be consistent with the other JDK tools,
>> There is a UnsupportedPlatformException but a Platform can be UNKOWN ??
>>
>> I will stop here, my point is that there is a lot of cleaning that should
>> appear
>> before the code is integrated into the JDK and given the size of the code, i
>> wonder if it's not better to start an OpenJDK project
Re: Prototype of jpackager in jdk/sandbox [was: Draft JEP proposal: JDK-8200758: Packaging Tool]
Hi Remy,
Thank you for taking a look.
Yes, the javapackager code that forms the basis for the jpackager
prototype was initially developed on older JDKs and evolved from there.
I'm sure the improvements you suggest are all good ones, and it doesn't
seem like it would be too hard to address the most important of them,
especially the try-with-resources or anything else that would affect the
robustness of the tool. As long as we do address the robustness issues,
I think it is more important to get the feature set right, and make sure
that the public interfaces -- the command line options and ToolProvider
interface -- are clean. I don't see the need to rewrite the tool or take
an extra couple of months to modernize all of the implementation to use
JDK 11 APIs everywhere.
Also, I don't agree that jpackager is too large for jdk/sandbox or that
it needs it own project. The jdk/sandbox is perfect for new modules /
new tools that don't impact other parts of the JDK.
-- Kevin
On 7/6/2018 3:07 PM, Remi Forax wrote:
I've just taking a look at the patch,
i don't see how this can be integrated soon, the code is consistently
inconsistent as one of my colleague would say, even the coding conventions are
not respected.
i believe that's it's because the code have been written first in Java 6 an
without refactoring was moved to use Java 7, 8, 9, 10 and 11.
The I/O code still using java.io.File for some parts, no try-with-resources so
most of the try/finally are not safe,
a lot of code like new BufferedWriter(new FileWriter(file)) instead of
Files.newBufferedWriter, etc. The code should use the package java.nio.file,
and not the old java.io,
most of the code try to manage the exception right were they appear instead of
propagating them so there are too many try/catch,
a lot of catch are ignored which is a code smell, some codes use the internal
logger (jdk.packager.internal.Log), but a lot of codes doesn't,
for the collection code, there is a lot of copy of data small structures (which
suggest that published collections are not immutable),
there are dubious @SuppressWarnings("unchecked"), some or them are due to the
fact that the code use Class as a type token instead of using lambdas,
Stream are not used when they should (to avoid multiple copy between data
structures) and streams that need to be closed are not (the result of
Files.list by example),
there are usual "don't do that in Java" like a loop using an integer index to
traverse a List without knowing if it's a random access list or not,
there is a lot of nullchecks instead of using Optional,
a lot of code initialize local variables to null which is a code smell (and a
side effect of having a lot of try/catch but not only),
constructors should not do work, just initialization, use static factory method
instead (so you will not have to debug half constructed objects),
the code uses BigIntegers to parse a bundle version, just in case,
the code uses an AtomicReference as a box that a lambda can mutate, instead of
wrapping the exception into a runtime and unwrapping it at call site,
The code of jdk.packager.internal.IOUtils should be updated to use methods of
the JDK 11 and methods like readFully should be replaced by the JDK's one.
listOfPathToString and setOfStringToString are just WTF, like in
getRedistributableModules(), where the variable stream is an Optional,
A class like Platform should be used everywhere to do platform specific stuff,
a lot of code still use String matching (the version parsing should use
System.Version).
All the argument parsing should be delegated to JOpt (the one integrated with
the JDK), so it will be consistent with the other JDK tools,
There is a UnsupportedPlatformException but a Platform can be UNKOWN ??
I will stop here, my point is that there is a lot of cleaning that should
appear before the code is integrated into the JDK and given the size of the
code, i wonder if it's not better to start an OpenJDK project for it and
iterate on the code before trying to include it in the JDK. For me, the code is
too big to use the jdk/sandbox.
regards,
Rémi
- Mail original -
De: "Kevin Rushforth"
À: "core-libs-dev"
Cc: "Alexey Semenyuk" , "Andy Herrick"
Envoyé: Vendredi 6 Juillet 2018 22:14:29
Objet: Prototype of jpackager in jdk/sandbox [was: Draft JEP proposal:
JDK-8200758: Packaging Tool]
An initial prototype of the jpackager tool has been pushed to a new
'JDK-8200758-branch' branch in the JDK sandbox [1]. If anyone is
interested in taking a look, you can clone it as follows:
hg clone http://hg.openjdk.java.net/jdk/sandbox
cd ./sandbox
hg update JDK-8200758-branch
I plan to reply to the feedback already provided, and update the JEP [2]
next week some time, but in the mean time if you have additional
questions or comments, feel free to reply.
-- Kevin
[1] http://hg.openjdk.java.net/jdk/sandbox/shortlog/JDK-8200758-branch
[2]
Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives
Hi Matthias, I am not a reviewer and neither do I have enough knowledge about whether jar/file _names_ are considered security sensitive. However, the patch that's proposed for this change, prints the file _path_ (and not just the name). That I believe is security sensitive. -Jaikiran On 06/07/18 6:14 PM, Baesken, Matthias wrote: Hi Alan ,so it looks like JDK-8204233 added a switch (system property) to enable the enhanced socket IOException messages . That would be an option as well for 8205525 . 8205525 adds the jar file name and the line number info to the exception message . In case that only the jar file name would be considered sensitive , I would prefer to just output the line number (and omit the system property ). What do you think ? Best regards, Matthias -Original Message- From: Alan Bateman [mailto:alan.bate...@oracle.com] Sent: Montag, 25. Juni 2018 16:52 To: Baesken, Matthias ; core-libs- d...@openjdk.java.net Cc: Lindenmaier, Goetz Subject: Re: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives On 25/06/2018 15:29, Baesken, Matthias wrote: Hi, do you consider both the file name and line number as sensitive ? There was a similar discussion on net-dev recently related to leaking host names in exceptions. Something similar may be needed here Do you know the outcome of this discussion ? All the details are in JDK-8204233 and the associated CSR. -Alan
