[GitHub] ant-ivy issue #73: Enable XML report parser to produce qualified extra attri...

2018-08-01 Thread twogee
Github user twogee commented on the issue:

https://github.com/apache/ant-ivy/pull/73
  
Sorry, my reasoning took the wrong turn, must be the heat 😦Yes, passing 
qualified attributes to XmlReportWriter is the way to go.

I wonder what was the reason for using "extra-" hack? I assume it was 
conceived as a way to avoid dealing with with random namespaces in the XML 
report. If it makes sense to continue with that, a way to encode the namespace 
prefix must be figured out.

That means one should look at the permitted characters in the [XML 
Specification](https://www.w3.org/TR/xml11/#NT-NameChar) and try to figure out 
a separator such that from a name constructed as  
"extra-" a namespace prefix and a 
name could be extracted unambiguously.

I propose using something like ".-_-." as a separator since it is unlikely 
to occur in any namespace prefixes for extra attributes describing Ivy 
artifacts (if somebody does that, it must be on purpose and thus an attempt to 
shoot oneself in the foot). Or should we rather deal with namespaces in XML 
reports?

P.S. You're right about the Jira issue, please add it to the name of the PR 
(and eventually to the commit name). 


---

-
To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org
For additional commands, e-mail: dev-h...@ant.apache.org



[GitHub] ant-ivy issue #73: Enable XML report parser to produce qualified extra attri...

2018-08-01 Thread aprelev
Github user aprelev commented on the issue:

https://github.com/apache/ant-ivy/pull/73
  
By the way, found [issue 
IVY-1104](https://issues.apache.org/jira/browse/IVY-1104) in your issue 
tracker, seems to be the case.


---

-
To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org
For additional commands, e-mail: dev-h...@ant.apache.org



[GitHub] ant-ivy issue #73: Enable XML report parser to produce qualified extra attri...

2018-08-01 Thread aprelev
Github user aprelev commented on the issue:

https://github.com/apache/ant-ivy/pull/73
  
As of now, two sets of dependencies are constructed in a different ways:

- *currently resolved dependecies* are produced by Ivy resolve engine, with 
**both qualified and unqualified attributes** (`super.qualifiedExtraAttributes 
!= super.extraAttributes`), and
- *previously resolved dependencies* are parsed from last report by 
`XmlReportParser`, with **unqualified attributes only** 
(`super.qualifiedExtraAttributes == super.extraAttributes`).

Set comparison at `ConfigurationResolveReport::checkIfChanged()` invokes 
`ModuleRevisionId::equals()` for individual dependencies comparison, which 
compares `super.qualifiedExtraAttributes` maps as in:
```Java
@Override
public boolean equals(Object obj) {
  ...
  return other.getRevision().equals(getRevision())
&& !(other.getBranch() == null && getBranch() != null)
&& !(other.getBranch() != null && 
!other.getBranch().equals(getBranch()))
&& other.getModuleId().equals(getModuleId())
&& 
other.getQualifiedExtraAttributes().equals(getQualifiedExtraAttributes()); //< 
here
}
```
which, obviously, yields a `false`, since keys in 
`other.getQualifiedExtraAttributes()` map are stripped of qualifiers as 
explained above.

That is why I introduced parameterised version of 
`ExtendableItemHelper::getExtraAttributes()` used by `XmlReportParser`, this 
way parsed dependencies have both versions of qualifiers, same as resolved 
dependencies.

As I pointed out in original PR message, issue may instead be solved by 
modifying `ModuleRevisionId::equals()` to use `super.extraAttributes`, provided 
attributes namespaces cannot clash, of course, which will result in ignoring 
qualifiers:
```Java
@Override
public boolean equals(Object obj) {
  ...
  return other.getRevision().equals(getRevision())
&& !(other.getBranch() == null && getBranch() != null)
&& !(other.getBranch() != null && 
!other.getBranch().equals(getBranch()))
&& other.getModuleId().equals(getModuleId())
&& other.getExtraAttributes().equals(getExtraAttributes()); //< 
ignoring qualifiers
}
```
At what point do you suggest we check for presense of `':'` in the names of 
attributes? 


---

-
To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org
For additional commands, e-mail: dev-h...@ant.apache.org



[GitHub] ant-ivy issue #73: Enable XML report parser to produce qualified extra attri...

2018-08-01 Thread twogee
Github user twogee commented on the issue:

https://github.com/apache/ant-ivy/pull/73
  
Gradle seemed to have [different 
ideas](https://github.com/gradle/gradle/issues/954) about Ivy XML namespaces 
(not to be confused with Ivy 
[namespaces](http://ant.apache.org/ivy/history/2.5.0-rc1/settings/namespaces.html)
 😉); perhaps we should keep the parser namespace-unaware and simply check 
for presence of `':'` in the attribute names of the artifact descriptors?


---

-
To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org
For additional commands, e-mail: dev-h...@ant.apache.org



[GitHub] ant-ivy issue #73: Enable XML report parser to produce qualified extra attri...

2018-08-01 Thread aprelev
Github user aprelev commented on the issue:

https://github.com/apache/ant-ivy/pull/73
  
@twogee Can you point me in the right direction regarding namespace URL? 
And what are your thoughts on including the namespace prefix in the report?


---

-
To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org
For additional commands, e-mail: dev-h...@ant.apache.org



[GitHub] ant-ivy issue #73: Enable XML report parser to produce qualified extra attri...

2018-08-01 Thread twogee
Github user twogee commented on the issue:

https://github.com/apache/ant-ivy/pull/73
  
@aprelev the latter, you can push a new commit. What I meant is that 
hardcoding "e:" is probably a bad idea, one may choose a different namespace 
prefix; rather, we should be enforcing use of namespace URL for extra 
attributes.


---

-
To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org
For additional commands, e-mail: dev-h...@ant.apache.org



[GitHub] ant-ivy issue #73: Enable XML report parser to produce qualified extra attri...

2018-08-01 Thread jaikiran
Github user jaikiran commented on the issue:

https://github.com/apache/ant-ivy/pull/73
  
>> Should I reopen PR?


---

-
To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org
For additional commands, e-mail: dev-h...@ant.apache.org



[GitHub] ant-ivy issue #73: Enable XML report parser to produce qualified extra attri...

2018-08-01 Thread aprelev
Github user aprelev commented on the issue:

https://github.com/apache/ant-ivy/pull/73
  
Should I reopen PR?


---

-
To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org
For additional commands, e-mail: dev-h...@ant.apache.org



[GitHub] ant-ivy issue #73: Enable XML report parser to produce qualified extra attri...

2018-08-01 Thread jaikiran
Github user jaikiran commented on the issue:

https://github.com/apache/ant-ivy/pull/73
  
retest this please



---

-
To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org
For additional commands, e-mail: dev-h...@ant.apache.org



[GitHub] ant-ivy issue #73: Enable XML report parser to produce qualified extra attri...

2018-08-01 Thread aprelev
Github user aprelev commented on the issue:

https://github.com/apache/ant-ivy/pull/73
  
> Rather than hardcoding prefix, shouldn't we retrieve it by URL 
"http://ant.apache.org/ivy/extra;?

Can you elaborate on prefix retrieving? The **extra-** prefix is [hardcoded 
at](https://github.com/apache/ant-ivy/blob/5918182e0d6836d89c42260da9de4428d4cbcec0/src/java/org/apache/ivy/plugins/report/XmlReportWriter.java#L175)
 `XmlReportWriter::extraToString()` regardless of attribute namespace:
```Java
sb.append(prefix).append("extra-").append(entry.getKey()).append("=\"")
.append(XMLHelper.escape(entry.getValue())).append("\"");
```

Maybe including namespace in XML report by passing *qualified* attributes 
to `XmlReportWriter::extraToString()` would be a better choice then?


---

-
To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org
For additional commands, e-mail: dev-h...@ant.apache.org



[GitHub] ant-ivy issue #73: Enable XML report parser to produce qualified extra attri...

2018-08-01 Thread aprelev
Github user aprelev commented on the issue:

https://github.com/apache/ant-ivy/pull/73
  
> Tests have passed https://builds.apache.org/job/Ivy-GithubPR/361/

Not really, my changes were not applied in this build.
Test *testDepsWithAttributesChanged* should have run under 
*org.apache.ivy.ant/IvyResolveTest* suite.


---

-
To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org
For additional commands, e-mail: dev-h...@ant.apache.org



[GitHub] ant-ivy issue #73: Enable XML report parser to produce qualified extra attri...

2018-08-01 Thread twogee
Github user twogee commented on the issue:

https://github.com/apache/ant-ivy/pull/73
  
@aprelev Rather than hardcoding prefix, shouldn't we retrieve it by URL 
"http://ant.apache.org/ivy/extra;?


---

-
To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org
For additional commands, e-mail: dev-h...@ant.apache.org



[GitHub] ant-ivy issue #73: Enable XML report parser to produce qualified extra attri...

2018-08-01 Thread jaikiran
Github user jaikiran commented on the issue:

https://github.com/apache/ant-ivy/pull/73
  
Tests have passed https://builds.apache.org/job/Ivy-GithubPR/361/



---

-
To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org
For additional commands, e-mail: dev-h...@ant.apache.org



[GitHub] ant-ivy issue #73: Enable XML report parser to produce qualified extra attri...

2018-08-01 Thread jaikiran
Github user jaikiran commented on the issue:

https://github.com/apache/ant-ivy/pull/73
  
>> By the way, latest commits 43ddccb and 5918182 introduced nested 
"javaversion" element, which breaks Jenkins CI build; I had to remove this for 
both
I just fixed the PR build to use the latest 1.9.x version of Ant to build 
the project. 


---

-
To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org
For additional commands, e-mail: dev-h...@ant.apache.org



[GitHub] ant-ivy issue #73: Enable XML report parser to produce qualified extra attri...

2018-08-01 Thread jaikiran
Github user jaikiran commented on the issue:

https://github.com/apache/ant-ivy/pull/73
  
retest this please



---

-
To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org
For additional commands, e-mail: dev-h...@ant.apache.org



[GitHub] ant-ivy issue #73: Enable XML report parser to produce qualified extra attri...

2018-08-01 Thread aprelev
Github user aprelev commented on the issue:

https://github.com/apache/ant-ivy/pull/73
  
By the way, latest commits 43ddccb859b94c79350ece7520af4c991c2bb5e6 and 
5918182e0d6836d89c42260da9de4428d4cbcec0 introduced [nested "javaversion" 
element](https://github.com/apache/ant-ivy/blob/5918182e0d6836d89c42260da9de4428d4cbcec0/build.xml#L29),
 which breaks Jenkins CI build; I had to remove this for both 

- OpenJDK 1.8.0_171, Ant 1.9.6 (July 8, 2015), Ubuntu 16.04, and
- Java 1.8.0_181, Ant 1.9.6 (April 16, 2016), Windows 10.1709

in order to compile Ivy.


---

-
To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org
For additional commands, e-mail: dev-h...@ant.apache.org