[GitHub] ant-ivy pull request #73: Enable XML report parser to produce qualified extr...

classic Classic list List threaded Threaded
32 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[GitHub] ant-ivy pull request #73: Enable XML report parser to produce qualified extr...

asfgit
GitHub user aprelev opened a pull request:

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

    Enable XML report parser to produce qualified extra attributes

    ### Issue
    `${ivy.deps.changed}` is always `true` for dependencies with extra attributes.
   
    ### Problem
    `XmlReportParser::startElement()` creates revision IDs [as follows](https://github.com/apache/ant-ivy/blob/5918182e0d6836d89c42260da9de4428d4cbcec0/src/java/org/apache/ivy/plugins/report/XmlReportParser.java#L96):
    ```Java
    mrid = ModuleRevisionId.newInstance(organisation, module, branch, revision,
        ExtendableItemHelper.getExtraAttributes(attributes, "extra-"));
    ```
    Here, `ExtendableItemHelper::getExtraAttributes()` method returns *unqualified* attributes of previously resolved dependencies, which are then compared with *qualified* attributes of currently resolved dependencies in `ConfigurationResolveReport::checkIfChanged()` [as follows](https://github.com/apache/ant-ivy/blob/5918182e0d6836d89c42260da9de4428d4cbcec0/src/java/org/apache/ivy/core/report/ConfigurationResolveReport.java#L101):
    ```Java
    Set<ModuleRevisionId> previousDepSet = new HashSet<>(
        Arrays.asList(parser.getDependencyRevisionIds()));
    hasChanged = !previousDepSet.equals(getModuleRevisionIds());
    ```
    which effectively renders sets of dependecies *unequal*.
   
    ### Solutions
    One solution would be to compare unqualified attributes when [testing revision IDs for equality](https://github.com/apache/ant-ivy/blob/5918182e0d6836d89c42260da9de4428d4cbcec0/src/java/org/apache/ivy/core/module/id/ModuleRevisionId.java#L237); another one would be to [produce qualified attributes](https://github.com/aprelev/ant-ivy/blob/1d508c14bbc68411b9b215f2e4e552fe20d3ae1a/src/java/org/apache/ivy/util/extendable/ExtendableItemHelper.java#L37) when parsing XML report.
    I've implemented second solution (it seemed cleaner to me), and included unit-test to demonstrate the issue.

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/aprelev/ant-ivy master

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/ant-ivy/pull/73.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #73
   
----
commit 1d508c14bbc68411b9b215f2e4e552fe20d3ae1a
Author: aprelev <arseny.aprelev@...>
Date:   2018-07-31T14:15:26Z

    Enable XML report parser to produce qualified extra attributes

----


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

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

asfgit
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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

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

asfgit
In reply to this post by asfgit
Github user jaikiran commented on the issue:

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



---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

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

asfgit
In reply to this post by asfgit
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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

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

asfgit
In reply to this post by asfgit
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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

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

asfgit
In reply to this post by asfgit
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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

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

asfgit
In reply to this post by asfgit
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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

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

asfgit
In reply to this post by asfgit
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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

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

asfgit
In reply to this post by asfgit
Github user jaikiran commented on the issue:

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



---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

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

asfgit
In reply to this post by asfgit
Github user aprelev commented on the issue:

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


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

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

asfgit
In reply to this post by asfgit
Github user jaikiran commented on the issue:

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


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

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

asfgit
In reply to this post by asfgit
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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

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

asfgit
In reply to this post by asfgit
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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

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

asfgit
In reply to this post by asfgit
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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

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

asfgit
In reply to this post by asfgit
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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

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

asfgit
In reply to this post by asfgit
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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

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

asfgit
In reply to this post by asfgit
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<separator><namespace-prefix><separator>-<name>" 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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] ant-ivy issue #73: IVY-1104 Enable XML report parser to produce qualified ex...

asfgit
In reply to this post by asfgit
Github user aprelev commented on the issue:

    https://github.com/apache/ant-ivy/pull/73
 
    @twogee, why don't we just stick with deterministic encoding like
    ```Java
    extra-<qualifiedName> // e.g. extra-e:foo, extra-m:classifier, extra-foo
    ```
    since both `'-'` and `':'` are allowed name characters? This would also incur minimum changes to the codebase: literally passing `::getQualifiedExtraAttributes()` to `XmlReportWriter::extraToString()` so qualifiers can be picked up by `XmlReportParser::startElement()` automatically.
   
    Or, if you want to avoid using `':'` in the report (if original `extra-` prefix was supposed to replace the namespace, as you guessed it was), we could use another deterministic encoding like
    ```Java
    extra-<qualifier>-<name> // e.g. extra-e-foo, extra-m-classifier, extra--foo
    ```
    which would require additional encoding and decoding overhead in both XML report parser and writer, but otherwise imply no restrictions on naming?
   
    Also, in the name of code clarity and with respect to single responsibility principle, in my opinion, it would be better if `ExtendableItemHelper` handled both encoding and decoding of attributes (as of now, encoding is handled by `XmlReportWriter::extraToString()`).


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] ant-ivy issue #73: IVY-1104 Enable XML report parser to produce qualified ex...

asfgit
In reply to this post by asfgit
Github user twogee commented on the issue:

    https://github.com/apache/ant-ivy/pull/73
 
    Keeping `':'` would require XML report to deal with all those pesky namespaces, because it must be a valid XML.
   
    Using a simple `'-'` as a separator may cause trouble if somebody decides to use a namespace prefix with `'-'`. That's why I insist on something less likely to be used.  I hope dev list would comment, too.
   
    +1 for placing codec routines into `ExtendableItemHelper`.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] ant-ivy issue #73: IVY-1104 Enable XML report parser to produce qualified ex...

asfgit
In reply to this post by asfgit
Github user aprelev commented on the issue:

    https://github.com/apache/ant-ivy/pull/73
 
    > Using a simple '-' as a separator may cause trouble if somebody decides to use a namespace prefix with '-'.
   
    I don't actually see a problem with that: we can
    - either escape dashes with some other character, like `.` (`-ns-:-foo.` → `extra-.-ns.--.-foo..`),
    - or replace them with `&#45;` (`-ns-:-foo.` → `extra-&#45;ns&#45;-&#45;foo.`).


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

12