On 04/08/2016 07:54 PM, Steve Drach wrote:
I’ve put up a new webrev that addresses the issue of having spaces before (and
after) the value “true” in the Multi-Release attribute.
Is some or all of that really necessary? since the we can specify domain of
values.
I think it is. The spec states that one can have an arbitrary amount of
leading/trailing spaces in the value part of the attribute. Apparently
attribute values could also have “hidden” characters such as vertical tab or
nak for example. I wish the spec was tighter here. I’m merely stating the the
domain can be
“ *TRUE *” for upper/lower case letters.
AFAICT the so called “spec" says nothing about trimming white space from values and
although not explicitly called out the actual value has to be what constitutes the
character sequence for the “otherchar" definition (otherwise the continuation space
and newline characters would also be part of the actual value and that makes no sense).
No it doesn’t say you can trim white space, but if one doesn’t, then do we
accept “true”, “ true”, “ true”, etc.?
So it seems you may have quite a bit of wiggle room here :-) and it’s up to
each attribute definition to state what constitutes its domain of possible
valid values based on “other char”.
So, we can say *otherchar is upper/lower case “true” only?
For example, the Sealed attribute can take on a value of “true” or “false”
(case ignored), but there is no white space trimming.
Well then “Sealed: true” won’t work, but the spec says it should work.
Where does the “spec” state that it should?
It really comes down to the interpretation of otherchar. If it can be
interpreted on an attribute by attribute basis then it’s not a problem.
The value is literally the string “ true”, at least in one interpretation of
the “spec” :-)
I’m fine with that. And we really should do something about the spec, it’s too
loose and ambiguous.
I suspect it’s ok to go with finding “Multi-Release: true” (lower/upper case)
as in your first patch, then check if it is terminated with a newline (and
ignore the continuation case)
Okay. I’d like Claes to weigh in because he’s the one who brought it up. He’s
traveling today, so I don’t expect to hear from him soon.
Yeah, I'm guilty of raising the silly question of whether or not
accepting untrimmed true is OK according to spec.
With a generous interpretation and the presence of prior art (Sealed) I
think it's OK to go back to the first version of the patch (with the
addition to check that Multi-Release: true is followed by a LF, CR or
the end of the manifest) and add a similar note to the spec/notes that
Multi-Release has the same value restrictions as Sealed.
Perhaps both cases should be clarified in the notes to say that
leading/trailing whitespace is not accepted, since that isn't directly
obvious to a casual reader.
Thanks!
/Claes
Paul.
I am fine without doing this nonsense, but I think we need to change the spec to
make it correct. We could change the definition of "otherchar” to something
like this
otherchar:=ALPHA EXTALPHANUM*
ALPHA:=[A-Z | a-z | _]
EXTALPHANUM:=[ALPHA | 0-9 | SPACE]
Even this will still allow trailing spaces, so after matching TRUE we’d need to
make sure the next char is SPACE, \r, or \n.
Other ideas?
Paul.
Hi,
Please review this simple fix to require that the jar manifest Multi-Release
attribute has a value of “true" in order to be effective, i.e. to assert the
jar is a multi-release jar.
issue: https://bugs.openjdk.java.net/browse/JDK-8153213
<https://bugs.openjdk.java.net/browse/JDK-8153213>
webrev: http://cr.openjdk.java.net/~sdrach/8153213/webrev/index.html
<http://cr.openjdk.java.net/~sdrach/8153213/webrev/index.html>
Thanks
Steve
